Bug 55092 - window.onerror should pass the ErrorEvent's 'error' property as the 5th argument to the event handler
: window.onerror should pass the ErrorEvent's 'error' property as the 5th argum...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To: Joseph Pecoraro
: InRadar
Depends on: 158192
Blocks: 158657
  Show dependency treegraph
 
Reported: 2011-02-23 15:14 PST by Charles Kendrick
Modified: 2016-06-13 20:26 PDT (History)
32 users (show)

See Also:


Attachments
Patch (90.11 KB, patch)
2014-02-24 13:55 PST, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (112.49 KB, patch)
2016-06-11 00:23 PDT, Joseph Pecoraro
buildbot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (959.21 KB, application/zip)
2016-06-11 01:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.51 MB, application/zip)
2016-06-11 01:31 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (113.26 KB, patch)
2016-06-13 13:46 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Kendrick 2011-02-23 15:14:25 PST
When adding an onerror handler, the uncaught exception is not provided, and using "new Error().stack" also does
not work: it produces a stack which ends at onerror.

Other means of acquiring stacks, like walking the arguments.caller or arguments.callee.caller chain, are likewise unavailable.

This makes onerror mostly useless because you can get better diagnostics by putting try/catch blocks around your top-level event handling code, where you will get a valid error.stack.  But this is cumbersome and doesn't catch every scenario.

So at the moment we (Isomorphic, creators of SmartClient/SmartGWT) need to continue to recommend that users launch Internet Explorer to get definitive error reporting, which is a shame.

<html>
  <body>
    <script>
      window.onerror = function onErrorHandler (message, url, linenumber, arg4) {
         alert("Error via new Error(): " + new Error().stack);

         var caughtError;
         try {
            throw new Error("whoops");
         } catch (e) {
            caughtError = e;
         }
         alert("Error via try..catch: " + caughtError.stack);

         alert("args.caller, args.callee.caller: " + [arguments.caller, arguments.callee.caller]);
       };

      function func3 () {
        crash();
      }

      function func2 () {
        func3();
      }

      function func1 () {
        func2();
      }

      func1();
    </script>

  </body>
</html>
Comment 1 Garrett Smith 2011-02-23 23:18:13 PST
I believe this bug may be INVALID.

`new Error().stack` shouldn't have anything other than itself in its stack. What else would that new Error possibly have on it?

I'm pretty sure that what was called a problem is not a problem at all. 

Perhaps what you want to do is find what caused onerror to fire, yeah? Perhaps you want error handler that takes the thrown error as a parameter.

function handler(errorEvent) {
  alert(errorEvent.stack);
}


window.addEventListener("scripterror", handler, false);
// OR:
// window.onscripterror = handler;

asdf4++; // cause an error.

And when that error is thrown, then the browser fires a scripterror at the window and a scripterror is just an error so it has the stack, type, message, etc. Post to WHATWG, this is OT. 

Is this bug INVALID?
Comment 2 Charles Kendrick 2011-02-24 09:04:58 PST
new Error().stack in other circumstances provides the full stack, for example, calling element.focus() and checking new Error().stack in an onfocus handler shows the stack including code that called element.focus().  onerror is behaving specially here.

Your suggestion of passing in or otherwise making available the original error object is interesting, and perhaps something you could raise with WhatWG, but is separate from the problem of new Error().stack providing incomplete information within onerror.
Comment 3 Timothy Quinn 2011-08-11 14:12:04 PDT
(In reply to comment #2)
I agree with the essence of the bug in that the window.onerror simply does not provide the functionality required for inline debugging. Its about as useful as IE's "Object not defined" ubiquitous error.

I strongly suggest further discussion of this to try and nail down a solution to make window.onerror more useful in WebKit.

In FireFox, we get an error object and thus we can read the error.stack and parse the result. It's not pretty, but it works as I have a full stack of source file names and line numbers which is 95% of what I need. Personally, this is my preferred

IE supports the call stack crawling and this has some nice features like being able to inspect the arguments of each level within the call stack. This methodology does not contain the source file names or line numbers which in its one kind of sucks.

If we can get both methods that would be absolutely awesome :)

PLEASE don't kill this request off or at least help us get to a solution that brings final closer to this fundamental gap in WebKit. I've been waiting for years for this fix and would love to get my libraries ported over to WebKit but for now, I just cannot support WebKit because it lacks a decent master error handler.

- TCQ

BTW I really do care about error handlers. I'm the dude who drove critical fix for Mozilla 1.3 which brought it up to par for better debugging: https://bugzilla.mozilla.org/show_bug.cgi?id=158592
Comment 4 Xavier Morel 2012-02-06 08:00:48 PST
> In FireFox, we get an error object and thus we can read the error.stack and parse the result.

Could you explain? As far as I know and according to #355430 `window.onerror` does not provide any error object in Firefox. Which is in fact bothersome.

355430: https://bugzilla.mozilla.org/show_bug.cgi?id=355430
Comment 5 Timothy Quinn 2012-02-07 13:56:04 PST
(In reply to comment #4)
Wow, I am embarrassed. You are correct that window.onerror in Firefox does not contain the error object in the parameters. I got around this by setting an global variable in my error handlers and then throwing the error up to the window.onerror where I would then retrieve the last error. Sorry for the confusion ( old code ;).

I totally agree that there should be a common ground for all browsers that have a master error handler and that such an error handler receive the last un-handled error as one of its parameters. 

I would wish that the standards organization would also streamline the error object so that it has a system readable and traversable error stack like in Java's StackTraceElement[] return for Throwable::getStackTrace(). This would be a huge win for JavaScript.

I would love to see WebKit be proactive and build such an improved master error handler but IMO, this may not happen unless the ECMA script specification can be updated. I've been meaning to take the time to try and write a detailed justification and specification but I have no free time to do this justice on my own. If anybody is interested in collaborating, I'd be very interested in working together.
Comment 6 Timothy Quinn 2012-04-26 22:46:27 PDT
Wow, no need to collaborate, looks like Bug 66571 implements a stack like error.stack in Firefox. Thanks everyone :)

Now if we can just get this ticket implemented and WebKit can be one step ahead of Firefox.
Comment 7 Timothy Quinn 2012-06-15 19:30:09 PDT
It appears that there is significant progress being made on bug 40118 which is driving towards building an Error.stack capability into JavaScriptCore. Fingers crossed...
Comment 8 Bret Little 2012-07-16 14:04:27 PDT
(In reply to comment #7)

It seems as though that issue is now resolved, what now needs to be done to expose the stack trace inside the onerror callback?
Comment 9 kangax 2012-12-05 05:15:33 PST
Is there anything else blocking this?
Comment 10 Peter Flynn 2013-05-01 17:27:52 PDT
See also Chromium bug on this subject: https://code.google.com/p/chromium/issues/detail?id=147127

Another bug with this request, https://bugs.webkit.org/show_bug.cgi?id=104408, was closed based on security concerns. But that doesn't seem like a blocker - if you only provide a stack when the exception came from the same origin, this will still be incredibly useful for many developers.  There are threads like https://code.google.com/p/chromium/issues/detail?id=159566 that show Chromium, at least, does some origin-based onerror sanitizing already - not sure if WebKit also does that today.
Comment 11 Adam Roben (:aroben) 2014-02-24 11:04:07 PST
*** Bug 104408 has been marked as a duplicate of this bug. ***
Comment 12 Adam Roben (:aroben) 2014-02-24 11:05:20 PST
Sanitizing away the error object for cross-origin errors should be pretty easy.

I'm thinking about starting to work on this. We're collecting JS exception backtraces on github.com from browsers that support this (right now, just Chrome I believe). It would be nice to be getting this information from Safari users.
Comment 13 Adam Roben (:aroben) 2014-02-24 11:12:09 PST
According to the HTML spec (http://html5.org/r/8086, http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#runtime-script-errors) we should be making the original error object available on ErrorEvent and passing it to window.onerror functions. Since we already have Error.stack, this will make the stack trace available as well.
Comment 14 Adam Roben (:aroben) 2014-02-24 13:55:31 PST
Created attachment 225094 [details]
Patch
Comment 15 Adam Roben (:aroben) 2014-02-24 13:57:39 PST
Comment on attachment 225094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225094&action=review

Here's an initial attempt at implementing this. I think it's pretty close; I've highlighted a couple of issues below. I'd love some feedback!

> Source/WebCore/ChangeLog:64
> +        * workers/DefaultSharedWorkerRepository.cpp:
> +        (WebCore::postExceptionTask):
> +        * workers/WorkerMessagingProxy.cpp:
> +        (WebCore::WorkerExceptionTask::performTask):
> +        Use a null error object reporting exceptions to the main Document or
> +        to Worker objects, as specified in the HTML spec.

I'm not sure any of our tests actually demonstrate this behavior. I need to see if there are some existing tests that I can improve, or if I need to write some new ones.

> LayoutTests/ChangeLog:87
> +        * userscripts/window-onerror-for-isolated-world-1-expected.txt:
> +        * userscripts/window-onerror-for-isolated-world-1.html:
> +        * userscripts/window-onerror-for-isolated-world-2-expected.txt:
> +        * userscripts/window-onerror-for-isolated-world-2.html:
> +        Test that column numbers and error objects get passed for errors in
> +        other isolated worlds.

This seems bad. It's an information leak between worlds. We already have a leak by dispatching error events between worlds at all, but the new error object makes it possible to pass actual JS values between worlds.

I think this is already an issue because of CustomEvent. Seems like we should probably do the same thing in both places.
Comment 16 Adam Roben (:aroben) 2014-02-24 13:58:29 PST
Sam, perhaps you can comment on the isolated world issue?

It'd be great to get some JS-y folks' eyes on this too. <3
Comment 17 Adam Roben (:aroben) 2014-02-24 14:03:30 PST
Comment on attachment 225094 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225094&action=review

> LayoutTests/http/tests/security/script-no-crossorigin-error-event-should-be-sanitized.html:12
> +    shouldBe("event.error", "null");

Looks like I should use shouldBeNull here.

> LayoutTests/http/tests/security/script-no-crossorigin-onerror-should-be-sanitized.html:17
> +    shouldBe("error", "null");

…and here.
Comment 18 Jérémy Lal 2014-09-10 16:18:39 PDT
Adam,
if leaking information between worlds is an issue with CustomEvent,
then it is an issue with KeyboardEvent keyIdentifier as well, no ?
Comment 19 Adam Roben (:aroben) 2014-09-12 07:36:58 PDT
(In reply to comment #18)
> if leaking information between worlds is an issue with CustomEvent,
> then it is an issue with KeyboardEvent keyIdentifier as well, no ?

AFAICT keyIdentifier is just a string, not an arbitrary JS object, so I don't think it has the same issue as CustomEvent and ErrorEvent.
Comment 20 David Børresen 2015-08-13 00:57:53 PDT
Any updates on the matter? 

Missing the 4th argument is really a pain. It's virtually impossible to handle custom exceptions across browsers when relying on window.onerror, and missing the thrown exception.
Comment 21 Timothy Quinn 2015-12-22 14:48:08 PST
FYI, this has been implemented in Chrome (chromium) and Firefox.

Both versions implement the same solution which window.onerror has 5 arguments: [message, url, line, column, errorObject]

I confirmed on linux using:
- electron v0.35.4 (chromium)
- chrome 47.0.2526.106 (64-bit) 
- firefox 42.1

The only webkit browser I tested was midoir 0.5.1.1 which implemented only the first 4 arguments.
Comment 22 Demian 2016-01-20 11:09:27 PST
Hello, is anyone working on Webkit nowadays? Hello?
Almost 6 years waiting for this 'feature' to be implemented and still no news...
Hello, I want to be able to capture stack traces inside the global error handler. Is that too much to ask?
It's year 2016 and Safari 9 is the new IE6. Really sad.
Comment 23 Radar WebKit Bug Importer 2016-04-14 11:42:48 PDT
<rdar://problem/25731279>
Comment 24 Brian Burg 2016-05-29 06:45:55 PDT
There are two issues to fix here:

 - ErrorEvent does not include the uncaught exception, if any, in the `error` property.
 - window.onerror does not include Error object as the a 4th argument.

I'm retitling this to narrow scope to the latter, and the former has a separate bug here:

https://bugs.webkit.org/show_bug.cgi?id=158192
Comment 25 Joseph Pecoraro 2016-06-10 17:39:38 PDT
I'll take this. I already started looking into this and ended up doing what looks almost exactly like Adam's patch.
Comment 26 Joseph Pecoraro 2016-06-10 19:14:42 PDT
> I think this is already an issue because of CustomEvent. Seems like we
> should probably do the same thing in both places.

Correct, CustomEvent has since solved this issue, so we should follow suit here.
Comment 27 Joseph Pecoraro 2016-06-11 00:23:49 PDT
Created attachment 281082 [details]
[PATCH] Proposed Fix

Thanks Adam for a great starting point!

This includes all of the tests you added, and some more tests (regarding workers and isolated worlds) to check we avoid leaking cross world like CustomEvent.details.
Comment 28 WebKit Commit Bot 2016-06-11 00:26:48 PDT
Attachment 281082 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:57:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebCore/dom/ErrorEvent.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 92 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Build Bot 2016-06-11 01:21:03 PDT
Comment on attachment 281082 [details]
[PATCH] Proposed Fix

Attachment 281082 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1482729

New failing tests:
userscripts/window-onerror-for-isolated-world-1.html
userscripts/window-onerror-for-isolated-world-2.html
userscripts/window-onerror-for-isolated-world-3.html
Comment 30 Build Bot 2016-06-11 01:21:10 PDT
Created attachment 281085 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 31 Joseph Pecoraro 2016-06-11 01:23:41 PDT
Wow, the bots output is way better then my local output! cq- but I'll strive to get a build that look like the bots.
Comment 32 Build Bot 2016-06-11 01:31:32 PDT
Comment on attachment 281082 [details]
[PATCH] Proposed Fix

Attachment 281082 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1482746

New failing tests:
userscripts/window-onerror-for-isolated-world-1.html
userscripts/window-onerror-for-isolated-world-2.html
userscripts/window-onerror-for-isolated-world-3.html
Comment 33 Build Bot 2016-06-11 01:31:39 PDT
Created attachment 281090 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 34 Darin Adler 2016-06-11 12:27:46 PDT
Comment on attachment 281082 [details]
[PATCH] Proposed Fix

Regressions: Unexpected text-only failures (3)
  userscripts/window-onerror-for-isolated-world-1.html [ Failure ]
  userscripts/window-onerror-for-isolated-world-2.html [ Failure ]
  userscripts/window-onerror-for-isolated-world-3.html [ Failure ]
Comment 35 Joseph Pecoraro 2016-06-13 13:35:36 PDT
(In reply to comment #31)
> Wow, the bots output is way better then my local output! cq- but I'll strive
> to get a build that look like the bots.

Interesting, so these tests fail, and are skipped, on WebKit2. So I'll follow-suit and file a bug on WebKit2.
Comment 36 Alexey Proskuryakov 2016-06-13 13:38:41 PDT
To clarify, EWS failures are WebKit1.
Comment 37 Joseph Pecoraro 2016-06-13 13:46:22 PDT
Created attachment 281195 [details]
[PATCH] Proposed Fix
Comment 38 WebKit Commit Bot 2016-06-13 20:26:35 PDT
Comment on attachment 281195 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 281195

Committed r202023: <http://trac.webkit.org/changeset/202023>
Comment 39 WebKit Commit Bot 2016-06-13 20:26:47 PDT
All reviewed patches have been landed.  Closing bug.