TraceKit can now be used on frames by aquadk · Pull Request #68 · csnover/TraceKit

aquadk · 2017-01-17T00:25:34Z

Hi, have made this change so, I only need to include TraceKit once on my frame page.

Here is the code for the frame page.

function insertTraceKit() {

  function yourLogger(errorReport) {
    console.log(errorReport);
    return true;
  }

  TraceKit.report.subscribe(yourLogger);

  function wrapped(aThis) {
    TraceKit.report.subscribe(yourLogger, aThis.target.contentWindow);
    try {
      aThis.target.contentWindow.onunload = unwrapped;
    } catch (e) {
      //console.log('cross-origin frame detected');
    }
  }

  function unwrapped(aThis) {
    TraceKit.report.unsubscribe(yourLogger, aThis.currentTarget);
  }

  var frames = document.getElementsByTagName('frame');
  for (var i = 0; i < frames.length; i++) {
    frames[i].onload = wrapped;
  };

}

var readyStateCheckInterval = setInterval(waitInit, 10, this);

function waitInit() {
  //console.log(document.readyState);
  if (document.readyState === "interactive") {
    clearInterval(readyStateCheckInterval);
    insertTraceKit();
  }
}

Hi, have made this change so, I only need to include TraceKit once on my frame page. Here is the code for the frame page. function insertTraceKit() { function yourLogger(errorReport) { console.log(errorReport); return true; } TraceKit.report.subscribe(yourLogger); function wrapped(aThis) { TraceKit.report.subscribe(yourLogger, aThis.target.contentWindow); try { aThis.target.contentWindow.onunload = unwrapped; } catch (e) { //console.log('cross-origin frame detected'); } } function unwrapped(aThis) { TraceKit.report.unsubscribe(yourLogger, aThis.currentTarget); } var frames = document.getElementsByTagName('frame'); for (var i = 0; i < frames.length; i++) { frames[i].onload = wrapped; }; } var readyStateCheckInterval = setInterval(waitInit, 10, this); function waitInit() { //console.log(document.readyState); if (document.readyState === "interactive") { clearInterval(readyStateCheckInterval); insertTraceKit(); } }

niemyjski

Thanks for the pr, There are some things I saw that need to be updated or clarified,

I'm really hesitant on accepting this. I do see the value in this, but want to know the exact benefits gained. Also, we haven't really had any users report not seeing errors in iframes. I'd like to move away from window specific code, and have this in a plugin ideally. Can you add some tests for all the code here specifically one that deals with frames.

niemyjski · 2017-01-17T11:56:50Z

tracekit.js

-        installGlobalHandler();
-        handlers.push(handler);
+    function subscribe(handler, aWindow) {
+      aWindow = (aWindow || window);


Can we rename aWindow to win it's shorter and you know what it does, it's also consistent with your function above.

niemyjski · 2017-01-17T12:01:00Z

tracekit.js

                }
            } else if ((parts = lineRE3.exec(lines[line]))) {
-                var url = window.location.href.replace(/#.*$/, '');
+          var url = TraceKit.windowPointer.location.href.replace(/#.*$/, '');


What about parsing source in the inner frame?

niemyjski · 2017-01-17T12:01:33Z

tracekit.js

            lineRE3 = /^\s*Line (\d+) of function script\s*$/i,
            stack = [],
-            scripts = (window && window.document && window.document.getElementsByTagName('script')),
+        scripts = (TraceKit.windowPointer && TraceKit.windowPointer.document && TraceKit.windowPointer.document.getElementsByTagName('script')),


Can you move this window pointer to a local variable, this will not minify very well.

niemyjski · 2017-01-17T12:01:56Z

tracekit.js

            var domain = '';
-            try { domain = window.document.domain; } catch (e) { }
+        try {
+          domain = window.document.domain;


Why not use the pointer here>

niemyjski · 2017-01-17T12:02:31Z

tracekit.js

-        _oldOnerrorHandler = window.onerror;
-        window.onerror = traceKitWindowOnError;
-        _onErrorHandlerInstalled = true;
+      var _oldOnerrorHandler = aWindow.onerror;


if this is no longer a private field we need to change the variable name

niemyjski · 2017-01-17T12:02:48Z

tracekit.js

+        notifyHandlers(stack, true, arguments);
        }

-        if (_oldOnerrorHandler) {


Any reason for removing this?

is moved to notifyHandlers

niemyjski · 2017-01-17T12:03:53Z

tracekit.js

+        if (_has(handlers, i) && handlers[i][1] === TraceKit.windowPointer) {
                try {
-                    handlers[i](stack, isWindowError, error);
+            handlers[i][0](stack, isWindowError, (aArguments.length > 4) ? aArguments[4] : null);


What's the 3rd and 4th parameter here? This became much less readable.

niemyjski · 2017-01-17T12:04:11Z

tracekit.js

                } catch (inner) {
                    exception = inner;
                }
+          if (handlers[i][1]._oldOnerrorHandler) {


I thought this was a local variable.

Changed aWindow til win

value is not private anymore

+ made code easier to read

aquadk · 2017-01-18T14:23:38Z

Hi,
Have now made the changes you asked for.

niemyjski · 2017-01-18T16:06:33Z

tracekit.js

     * @throws An exception if an error occurs while calling an handler.
     */
-    function notifyHandlers(stack, isWindowError, error) {
+    function notifyHandlers(stack, isWindowError, aArguments) {


What does arguments contain? Can we call this args? and maybe a comment to break down what the possible structure is.

ok, arguments contains all the arguments from onerror event

niemyjski · 2017-01-18T16:07:28Z

tracekit.js

                try {
-                    handlers[i](stack, isWindowError, error);
+                  var errorObj=(aArguments.length > 4) ? aArguments[4] : null;
+                  handlers[i][0](stack, isWindowError, errorObj);


This is kind of confusing, so we call a method with stack, isWindowErorr and an array with 4 items? We seem to only be using the error object, so it might be better to revert this signature to just be error object. Previous thought: Maybe we need to add a comment stating that these arguments are actually: message, url, lineNo, columnNo, errorObj (if we were on es6, restructuring would make this so much more readable but we can't use that :(`,

niemyjski · 2017-01-18T16:08:27Z

tracekit.js

                    exception = inner;
                }
+          // Call old onerror events
+          if (handlers[i][2]) {


We weren't doing this before were we? It's just really hard to follow this and make sense of what's going on.. We call all the other handlers in a try catch.

niemyjski · 2017-01-18T16:09:33Z

tracekit.js

    function traceKitWindowOnError(message, url, lineNo, columnNo, errorObj) {
        var stack = null;
-
+      TraceKit.windowPointer = this;


So this modifies the pointer which might of been set earlier when you subscribed. I'm not sure this should be updated as it would change the subscription handler? Thoughts?

niemyjski · 2017-01-18T16:14:12Z

tracekit.js

+      if (isWindowAccessible(win)) {
        for (var i = handlers.length - 1; i >= 0; --i) {
-            if (handlers[i] === handler) {
+          if (handlers[i][0] === handler && win === handlers[i][1]) {


What's all stored on handler now in this array?

We weren't doing this before, but since you are calling unsubscribe and you have an array with the previous error handler. Do you think we should be setting it back?

Good point, i will insert the code

niemyjski · 2017-01-18T16:15:53Z

tracekit.js

    var debug = false,
-        sourceCache = {};
+        sourceCache = {},
+        curWin = TraceKit.windowPointer;


Can we rename this to win

niemyjski · 2017-01-18T16:16:29Z

tracekit.js

     * @memberof TraceKit.computeStackTrace
     */
    function computeStackTrace(ex, depth) {
+        curWin = TraceKit.windowPointer;


This doesn't seem to be used

niemyjski · 2017-01-18T16:16:54Z

tracekit.js

-        var originalFn = window[fnName];
-        window[fnName] = function traceKitAsyncExtension() {
+      var originalFn = TraceKit.windowPointer[fnName];
+      TraceKit.windowPointer[fnName] = function traceKitAsyncExtension() {


Might want to store this in a variable

niemyjski · 2017-06-29T01:36:29Z

I'm sorry for taking so long to get back to this. What is the current status of this?

aquadk · 2017-07-03T07:42:55Z

I have updated the code for Iframe page too.


   /**    
     * Description    
     * @param {type} aTracefunc Description    
     */
    function insertTraceKit(aTracefunc) {

      TraceKit.report.subscribe(aTracefunc);

      function wrapped(aThis) {
        var w = (aThis.target || aThis);

        function docUnloadChange(e) {
          TraceKit.report.unsubscribe(yourLogger, e.currentTarget);
          //console.log('unload', e.currentTarget.location.href, e.currentTarget.readyState);
          var l = e.currentTarget.location.href;
          var checkForUnload = function (e) {
            try {
              if (l != e.location.href) {
                clearInterval(unloadCheckInterval);
                //console.log(e.location.href);
                TraceKit.report.subscribe(aTracefunc, e);
              }
            } catch (exception) {
              clearInterval(unloadCheckInterval);
              unloadCheckInterval = setInterval(checkForUnload, 100, this);
              //console.log('cross-origin frame detected');
            }
          };
          var unloadCheckInterval = setInterval(checkForUnload, 1, this);

        }

        TraceKit.report.subscribe(aTracefunc, w.contentWindow);
        try {
          w.contentWindow.addEventListener("unload", docUnloadChange);
        } catch (e) {
          //console.log('cross-origin frame detected');
        }
      }

      var frames = document.querySelectorAll('frame,iframe');
      for (var i = 0; i < frames.length; i++) {
        frames[i].onload = wrapped;
        try {
          // console.log(frames[i].contentDocument.readyState, frames[i].contentDocument.location.href);
          frames[i].readyStateCheckInterval = setInterval(fasteractiveTrace, 1, frames[i]);
        } catch (e) {
          //console.log('cross-origin frame detected');
        }
      }

      function fasteractiveTrace(e) {
        try {
          if (e.contentDocument) {
            if (e.contentDocument.readyState == "complete" || e.contentDocument.readyState == "interactive") {
              wrapped(e);
              clearInterval(e.readyStateCheckInterval);
            }
            // console.log(e.contentDocument.readyState, e.contentDocument.location.href);
          }
        } catch (exception) {
          //console.log('cross-origin frame detected');
        }
      }

    }

    var readyStateCheckInterval = setInterval(waitInit, 10, this);

    function waitInit() {
      //console.log(document.readyState);
      if (document.readyState === "complete" || document.readyState === "interactive") {
        clearInterval(readyStateCheckInterval);
        insertTraceKit(yourLogger);
      }
    }

niemyjski · 2017-07-03T13:22:56Z

Thanks, were any other changes required for this pr? Do you think we should put this in a wiki entry?

niemyjski · 2018-02-23T17:06:03Z

Thanks, were any other changes required for this pr? Do you think we should put this in a wiki entry?

niemyjski · 2018-07-09T19:41:16Z

Thanks, were any other changes required for this pr? Do you think we should put this in a wiki entry? I'd like to get this merged in and cleaned up

niemyjski · 2018-07-09T19:43:46Z

tracekit.js

-            if (lastException === ex) {
+        window.setTimeout(function () {
+            if (lastException == ex) {
                processLastException();


Any specific reason to putting this to window? It might block users from using this in node?

niemyjski · 2018-07-09T19:44:10Z

tracekit.js

            };

-        notifyHandlers(stack, true, arguments);
+            notifyHandlers(stack, true, null);


any reason for not passing through the arguments?

niemyjski · 2018-07-09T19:44:17Z

tracekit.js

@@ -204,17 +215,16 @@ TraceKit.report = (function reportModuleWrapper() {
      TraceKit.windowPointer = this;


any reason for not passing through the arguments?

New handler object, which include Onunhandledrejection too.

package version upgraded

aquadk · 2018-07-17T12:07:58Z

I have merged our code and committed all changes.

niemyjski

I noticed that you merged in the lastest master changes but rejected all of those. Can you please go back over this pr and make sure we aren't missing anything like

```
   installGlobalHandler();
```

   installGlobalUnhandledRejectionHandler(); That would be greatly appreciated

# Conflicts: # tracekit.js

aquadk · 2018-07-18T12:35:06Z

Forgot to push all changes.. sorry

niemyjski · 2018-07-30T14:08:20Z

Hello, I'm not seeing any of the latest changes, can you please review the files changed here as you can see we added more code around installing additional global handlers.

aquadk · 2018-08-15T07:02:02Z

All code are pushed into aquadk/Tracekit master branch. Including global handlers.

niemyjski · 2018-08-18T13:06:18Z

tracekit.js

        }

-        if (_oldOnerrorHandler) {
-            return _oldOnerrorHandler.apply(this, arguments);


I think we were calling old handlers to preserve existing work flows.

niemyjski · 2018-08-18T13:08:28Z

tsconfig.json

+{
+    "compilerOptions": {
+        "module": "commonjs",
+        "target": "es6",


We actually have to target es3 everywhere :.

niemyjski · 2018-08-18T13:08:58Z

Looks good, I added two comments on things, if we feel we are good I'll merge it in.

niemyjski · 2018-09-07T13:49:52Z

@aquadk Could you please review the feedback so we can take your great work and get it merged in.

ghostd · 2018-09-10T08:15:54Z

tracekit.d.ts

+declare global {
+    interface Window {
+        onunhandledrejection: PromiseRejectionEvent;
+        _onErrorHandlerInstalled : boolean;


Do we really need to expose this private var?

I don't think so.

niemyjski · 2018-11-20T19:54:32Z

Can we get this updated so we can get it merged in.

niemyjski · 2020-04-08T13:38:19Z

Can we get this updated so we can get it merged in.

niemyjski · 2024-09-19T14:51:08Z

Can we get this updated so we can get it merged in.

niemyjski · 2026-01-27T02:17:47Z

There are a lot of merge conflicts and I'd really like to get this merged in, would it be possible to resolve the conflicts. Really sorry it's taken so long.

niemyjski requested changes Jan 17, 2017

View reviewed changes

aquadk added 3 commits January 18, 2017 11:04

rename param

1ba9d85

Changed aWindow til win

removed _

a182223

value is not private anymore

Added local value for window pointer

93a7322

+ made code easier to read

aquadk closed this Jan 18, 2017

aquadk reopened this Jan 18, 2017

Set theme jekyll-theme-tactile

69b165e

niemyjski requested changes Jan 18, 2017

View reviewed changes

aquadk added 2 commits January 18, 2017 20:12

Doc and better names on function arguments

343dc39

Doc and return value

0645272

Merge branch 'master' into master

7453eba

niemyjski reviewed Jul 9, 2018

View reviewed changes

altru-ist added 6 commits July 17, 2018 13:54

Validates better with typescript now.

3577701

New handler object, which include Onunhandledrejection too.

Changed fra grunt to gulp

9b0c75d

package version upgraded

Updated karma/ jasmine with chrome for debugging

66aa36c

jsdoc output to doc folder

c17ff27

VS code config for typescript

c4f0d94

Only use oldhandler if there is one.

ca67a8f

Merge branch 'master' into master

7f870a5

niemyjski requested changes Jul 18, 2018

View reviewed changes

Merge branch 'master' of https://github.com/aquadk/TraceKit

f416dd1

# Conflicts: # tracekit.js

Added code to doc, about Iframe

6864a38

niemyjski reviewed Aug 18, 2018

View reviewed changes

tsconfig.json

{

"compilerOptions": {

"module": "commonjs",

"target": "es6",

Copy link

Collaborator

niemyjski Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually have to target es3 everywhere :.

niemyjski mentioned this pull request Sep 7, 2018

Catch unhandled promise rejection events #72

Closed

ghostd reviewed Nov 17, 2018

View reviewed changes

		@@ -204,17 +215,16 @@ TraceKit.report = (function reportModuleWrapper() {
		TraceKit.windowPointer = this;

Conversation

aquadk commented Jan 17, 2017

Uh oh!

niemyjski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aquadk commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemyjski commented Jun 29, 2017

Uh oh!

aquadk commented Jul 3, 2017

Uh oh!

niemyjski commented Jul 3, 2017

Uh oh!

niemyjski commented Feb 23, 2018

Uh oh!

niemyjski commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aquadk commented Jul 17, 2018

Uh oh!

niemyjski left a comment

Choose a reason for hiding this comment

Uh oh!

aquadk commented Jul 18, 2018

Uh oh!

niemyjski commented Jul 30, 2018

Uh oh!

aquadk commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemyjski commented Aug 18, 2018