Bug 8519 – WebCore doesn't fire window.onerror event when uncaught JavaScript exceptions are thrown

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jon]

window.onerror registers an event listener, but WebCore never actually fires the onerror event.


Onerror references: 

http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onerror.asp

http://developer.mozilla.org/en/docs/DOM:window.onerror

http://www.jaws.umn.edu/javascript_1.1/evnt8.htm

[Jon]

Created attachment 7877 [details]
Test case based on ggaren's test for 8511

[Jesse Costello-Good]

If someone takes the effort to implement onerror, it would be great if it improves upon the usefulness of the current implementations in Internet Explorer and Firefox. In my opinion, the true value of window.onerror is to approximate this common Java idiom that provides invaluable error diagnostics to programmers:

public static void main(String[] args) {
  try {
    ...
  } catch (Throwable t) {
    t.printStackTrace();
    System.exit(1);
  }
}

Because JavaScript in a browser is so event-driven, it is tedious to impossible to wrap every entry point in a JavaScript try/catch block. It would be nice if window.onerror could act as the catch block in the main method in the Java code sample above. For this to be the case, the call stack of the thrown exception must be available in the onerror method. Ideally, the actual thrown exception object should also be available. 

Internet Explorer currently provides the call stack of the line where the exception was raised by introspecting arguments/caller/callee. The exception object not available. (The standard arguments to onerror are message, file, and line number). Unfortunately, if an object is explicitly thrown with the "throw" statement, the message argument to onerror is always "Exception thrown and not caught". And of course, the file argument is always the HTML page hosting the JavaScript, not the actually JavaScript file. 

Firefox has its own weaknesses. The three arguments to onerror are typically useful. The message is relevant even if the error was thrown with the throw statement. The line number and file URL even pinpoint the JavaScript line that threw the exception even if that line is in an external JavaScript function. Unfortunately, the uncaught exception object is not available in the onerror function. Worse, there is no way to get the call stack of the exception, even through arguments/caller/callee. Since Firefox already supports the Error.stack property, it would make more sense for them to just expose the uncaught exception object.

My suggestion for Safari (and Firefox) is to either add a 4th argument to the onerror function that is the uncaught exception object, or to set a property of the window object to the last uncaught exception. 

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=355430

[Alexey Proskuryakov]

*** Bug 13645 has been marked as a duplicate of this bug. ***

[Garrett Smith]

comment #2. 

window.onerror sucks. JScript thinks it's an Event. Poor API design copied during browser wars. 

Should we copy this legacy approach?

Event onerror callback signature should be single argument: an object. 

This could be either of type: either Error or ErrorEvent.

Example:

errorCaught = function errorCaught( ev ) {
  var stack = ev.stack;
  log( stack );
  ev.preventDefault();
};

window.addEventListener( "error", errorCaught, false );

[Adele Peterson]

<rdar://problem/3175102> please implement onError

[Adele Peterson]

looks like we have some support for this event (see LayoutTests/fast/dom/onerror-img.html).

From looking at the code, we dispatch this event in these situations:
1) In the tokenizer (both HTML and XML versions), if an error occurred in a cached script. (HTMLTokenizer.cpp, XMLTokenizer.cpp)
2) If an error occurred loading an image (HTMLImageLoader.cpp)
3) If an error occurs parsing a script (HTMLScriptElement.cpp)

So there's probably some other places where we should be firing the event.  And there also might be some investigation needed about when the event bubbles.

[Kay Summers]

I have yet to see a single example of a parse error being sent to the window object's error handler. Everything goes to the console no matter what. 

[Sam Weinig]

*** Bug 17758 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

I've received two (unsolicited) emails in the last 24 hours asking if WebKit supported this.  One of them was for catching uncaught JS exceptions (and I guess is used by an internal testing tool here @ Google).  A second was wanting to catch XML parse errors when rendering an SVG to an image (using the Obj-C API).  It seems to me that there are real needs out there for this feature.  Certainly the XML error, I can think of no other way to detect such.

[David Kilzer (:ddkilzer)]

Adding GoogleBug keyword per Comment #9.

[Garrett Smith]

The test for verifying deferError depends on onerror working.
http://dhtmlkitchen.com/ape/test/tests/APE-test.html

The code that the test is testing against, APE.deferError, actually works. There doesn't seem to be any other way (other than onerror) to determine that the code works, and so test fails (falsely).

[Darin Adler]

This bug really needs more details and possibly to be broken up into multiple bugs.

The original complaint was that we don't support the error event at all. But it's clear that we do have the event and some code to dispatch it in some cases. We can fix the attached single test case, but it's likely that single fix won't cause us to dispatch error events in all the various cases people want us to.

[Aaron Boodman]

On Wed, Apr 16, 2008 at 7:39 AM, Darin Adler <[e-mail]> wrote:
> That onerror bug report is way too broad. The original claim is that the
> error event isn't supported at all by WebKit, and that's not true. It would
> be helpful to know exactly which types of errors you would find this useful
> for. It might be a straightforward fix. I think your comments in the bug
> might make that clear, but I wasn't completely sure when reading it.

Sorry, we weren't aware that it worked at all because we were interested in catching runtime javascript errors, which it is not fired for.

Ideally, window.onerror would be called for any uncaught javascript error, and for any error inside asynchronous native code (eg databases).

Our immediate goal is to implement a unit test system where any error (even those fired asynchronously) is caught and reported as a test failure. But window.onerror is also frequently used to implement logging for deployed applications. For example, Gmail uses this API to catch all errors and report them back to the server.

And FWIW, I agree with Garrett Smith's comments. It would be great if WebKit would take the lead here and implement a really useful global error handler with lots of rich details about what went wrong. If you did not want to break compatibility with existing users of window.onerror, you could either add a new API (window.addEventListener("error"), or window.onexception), or just add new params to the existing signature.

[Darrell Esau]

Created attachment 20597 [details]
6 other test cases which throw JS errors that should be caught with window.onerror

(In reply to comment #12)
> This bug really needs more details ...
> 
> ... We can fix the attached single test case, but it's likely that single
> fix won't cause us to dispatch error events in all the various cases people
> want us to.

I think fixing the attached case will go a long way ...

If you need more test cases, I've attached 6 other test cases in this zip file.

[Eric Seidel (no email)]

I'm willing to implement this.  My primary concern is test cases (which the 6 posted here do help some with).  I'm also concerned as to if we should really match this API or if we should come up with something better as window.onexception or similar.  I'm currently leaning towards that we should implement a IE/FF compatible window.onerror and then think more about *also* implementing a newer nicer thing, but that HTML5 should be involved in specing some nicer thing.

[Eric Seidel (no email)]

One question would be if this should be implemented in JavaScriptCore as an "unhandledException" callback (which would have to be wired into the right Interpreter::execute() calls, or if this should be a WebCore-only thing, which could be wired into ScriptController::evaluate() pretty easily.

I think I'll start with ScriptController::evaluate() and we can wire up a JSC-based solution later if that's preferred.

[Ian 'Hixie' Hickson]

What's wrong with what HTML5 specs? (I tried to make it match the other browsers; doesn't it?)

[Erik Arvidsson]

Ian, the proposed spec says that onerror is called with 3 DOMStrings. The third argument (the line number) should be a number.

[Eric Seidel (no email)]

(In reply to comment #17)
> What's wrong with what HTML5 specs? (I tried to make it match the other
> browsers; doesn't it?)

Link to the HTML5 spec:
http://www.whatwg.org/specs/web-apps/current-work/#runtime-script-errors

[Eric Seidel (no email)]

so window.onerror is not actually an event listener.

It's a callback function.  It does not follow the same signature (or dispatch rules) as normal EventListener objects.  This is sad, and broken.

I wonder how important it is that we match IE/FF's (broken) syntax here.  How much of a compat issue would it be if we add window.onerror but that we make it fire a real event?   Like the Worker error event:
http://www.whatwg.org/specs/web-workers/current-work/#fire-a-worker-error-event

[Eric Seidel (no email)]

(In reply to comment #20)

I will also note that the other place in the DOM where we have an onerror handler is XHR.  But that is explicitly left out of the W3C draft:
http://www.w3.org/TR/XMLHttpRequest/#notcovered

Oh yeah, and geolocation... which has it's own, lamely different onerror:
http://dev.w3.org/geo/api/spec-source.html#error-callback

[Eric Seidel (no email)]

(In reply to comment #21)

Actually ApplicationCache has its own onerror too:
http://www.whatwg.org/specs/web-apps/current-work/#handler-appcache-onerror

Although that looks like an event again, although there is no definition yet as to what an ErrorEvent is.

[Erik Arvidsson]

(In reply to comment #20)
> I wonder how important it is that we match IE/FF's (broken) syntax here.  How
> much of a compat issue would it be if we add window.onerror but that we make it
> fire a real event?   Like the Worker error event:
> http://www.whatwg.org/specs/web-workers/current-work/#fire-a-worker-error-event
> 

Yeah, window.onerror is not an event listener. Lame? Yes, but I think we are better of following IE and FF here and then later later come up with a better solution.

[Eric Seidel (no email)]

I'm going to work on implementing the historical compatible behavior.

window.onerror will take a callback function instead of a event listener.

[Eric Seidel (no email)]

Actually, looking at this more this looks pretty much useless.  Yes, the functionality of catching all exceptions is useful, but this callback isn't.  It doesn't even give you the actual thrown exception.

I don't think this is worth doing.  It can't be a compat concern because the feature is too weak for sites to actually depend on.  I think it is worth adding a real error event however.  The worker ErrorEvent does not expose an exception so that's not a fit.

A non-event way would be to implement an onerror callback which includes the Exception.  Passing the exception as the first argument would keep "compatible" arguments[0].toString() behavior even if the type would be different.  I could also pass the exception as the 4th argument and maintain compatibility.

I still prefer an EventListener as that fits better in our system (and is something we can define via IDL).  But a callback is also possible (and might actually be useful if it exposed the actual exception object -- so that eventually someone might be able to get a stack from it).

[Sam Weinig]

*** Bug 26052 has been marked as a duplicate of this bug. ***

[Greg Hazel]

Since FF and IE offer window.onerror, why not support it as well? Websites do use it, even if it is not as useful as a real exception, to find out if the page encountered an error. Currently WebKit offers no notification, which is certainly worse than file:line.

If you add a new mechanism that provides an exception that would be great, but websites would have to implement support for it. Adding window.onerror in addition would mean sites get the benefit of it right away.

[Erik Kay]

When you say "it can't be a compat concern", you're only looking at it from one angle of compat.  I agree that it's unlikely that websites will break if this callback doesn't exist.  However, the existence of the feature allows websites to detect and fix other webkit compat issues more quickly.  For example, the gmail case sited previously means that any unhandled exceptions can be logged to a server where they can be detected, quantified, analyzed, etc.  Without this, it's quite possible that the site owners won't realize that their site is broken in webkit browsers as quickly as other browsers (or perhaps at all if the effect of the error is subtle).

Incidentally, I ran into this myself trying to build some unit tests for chrome extensions and not wanting to have to fill the code with zillions of try/catch blocks.  The net effect for me is a lot more effort and a longer debug cycle (since unhandled exceptions wind up leading to timeouts in the test rather than hard errors).

[Chris Goddard]

Although I understand the desire to not support

[Garrett Smith]

On Sun, Aug 16, 2009 at 2:15 PM, <[e-mail]> wrote:
> https://bugs.webkit.org/show_bug.cgi?id=8519
>
>
> Erik Kay <[e-mail]> changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                 CC|                            |[e-mail]
>
>
>
>
> --- Comment #28 from Erik Kay <[e-mail]>  2009-08-16 14:15:45 PDT ---
> When you say "it can't be a compat concern", you're only looking at it from one
> angle of compat.  I agree that it's unlikely that websites will break if this
> callback doesn't exist.  However, the existence of the feature allows websites
> to detect and fix other webkit compat issues more quickly.  For example, the
> gmail 

GMail is not such a good case because the code is not valid syntax for an ECMAScript program.

case sited previously means that any unhandled exceptions can be logged
> to a server where they can be detected, quantified, analyzed, etc.  Without
> this, it's quite possible that the site owners won't realize that their site is
> broken in webkit browsers as quickly as other browsers (or perhaps at all if
> the effect of the error is subtle).
>
> Incidentally, I ran into this myself trying to build some unit tests for chrome
> extensions and not wanting to have to fill the code with zillions of try/catch
> blocks.  The net effect for me is a lot more effort and a longer debug cycle
> (since unhandled exceptions wind up leading to timeouts in the test rather than
> hard errors).
>
Wrapping everything in a try/catch is something that beginners will tend to do.

If the unhandled exception was thrown from a setTimeout, catch it using an asynchronous testing approach. 

testXXX : function() {
   function myCallback() {
     fired = true;
   }
   someObj.sendMessage(myCallback);
 
   currentTest.wait(function(){ 
    Assert.isTrue(fired, "callback didn't fire");
   }, 20);
}

And you can see global the try-catch in GMail. Comment 13 mentions using |window.onerror| instead:

Here we can see what looks like a function declaration inside of a block; 

try{function e(b){throw b;}var h=true,j=null,k=false,aa=encodeURIComponent,aaa=JS_OBFUSCATED,ba=Object,l=Error,ca=parseInt,da=parseFloat,baa=Function,ea=GLOBALS,fa=decodeURIComponent,ga=

It would be reasonable to expect a SyntaxError from such syntactically invalid code, except for the fact that that is not what browsers do, and that falls into the realm of syntax extensions and error handling and this depends on the implementation.

A window.onerror event callback mechanism would be useful for a testing framework. I can see how window.onerror seems attractive for reasons mentioned in comment #13, however such motivation is both driven by and serves to encourage poor programming practices (such as those used in GMail) and should not be used in production (as GMail).

So, if a window.onerror is desirable, the reason for it is not given in comment 13.

[Benoit Marchant]

The bottom line is that you can't put try/catch everywhere, and especially (and unfortunately) considering the performance hit it represent. What I'm interested in building is a way to trap unexpected errors and send it automatically with details, user agent, stack trace etc to a backend application in order to learn about bugs users will never report and help have an idea of their frequency.

I don't see another way than a top-level error handler to do that, and unfortunately, there's not a realistic way to put all one's code and framework within one global function that would wrap every lines of code in one try/catch.

[kangax]

(In reply to comment #31)
> The bottom line is that you can't put try/catch everywhere, and especially (and
> unfortunately) considering the performance hit it represent. What I'm
> interested in building is a way to trap unexpected errors and send it
> automatically with details, user agent, stack trace etc to a backend
> application in order to learn about bugs users will never report and help have
> an idea of their frequency.

[...]

I second that, and actually is exactly what I'm doing in supporting clients. Trapping all unexpected errors at the top level does in fact reveal some of the unexpected glitches in application.

[Greg Hazel]

(In reply to comment #32)
> (In reply to comment #31)
> > The bottom line is that you can't put try/catch everywhere, and especially (and
> > unfortunately) considering the performance hit it represent. What I'm
> > interested in building is a way to trap unexpected errors and send it
> > automatically with details, user agent, stack trace etc to a backend
> > application in order to learn about bugs users will never report and help have
> > an idea of their frequency.
> 
> [...]
> 
> I second that, and actually is exactly what I'm doing in supporting clients.
> Trapping all unexpected errors at the top level does in fact reveal some of the
> unexpected glitches in application.

Agreed. Another good example of this is that window.onerror is called if there is a syntax error on the page, which try/catch can not catch.

[Garrett Smith]

> Agreed. Another good example of this is that window.onerror is called if there
> is a syntax error on the page, which try/catch can not catch.

Which errors? 

Whatever errors you have, they should have been prevented before the release and this comes back to proper testing.

[kangax]

(In reply to comment #34)
> > Agreed. Another good example of this is that window.onerror is called if there
> > is a syntax error on the page, which try/catch can not catch.
> 
> Which errors? 

Errors that I could not anticipate.

> 
> Whatever errors you have, they should have been prevented before the release

Isn't that like saying that whatever bugs in the system you have they should have been prevented before the release? Unfortunately, that's not how things work :)

> and this comes back to proper testing.
   
What is proper testing? I do have a test suite which I always keep "green". Yet, there are things I simply can't test or can't predict upfront. There could be errors due to unexpected user input. I do try to tame such input, but it still creeps in and being able to catch such errors later rather than never seems like a good idea to me.

Then there are unknown environments that I simply can't physically test. I don't have a Playstation 3 so I don't know how NetFront engine will react to my javascript (even though I try to design defensively). I can't test Opera 9.25 since (for whatever reason) it doesn't start on my Mac OS X. It is only after seeing these errors logged through `window.onerror` that I can "fix" them (and change test suite appropriately).

[Greg Hazel]

There are many cases where testing on major browsers cannot catch all errors in all browsers and all networks. However, window.onerror covers these cases quite well. Here are a few examples:

http://www.google.com/support/forum/p/Google%20Analytics/thread?tid=7e7a84f8b3ad1439&hl=en

http://www.google.com/support/forum/p/friendconnect/thread?tid=37300940d8ec540b&hl=en

http://www.google.com/support/forum/p/Chrome/thread?tid=7e9f87870a37e401&hl=en

http://www.seomoz.org/blog/google-analytics-javascript-is-broken

I wonder what the issue with implementing this is.

It's been up for discussion since 2006 and it's clearly a requested feature.

To me it doesn't matter if it is implemented in the same way as it's in firefox or IE, or if we choose a new way. 

The important thing is that there is a way to catch these errors.

What needs to be done to go forward with this feature?

[Jake Archibald]

I'd also like to see this implemented. When creating async tests in something like qunit we have to set a timeout in case the test errors. If window.onerror was implemented, qunit would be able to catch the error, report it, and proceed to the next test without a ~10 second timeout.

[Tak]

crosslinking to Google Chrome bug tracker entry for this same issue

http://code.google.com/p/chromium/issues/detail?id=7771

[Brandon Thomson]

Until all popular browsers have exactly the same javascript behavior in every release (haha, yeah right), we really do need a mechanism of reporting errors back to the server or we'll never know about them. The path of least resistance is to implement this legacy handler and worry about improving it later

[Walter Poch]

Guys, when this bug will be finally implemented. We need it for global error catching!

[Thomas Steinacher]

Would love to see this implemented. This is required to detect failed JSONP requests by setting an onerror callback on the script element. Right now the only way to detect them is by having a timeout.

[Alexey Proskuryakov]

> This is required to detect failed JSONP
> requests by setting an onerror callback on the script element.

If HTMLScriptElement.onerror doesn't work, please file a new bug. This one is about window.onerror.

[Charles Kendrick]

This is hugely important to Ajax frameworks.  In IE, any time an error occurs, we are able to walk the stack and log something like this:

11:37:59.734:RDQ4:WARN:Log:Error:
   'Object doesn't support this property or method'
   in http://localhost:8080/MyStaffTrack.html
   at line 821
   [a]ImgButton.$31r(_1=>13128, _2=>Obj{name:HOURS_CLOCKED}, _3=>[ListGrid ID:isc_OID_94], _4=>Obj, _5=>0, _6=>5)
   ListGrid.$315(_1=>13128, _2=>Obj, _3=>Obj{name:HOURS_CLOCKED}, _4=>0, _5=>5)
   ListGrid.getCellValue(_1=>Obj, _2=>0, _3=>5, _4=>[GridBody ID:isc_OID_94_body])
   [o]GridBody.getCellValue(record=>Obj, rowNum=>0, colNum=>5, gridBody=>[GridBody ID:isc_OID_94_body])
   GridRenderer.$22k(_1=>Obj, _2=>0, _3=>5)
   GridRenderer.getTableHTML(_1=>undef, _2=>undef, _3=>undef)
   GridRenderer.getInnerHTML(undef)
   Class.invokeSuper(_1=>null, _2=>"getInnerHTML", _3=>undef, _4=>undef, _5=>undef, _6=>undef, _7=>undef, _8=>undef, _9=>undef, _10=>undef)
   Class.Super(_1=>"getInnerHTML", _2=>Obj{length:1}, _3=>undef)
   GridBody.getInnerHTML(undef)

This has major benefits:

1. it appears in the flow of other logs, so we know exactly when the error occurred relative to other happenings

2. we are adding all kinds of SmartClient/SmartGWT-specific information that a general, Firebug-like error console can never replicate.  For example, it knows that the anonymous function "getCellValue" is attached to a class ListGrid but this could never be figured out by a general tool that is just looking at JavaScript code and doesn't know about the SmartClient class system.  We are even showing information like the prefix [o] - this means the method is an override of the default method for the class.  Note also the thread indicator (RDQ4) - this indicates it happens in the redraw queue thread - very important to know.

This is so valuable for troubleshooting that right in the SmartGWT FAQ and in all our support documents we say that whenever you encounter a JavaScript error the first step is to leave Safari/Chrome/whatever nice fast browser you actually like to use, and go replicate the error in Internet Explorer so we can get good diagnostic information.. and that's just sad.

We don't care about compatibility with IE's window.onerror vs an event listener or some other approach - just please give us a way to get the information.  Thanks.

[Thomas Steinacher]

Opened Bug 38428 for an issue concerning script onerror.

[Brian Harper]

4 years later and still no window.onerror. I'll add yet another note that this feature is hugely valuable to web application developers who rely on it to be notified of client errors. Like others we have implemented a logging system to collect information on errors that our users encounter, and have found it invaluable. No, window.onerror as it is implemented in FF and IE is not the best, or "right" solution. But it's infinitely better than no solution. Until window.onerror is implemented in Webkit, we can't recommend Chrome and Safari to our users, even though we support them and would otherwise prefer them. 

It makes little sense to me to plow ahead with a raft of new HTML5 features while this basic piece of infrastructure is missing. There's no way I'm going to implement new functionality that is only supported in Webkit if I can't get error reporting short of littering my code with try/catch blocks.

It's been a year since Eric's comment that it wasn't valuable, and that has been refuted by many developers who absolutely do get a lot of value from it. And it's been 4 years since this deficiency was reported. Please, somebody tackle this!

[Christopher Blum]

Totally agree with Brian.

We need this in order to create a cross browser exception notifier that sends us any javascript error that occured on our production system.

Thanks.

[Tak]

me too.

We use some libraries that attempt to compensate for this behavior, but we can't always use them.  It's a continued source of pain that we need to sniff for window.onerror and provide one if it doesn't exist.

[Justin Meyer]

onError is super crazy useful for big development efforts.  We even released an open source application(https://damnit.jupiterit.com/) and script that emails us when onerror happens in a page.

When you've got 100s of thousands of custom JS code in an application, you can't test for everything.

[Anders Mad.]

It's THE (only) thing missing!.. I'm joining the angry mob here - hoping that our pitchfork comments will make a difference :-)

[Alexey Proskuryakov]

The "me too" comments can only make this less likely to be implemented, because useful technical information is lost between them.

There is more than enough interest expressed in this feature, please refrain from commenting unless you're actually implementing it, or are answering to a specific request for feedback (like that in comment 12).

[Yury Semikhatsky]

Created attachment 65092 [details]
Patch

[WebKit Review Bot]

Attachment 65092 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
WebCore/bindings/v8/V8ConsoleMessage.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yury Semikhatsky]

(In reply to comment #52)
> Created an attachment (id=65092) [details]
> Patch

Not directly related to this patch but rater to the Element.onXXX handlers in general. One DOMWindow native instance may be shared between several DOM wrapper worlds. Each of such worlds may set its own function to window.onerror and since all of them will use DOMWindow.onerror as storage, the last assignment will overwrite all previous. This means that we need to store window.onerror handler on corresponding DOMWindow wrapper to allow window.onerror handler per wrapper world. Alternatively we may continue with current implementation and have at most one window.onerror handler no matter how many worlds share same DOMWindow. But in any case we shouldn't invoke the handler if it originates from a world other than that where the exception happened.

[Yury Semikhatsky]

Created attachment 65104 [details]
Patch

[Yury Semikhatsky]

Created attachment 65106 [details]
Patch

[Pavel Feldman]

Comment on attachment 65106 [details]
Patch

Looks sane to me. I am willing 

WebCore/dom/Document.cpp:408
 +      , m_reportingException(false)
This field is not used.


WebCore/dom/Document.h:1287
 +      bool m_reportingException;
ditto

[Yury Semikhatsky]

Created attachment 65109 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #57)
> (From update of attachment 65106 [details])
> Looks sane to me. I am willing 
> 
> WebCore/dom/Document.cpp:408
>  +      , m_reportingException(false)
> This field is not used.
> 
Done.

> 
> WebCore/dom/Document.h:1287
>  +      bool m_reportingException;
> ditto
Done.

[Sam Weinig]

Comment on attachment 65109 [details]
Patch

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

> LayoutTests/fast/events/window-onerror2.html:19
> +window.onerror = function(msg, url, line)
> +{
> +    url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> +    log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> +}
This would read more clearly if it returned false, instead of relying on the implicit return value of undefined being interpreted as false.

> LayoutTests/fast/events/window-onerror2.html:30
> +function delayedThrowException()
> +{
> +    throw "Exception in setTimeout";
> +}
> +setTimeout(delayedThrowException, 0);
> +
> +setTimeout(function() {
> +    if (window.layoutTestController)
> +        layoutTestController.notifyDone();
> +}, 0);
These racing setTimeouts seem unnecessarily complex/unreliable. Why not just have onerror keep a count, and notifyDone when it hits 3.

> LayoutTests/fast/events/window-onerror3.html:22
> +
> +function log(msg) {
> +    document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +function test1()
> +{
> +    window.onerror = function (error, url, line) {
> +        url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> +        log("Error caught successfully: " + error + "\nFile: " + url + "\nLine: " + line)
> +    };
> +    unknownObject.unknownProperty++;
> +}
> +</script>
> +<body onload="test1();">
> +<p>You should see a message if window.onerror is working properly for this test.<a href="https://bugs.webkit.org/show_bug.cgi?id=8519">Bug 8519</a>.</p>
> +<hr>
> +<div id='console'></div>
> +</body>
It seems like there should be no console message here since the onerror handler is returning false (meaning handled). Is this a bug?  You also should explicitly return false.

> LayoutTests/fast/events/window-onerror4.html:22
> +
> +function log(msg) {
> +    document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +function test1()
> +{
> +    window.onerror = function (error, url, line) {
> +        url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> +        log("Error caught successfully: " + error + "\nFile: " + url + "\nLine: " + line)
> +    };
> +    eval("1=2");
> +}
> +</script>
> +<body onload="test1();">
> +<p>You should see a log record if window.onerror is working properly for this test.<a href="https://bugs.webkit.org/show_bug.cgi?id=8519">Bug 8519</a>.</p>
> +<hr>
> +<div id='console'></div>
> +</body>
Same question as above?

> LayoutTests/fast/events/window-onerror5.html:17
> +function log(msg) {
> +    document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +function test1()
> +{
> +    window.onerror = function (error, url, line) {
> +        url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> +        log("Error caught successfully: " + error + "\nFile: " + url + "\nLine: " + line)
> +    };
> +    eval("a(");
> +}
> +</script>
Here too.

> LayoutTests/fast/events/window-onerror6.html:23
> +    document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +window.onerror = function(msg, url, line)
> +{
> +    url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> +    log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> +}
> +</script>
> +<script>
> +
> +a) // syntax error
> +
> +</script>
Same question.

> LayoutTests/fast/events/window-onerror7.html:24
> +
> +function log(msg) {
> +    document.getElementById("console").innerHTML += msg + "<br>";
> +}
> +
> +window.onerror = function(msg, url, line)
> +{
> +    url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> +    log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> +    throw "Nested error";
> +}
> +
> +throw 2010;
> +
> +</script>
> +</body>
> +</html>
According to HTML5:

"Any uncaught exceptions thrown or errors caused by this function may be reported to the user immediately after the error that the function was called for; the report an error algorithm must not be used to handle exceptions thrown or errors caused by this function."

It seems like the nested error is being reported first which seems incorrect.

> WebCore/dom/Document.cpp:4450
> +        // Invoke window.onerror.
> +        if (invokeOnErrorHandler(domWindow->onerror(), errorMessage, lineNumber, sourceURL))
> +            return;
> +    }
You should remove the comment. The code is clear enough not to need one.

> WebCore/dom/ScriptExecutionContext.cpp:275
> +bool ScriptExecutionContext::invokeOnErrorHandler(EventListener* onerror, const String& errorMessage, int lineNumber, const String& sourceURL)
> +{
> +    if (!onerror)
> +        return false;
> +    if (m_inErrorHandler)
> +        return false;
> +    m_inErrorHandler = true;
> +    RefPtr<ErrorEvent> errorEvent(ErrorEvent::create(errorMessage, sourceURL, lineNumber));
> +    onerror->handleEvent(this, errorEvent.get());
> +    m_inErrorHandler = false;
> +    return errorEvent->defaultPrevented();
> +}
There are a few problems with this code. 
1) It doesn't dispatch the event to listeners added via addEventListener()
2) It doesn't set the properties on the event like "target", "phase" and "currentTarget". Even though the event object is not available as an argument, it will be both in scope and available on the global object.

There is an easy solution though, the standard way to due this is to call dispatchEvent on the EventTarget.  Please make this change and add tests for each of these bugs.

> WebCore/dom/ScriptExecutionContext.h:98
> +        bool invokeOnErrorHandler(EventListener*, const String& errorMessage, int lineNumber, const String& sourceURL);
Error handler is too generic a name. How about we call this dispatchErrorEvent.

> WebCore/dom/ScriptExecutionContext.h:188
> +        bool m_inErrorHandler;
This should probably be renamed to m_inDispatchErrorEvent. Thats what we do for other recursion guards in WebCore.


r- due to the correctness problems but this is a very promising patch.

[Greg Hazel]

Created attachment 67141 [details]
Another test case with a syntax error which should be caught with window.onerror 

Maybe this is functionally already covered well by the eval tests, but I wanted to show exactly how it should be possible to use it before onload, and regarding regular script tags.

[Yury Semikhatsky]

(In reply to comment #60)
> (From update of attachment 65109 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=65109&action=prettypatch
> 
> > LayoutTests/fast/events/window-onerror2.html:19
> > +window.onerror = function(msg, url, line)
> > +{
> > +    url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> > +    log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> > +}
> This would read more clearly if it returned false, instead of relying on the implicit return value of undefined being interpreted as false.

Done.


> 
> > LayoutTests/fast/events/window-onerror2.html:30
> > +function delayedThrowException()
> > +{
> > +    throw "Exception in setTimeout";
> > +}
> > +setTimeout(delayedThrowException, 0);
> > +
> > +setTimeout(function() {
> > +    if (window.layoutTestController)
> > +        layoutTestController.notifyDone();
> > +}, 0);
> These racing setTimeouts seem unnecessarily complex/unreliable. Why not just have onerror keep a count, and notifyDone when it hits 3.
> 
Fixed. Thanks. I forgot that test will just time out if notifyDone is not called.


> > LayoutTests/fast/events/window-onerror3.html:22
> > +
> > +function log(msg) {
> > +    document.getElementById("console").innerHTML += msg + "<br>";
> > +}
> > +
> > +function test1()
> > +{
> > +    window.onerror = function (error, url, line) {
> > +        url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> > +        log("Error caught successfully: " + error + "\nFile: " + url + "\nLine: " + line)
> > +    };
> > +    unknownObject.unknownProperty++;
> > +}
> > +</script>
> > +<body onload="test1();">
> > +<p>You should see a message if window.onerror is working properly for this test.<a href="https://bugs.webkit.org/show_bug.cgi?id=8519">Bug 8519</a>.</p>
> > +<hr>
> > +<div id='console'></div>
> > +</body>
> It seems like there should be no console message here since the onerror handler is returning false (meaning handled). Is this a bug?  You also should explicitly return false.
> 

I've added explicit return false statements to the other tests as well. From the following sentence in HTML5 spec 
"If the function returns false, then the error is handled. Otherwise, the error is not handled."
I conclude that returning undefined value is supposed to be treated the same way as returning _true_. Actually that's how the code currently works: adding explicit "return false"s cleared the console messages. In my opinion although treating undefined as true complies with the spec, it is somewhat unusual in JavaScript world.



> > LayoutTests/fast/events/window-onerror4.html:22
> > +
> > +function log(msg) {
> > +    document.getElementById("console").innerHTML += msg + "<br>";
> > +}
> > +
> > +function test1()
> > +{
> > +    window.onerror = function (error, url, line) {
> > +        url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> > +        log("Error caught successfully: " + error + "\nFile: " + url + "\nLine: " + line)
> > +    };
> > +    eval("1=2");
> > +}
> > +</script>
> > +<body onload="test1();">
> > +<p>You should see a log record if window.onerror is working properly for this test.<a href="https://bugs.webkit.org/show_bug.cgi?id=8519">Bug 8519</a>.</p>
> > +<hr>
> > +<div id='console'></div>
> > +</body>
> Same question as above?
> 
Fixed. See my reply above.

> > LayoutTests/fast/events/window-onerror5.html:17
> > +function log(msg) {
> > +    document.getElementById("console").innerHTML += msg + "<br>";
> > +}
> > +
> > +function test1()
> > +{
> > +    window.onerror = function (error, url, line) {
> > +        url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> > +        log("Error caught successfully: " + error + "\nFile: " + url + "\nLine: " + line)
> > +    };
> > +    eval("a(");
> > +}
> > +</script>
> Here too.
> 
Ditto.

> > LayoutTests/fast/events/window-onerror6.html:23
> > +    document.getElementById("console").innerHTML += msg + "<br>";
> > +}
> > +
> > +window.onerror = function(msg, url, line)
> > +{
> > +    url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> > +    log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> > +}
> > +</script>
> > +<script>
> > +
> > +a) // syntax error
> > +
> > +</script>
> Same question.
> 
Ditto.

> > LayoutTests/fast/events/window-onerror7.html:24
> > +
> > +function log(msg) {
> > +    document.getElementById("console").innerHTML += msg + "<br>";
> > +}
> > +
> > +window.onerror = function(msg, url, line)
> > +{
> > +    url = url ? url.match( /[^\/]+\/?$/ )[0] : url;
> > +    log("Main frame window.onerror: " + msg + " at " + url + ":" + line);
> > +    throw "Nested error";
> > +}
> > +
> > +throw 2010;
> > +
> > +</script>
> > +</body>
> > +</html>
> According to HTML5:
> 
> "Any uncaught exceptions thrown or errors caused by this function may be reported to the user immediately after the error that the function was called for; the report an error algorithm must not be used to handle exceptions thrown or errors caused by this function."
> 
> It seems like the nested error is being reported first which seems incorrect.
> 
Fixed. All nested exceptions are collected and dispatched only after the original error is handled.


> > WebCore/dom/Document.cpp:4450
> > +        // Invoke window.onerror.
> > +        if (invokeOnErrorHandler(domWindow->onerror(), errorMessage, lineNumber, sourceURL))
> > +            return;
> > +    }
> You should remove the comment. The code is clear enough not to need one.
> 
> > WebCore/dom/ScriptExecutionContext.cpp:275
> > +bool ScriptExecutionContext::invokeOnErrorHandler(EventListener* onerror, const String& errorMessage, int lineNumber, const String& sourceURL)
> > +{
> > +    if (!onerror)
> > +        return false;
> > +    if (m_inErrorHandler)
> > +        return false;
> > +    m_inErrorHandler = true;
> > +    RefPtr<ErrorEvent> errorEvent(ErrorEvent::create(errorMessage, sourceURL, lineNumber));
> > +    onerror->handleEvent(this, errorEvent.get());
> > +    m_inErrorHandler = false;
> > +    return errorEvent->defaultPrevented();
> > +}
> There are a few problems with this code. 
> 1) It doesn't dispatch the event to listeners added via addEventListener()
> 2) It doesn't set the properties on the event like "target", "phase" and "currentTarget". Even though the event object is not available as an argument, it will be both in scope and available on the global object.
> 
> There is an easy solution though, the standard way to due this is to call dispatchEvent on the EventTarget.  Please make this change and add tests for each of these bugs.
>
Fixed both and provided tests. I don't see support for "error" event listeners in other browsers. Firefox passes an event object with type "error" but there are no message, filename, lineno fields on it. IE doesn't allow add such listener on the window object.

I'm not quite sure how to interpret section "6.1.6.5 Runtime script errors" in the HTML5 spec. It doesn't mention "error" listeners added via addEventListener, only onerror handlers. Does it mean that the listeners behavior is unspecified or should we treat "error" event here just like the other DOM events?


> > WebCore/dom/ScriptExecutionContext.h:98
> > +        bool invokeOnErrorHandler(EventListener*, const String& errorMessage, int lineNumber, const String& sourceURL);
> Error handler is too generic a name. How about we call this dispatchErrorEvent.
> 

Done. Renamed the method to dispatchErrorEvent.


> > WebCore/dom/ScriptExecutionContext.h:188
> > +        bool m_inErrorHandler;
> This should probably be renamed to m_inDispatchErrorEvent. Thats what we do for other recursion guards in WebCore.
> 
Done.


Additionally I added check for dom wrapper world into error handlers. If the exception occurred in a world other than the handler, the latter won't be invoked.

[Yury Semikhatsky]

Created attachment 69208 [details]
Patch, next iteration

[Darin Adler]

Comment on attachment 69208 [details]
Patch, next iteration

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

> WebCore/GNUmakefile.am:792
> +<<<<<<< HEAD

You left a merge marker in. Please fix that!

> WebCore/GNUmakefile.am:801
> +=======
> +>>>>>>> fix

Here’s the other half.

> WebCore/bindings/js/JSErrorHandler.cpp:86
> +        Event* savedEvent = globalObject->currentEvent();
> +        globalObject->setCurrentEvent(event);

Why is this implementing event dispatch? Why can’t this use the DOMWindow::dispatchEvent function?

[Darin Adler]

(In reply to comment #64)
> > WebCore/bindings/js/JSErrorHandler.cpp:86
> > +        Event* savedEvent = globalObject->currentEvent();
> > +        globalObject->setCurrentEvent(event);
> 
> Why is this implementing event dispatch? Why can’t this use the DOMWindow::dispatchEvent function?

I see, you’re thinking that 6.1.6.5 in HTML5 means only the function should be called, and other event dispatching rules do not apply.

[Yury Semikhatsky]

Created attachment 69462 [details]
Patch

[Yury Semikhatsky]

Rebased the patch, added some description to the changelogs.

(In reply to comment #64)
> (From update of attachment 69208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69208&action=review
> 
> > WebCore/GNUmakefile.am:792
> > +<<<<<<< HEAD
> 
> You left a merge marker in. Please fix that!
> 
Fixed.


> > WebCore/GNUmakefile.am:801
> > +=======
> > +>>>>>>> fix
> 
> Here’s the other half.
> 
Fixed.


(In reply to comment #65)
> (In reply to comment #64)
> > > WebCore/bindings/js/JSErrorHandler.cpp:86
> > > +        Event* savedEvent = globalObject->currentEvent();
> > > +        globalObject->setCurrentEvent(event);
> > 
> > Why is this implementing event dispatch? Why can’t this use the DOMWindow::dispatchEvent function?
> 
> I see, you’re thinking that 6.1.6.5 in HTML5 means only the function should be called, and other event dispatching rules do not apply.

Exactly. Do you think I misinterpreted that?

[WebKit Review Bot]

Attachment 69462 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
WebCore/dom/ErrorEvent.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 79 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 69462 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4243017

[WebKit Review Bot]

Attachment 69462 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4218040

[Yury Semikhatsky]

Created attachment 69474 [details]
Patch

[WebKit Review Bot]

Attachment 69474 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: ''
WebCore/dom/ErrorEvent.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 79 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 69474 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4173039

[Adam Barth]

Comment on attachment 69474 [details]
Patch

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

This patch looks a bit sloppy.  Some of that might be just style errors.  The isolated world stuff is subtle, and it feels like you're re-inventing the wheel.  Part of the problem is that V8 doesn't really use DOMWrapperWorld, which is something I should fix.  I'm not sure what the current state of that is.  I'd like to see a lot more testing around isolated worlds and onerror, since that's the toughest part.  It looks like you have one test, but you really should test all the cases, including inline event handlers, setTimeout, eval, etc.

> WebCore/bindings/js/JSErrorHandler.cpp:62
> +    // It is unsecure to report exception from one world to a handler from another.

unsecure => insecure

> WebCore/bindings/v8/DOMWrapperWorld.cpp:56
> +DOMWrapperWorld* enteredContextWorld()

I don't understand this function.  Why is this different than V8IsolatedContext::getEntered()->world() ?

> WebCore/bindings/v8/DOMWrapperWorld.cpp:59
> +    v8::Handle<v8::Context> enteredCotnext = v8::Context::GetEntered();

enteredCotnext => enteredContext

> WebCore/bindings/v8/DOMWrapperWorld.cpp:76
> +{
> +    ASSERT(scriptExecutionContext->isDocument());
> +    if (V8Proxy* proxy = V8Proxy::retrieve(scriptExecutionContext)) {
> +        v8::HandleScope handleScope;
> +        v8::Local<v8::Context> context = contextHandle.adjustedContext(proxy);
> +        v8::Context::Scope contextScope(context);
> +        return enteredContextWorld();

I don't really understand this code.  Are you truing to get the DOMWrapperWorld for scriptExecutionContext + contextHandle ?  This seems like a very round-about (and not necessarily correct) way of doing it.

> WebCore/bindings/v8/V8WindowErrorHandler.cpp:50
> +    // It is unsecure to report exception from one world to a handler from another.

unsecure => insecure

> WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:77
> -        context->reportException(errorMessage, lineNumber, sourceURL);
> +        context->reportException(0, errorMessage, lineNumber, sourceURL, 0);

What do these zeros mean?

> WebCore/dom/ScriptExecutionContext.cpp:273
> +    if (m_pendingExceptions) {

Prefer early return.

> WebCore/dom/ScriptExecutionContext.cpp:294
> +    RefPtr<ErrorEvent> errorEvent(ErrorEvent::create(wrapperWorld, errorMessage, sourceURL, lineNumber));

We usually use the = form of the constructor.

> WebCore/dom/ScriptExecutionContext.h:193
> +        struct PendingException : public Noncopyable {

class?

> WebCore/workers/WorkerContext.cpp:261
> +}

Missing a blank line after this }

[Yury Semikhatsky]

Created attachment 69610 [details]
Patch

[Early Warning System Bot]

Attachment 69610 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4185063

[Yury Semikhatsky]

(In reply to comment #74)
> (From update of attachment 69474 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69474&action=review
> 
> This patch looks a bit sloppy.  Some of that might be just style errors.  The isolated world stuff is subtle, and it feels like you're re-inventing the wheel.  Part of the problem is that V8 doesn't really use DOMWrapperWorld, which is something I should fix.  I'm not sure what the current state of that is.  I'd like to see a lot more testing around isolated worlds and onerror, since that's the toughest part.  It looks like you have one test, but you really should test all the cases, including inline event handlers, setTimeout, eval, etc.
> 
The code seems to work incorrectly for "error" event listeners. The ErrorEvent will be propagated to all listeners no matter in which isolated world they were added. That is a real problem and the reason why in my very initial patch I used a custom code to dispatch ErrorEvent to the global onerror handler without notification to "error" event listeners. I think we may want to stick with that approach(it would match other browsers behavior which as I mentioned above don't provide any meaningful info to the "error" event listeners), alternatively we could implement custom dispatching for error event that will compare listener's world with event's one before calling the listener. Before writing additional tests for the cross-world error reporting we need to agree how this should work in WebKit. What do you think, Adam?


> > WebCore/bindings/js/JSErrorHandler.cpp:62
> > +    // It is unsecure to report exception from one world to a handler from another.
> 
> unsecure => insecure
> 
Fixed.


> > WebCore/bindings/v8/DOMWrapperWorld.cpp:56
> > +DOMWrapperWorld* enteredContextWorld()
> 
> I don't understand this function.  Why is this different than V8IsolatedContext::getEntered()->world() ?
> 
Because V8IsolatedContext::getEntered() assumes that last entered context is an isolated world's context. As far as I understand main world and isolated world contexts have different sets of internal fields and it would be wrong to treat entered context as isolated world one if it was from main world.


> > WebCore/bindings/v8/DOMWrapperWorld.cpp:59
> > +    v8::Handle<v8::Context> enteredCotnext = v8::Context::GetEntered();
> 
> enteredCotnext => enteredContext
> 
Fixed.


> > WebCore/bindings/v8/DOMWrapperWorld.cpp:76
> > +{
> > +    ASSERT(scriptExecutionContext->isDocument());
> > +    if (V8Proxy* proxy = V8Proxy::retrieve(scriptExecutionContext)) {
> > +        v8::HandleScope handleScope;
> > +        v8::Local<v8::Context> context = contextHandle.adjustedContext(proxy);
> > +        v8::Context::Scope contextScope(context);
> > +        return enteredContextWorld();
> 
> I don't really understand this code.  Are you truing to get the DOMWrapperWorld for scriptExecutionContext + contextHandle ?  This seems like a very round-about (and not necessarily correct) way of doing it.
> 
Yes, the function tries to get DOMWrapperWorld for a V8 context which in turn can be retrieved by the ScriptExecutionContext and WorldContextHandle. We need this if we want to have common reportException implementation for both engines.  I don't understand why we use ScriptExecutionContext+WorldContextHandle to get world context instead of relying on ScriptExecutionContext+DOMWrapperWorld. This is the main reason why I need this function at all.


> > WebCore/bindings/v8/V8WindowErrorHandler.cpp:50
> > +    // It is unsecure to report exception from one world to a handler from another.
> 
> unsecure => insecure
> 
Fixed.


> > WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:77
> > -        context->reportException(errorMessage, lineNumber, sourceURL);
> > +        context->reportException(0, errorMessage, lineNumber, sourceURL, 0);
> 
> What do these zeros mean?
> 
The first parameter is a DOM wrapper world. There are no isolated worlds in the worker context though. Last parameter is ScriptCallStack. Stack traces are not supported for uncaught worker exceptions yet.


> > WebCore/dom/ScriptExecutionContext.cpp:273
> > +    if (m_pendingExceptions) {
> 
> Prefer early return.
> 
Done.


> > WebCore/dom/ScriptExecutionContext.cpp:294
> > +    RefPtr<ErrorEvent> errorEvent(ErrorEvent::create(wrapperWorld, errorMessage, sourceURL, lineNumber));
> 
> We usually use the = form of the constructor.
> 
Done. Should we state this in the style guide?


> > WebCore/dom/ScriptExecutionContext.h:193
> > +        struct PendingException : public Noncopyable {
> 
> class?
> 
Ditto. I didn't find any recommendations on using struct vs. class in WebKit code and decided to use struct for objects that are plain data containers.


> > WebCore/workers/WorkerContext.cpp:261
> > +}
> 
> Missing a blank line after this }
Fixed.

[Adam Barth]

I'm confused.  Generally, we fire all events in all worlds, similar to how DOM nodes exist in all worlds.  The important part is that each world get a world-specific JS wrapper for the underlying event object.

[Chris Griego]

In browsers today, window.onerror is treated as a callback, not an event listener. It doesn't behave anything like, say, window.onload.

[Adam Barth]

(In reply to comment #79)
> In browsers today, window.onerror is treated as a callback, not an event listener. It doesn't behave anything like, say, window.onload.

I see.  That's not confusing at all.  :)

[Yury Semikhatsky]

(In reply to comment #78)
> I'm confused.  Generally, we fire all events in all worlds, similar to how DOM nodes exist in all worlds.  The important part is that each world get a world-specific JS wrapper for the underlying event object.

ErrorEvent is different from other DOM events as it always contains information(which otherwise cannot be obtained from the other worlds) about JavaScript context where the exception happened and ideally global onerror handler should be a property on a particular global JS object, rather than a property on DOMWindow which is shared by different isolated worlds. Simply having world-specific JS wrapper for the onerror handler is not enough in the case of ErrorEvent as it doesn't prevent leaks of exception details between worlds: you may have exception in an isolated world and window.onerror set in the main one, the handler will still be world-specific but it will learn about exception in a different world, looks like a vulnerability. Do you suggest we should dispatch ErrorEvents to all registered listeners/handlers no matter which world they originate from just for consistency with other events?

[Yury Semikhatsky]

(In reply to comment #80)
> (In reply to comment #79)
> > In browsers today, window.onerror is treated as a callback, not an event listener. It doesn't behave anything like, say, window.onload.
> 
> I see.  That's not confusing at all.  :)

The error handling is confusing, that's true. But nowadays it's at least described in HTML5 spec(even the fact that window.onerror accepts 3 separate arguments instead of a single ErrorEvent) and I think we should provide at least the same functionality that exists in other browsers. People are used to the error handling mechanism and I believe it's better to support it in WebKit than don't provide any comparable solution.

[Adam Barth]

Comment on attachment 69610 [details]
Patch

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

> WebCore/bindings/v8/DOMWrapperWorld.cpp:72
> +    if (V8Proxy* proxy = V8Proxy::retrieve(scriptExecutionContext)) {

Prefer early exit.

> WebCore/bindings/v8/DOMWrapperWorld.cpp:76
> +        return enteredContextWorld();

I'm not sure this is right.  There should be a more direct way to get to the DOMWrapperWorld from the v8 context.  The v8 context should have an internal field that maps to a V8IsolatedWorld and that isolated world should now what its DOMWrapperWorld is.  This whole business of writing to static state so that we can later read from it doesn't feel very robust.

> WebCore/dom/ScriptExecutionContext.cpp:287
> +    if (!wrapperWorld && !isWorkerContext())
> +        return false;

Should this be an ASSERT?  How calls this with a null world in a non-worker context?

[Yury Semikhatsky]

Created attachment 71157 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #84)
> Created an attachment (id=71157) [details]
> Patch

Removed code matching isolated worlds. Now each ErrorEvent will be dispatch to all "error" event listeners no matter in which world they were added. Added a couple more tests for that. Please take a look.

[Early Warning System Bot]

Attachment 71157 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4449103

[Csaba Osztrogonác]

(In reply to comment #86)
> Attachment 71157 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/4449103

You missed to add new files to WebCore.pro:
WebCore/bindings/js/JSErrorHandler.cpp
WebCore/bindings/js/JSErrorHandler.h

You missed to add new files to WebCore.pro v8 sections:
WebCore/bindings/v8/V8WindowErrorHandler.cpp
WebCore/bindings/v8/V8WindowErrorHandler.h

[Yury Semikhatsky]

Created attachment 71164 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #88)
> Created an attachment (id=71164) [details]
> Patch

Added new files to WebCore.pro. Thanks  Csaba!

[Aaron Boodman]

(In reply to comment #81)
> (In reply to comment #78)
> > I'm confused.  Generally, we fire all events in all worlds, similar to how DOM nodes exist in all worlds.  The important part is that each world get a world-specific JS wrapper for the underlying event object.
> 
> ErrorEvent is different from other DOM events as it always contains information(which otherwise cannot be obtained from the other worlds) about JavaScript context where the exception happened and ideally global onerror handler should be a property on a particular global JS object, rather than a property on DOMWindow which is shared by different isolated worlds. Simply having world-specific JS wrapper for the onerror handler is not enough in the case of ErrorEvent as it doesn't prevent leaks of exception details between worlds: you may have exception in an isolated world and window.onerror set in the main one, the handler will still be world-specific but it will learn about exception in a different world, looks like a vulnerability. Do you suggest we should dispatch ErrorEvents to all registered listeners/handlers no matter which world they originate from just for consistency with other events?

I agree with the sentiment here. window.onerror is different from other event handlers (or callbacks or whatever) in that it is really (should really be) a property of the global object, not of DOMWindow.

There should be separate instances of onerror for each world, and we should only fire onerror in the context the error occurred in. It doesn't make sense to me to spam someone else's script with errors happening in your script, and it could lead to unintentional bridging of worlds.

[Yuxiang Luo]

I recommend adding a fourth parameter 'stack'. For chrome/V8, besides error description, file url, and line number, an Error object also contains stack info. So, it will be great and more consistent if window.onerror also accepts stack info as an additional parameter, and this won't break existing code that uses window.onerror.

An actual requirement is from one of our chrome compatibility audit tools, which listens to onerror event, and analyze error information when any occurs. The analysis requires error stack info (and also column info besides line number).

[Yury Semikhatsky]

Created attachment 76202 [details]
Patch

[Yury Semikhatsky]

I'd love to have window.onerror per isolated world, it would make the things clearer, but there is also Error DOM event which should be dispatched to all DOM listeners. One option would be to leave it unimplemented and dispatch the error only to the onerror handlers. If we decided to support DOM listeners as well we would need to extend DOM Error message with some information about the isolated world where the error happened so that the listeners could filter out errors that happened in other contexts. To implement the latter approach we would have to expose the concept of isolated worlds to the page.


(In reply to comment #90)
> (In reply to comment #81)
> > (In reply to comment #78)
> > > I'm confused.  Generally, we fire all events in all worlds, similar to how DOM nodes exist in all worlds.  The important part is that each world get a world-specific JS wrapper for the underlying event object.
> > 
> > ErrorEvent is different from other DOM events as it always contains information(which otherwise cannot be obtained from the other worlds) about JavaScript context where the exception happened and ideally global onerror handler should be a property on a particular global JS object, rather than a property on DOMWindow which is shared by different isolated worlds. Simply having world-specific JS wrapper for the onerror handler is not enough in the case of ErrorEvent as it doesn't prevent leaks of exception details between worlds: you may have exception in an isolated world and window.onerror set in the main one, the handler will still be world-specific but it will learn about exception in a different world, looks like a vulnerability. Do you suggest we should dispatch ErrorEvents to all registered listeners/handlers no matter which world they originate from just for consistency with other events?
> 
> I agree with the sentiment here. window.onerror is different from other event handlers (or callbacks or whatever) in that it is really (should really be) a property of the global object, not of DOMWindow.
> 
> There should be separate instances of onerror for each world, and we should only fire onerror in the context the error occurred in. It doesn't make sense to me to spam someone else's script with errors happening in your script, and it could lead to unintentional bridging of worlds.

[WebKit Review Bot]

Attachment 76202 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6983038

[Yury Semikhatsky]

Created attachment 76207 [details]
Patch

[Early Warning System Bot]

Attachment 76202 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6953042

[Early Warning System Bot]

Attachment 76207 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6923042

[Csaba Osztrogonác]

(In reply to comment #97)
> Attachment 76207 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/6923042

You missed adding JSErrorHandler.cpp to WebCore.pro.

[Yury Semikhatsky]

Created attachment 76222 [details]
Patch

[Yury Semikhatsky]

The latest version fixes that.

(In reply to comment #98)
> (In reply to comment #97)
> > Attachment 76207 [details] [details] did not build on qt:
> > Build output: http://queues.webkit.org/results/6923042
> 
> You missed adding JSErrorHandler.cpp to WebCore.pro.

[Adam Barth]

Comment on attachment 76222 [details]
Patch

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

This patch is very subtle, but I think it looks good.  Sorry this took a while to review.  A couple minor nits below that would be nice to polish up before landing.

> LayoutTests/ChangeLog:1
> [phone]  [e-mail]  <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>

Woah, this email address is crazy.

> WebCore/ChangeLog:28
> +        Tests: fast/events/window-onerror1.html
> +               fast/events/window-onerror10.html
> +               fast/events/window-onerror11.html
> +               fast/events/window-onerror2.html
> +               fast/events/window-onerror3.html
> +               fast/events/window-onerror4.html
> +               fast/events/window-onerror5.html
> +               fast/events/window-onerror6.html
> +               fast/events/window-onerror7.html
> +               fast/events/window-onerror8.html
> +               fast/events/window-onerror9.html
> +               http/tests/security/window-onerror-exception-in-iframe.html
> +               userscripts/window-onerror-for-isolated-world-1.html
> +               userscripts/window-onerror-for-isolated-world-2.html

This is an impressive set of tests.

> WebCore/bindings/js/JSErrorHandler.cpp:79
> +        ref();

Manual ref counting is sadness.  Please use RefPtr<...> protector(this).

> WebCore/dom/ScriptExecutionContext.cpp:272
> +            m_pendingExceptions = new Vector<OwnPtr<PendingException> >();
> +        m_pendingExceptions->append(new PendingException(errorMessage, lineNumber, sourceURL, callStack));

These are naked calls to new.  Can we either use adoptPtr or adoptRef?

> WebCore/dom/ScriptExecutionContext.cpp:283
> +    for (size_t i = 0; i < m_pendingExceptions->size(); i++) {

What happens if m_pendingExceptions changes during this loop?  Does m_inDispatchErrorEvent stop that?

> WebKit/chromium/src/WebWorkerClientImpl.cpp:388
>          thisPtr->m_scriptExecutionContext->reportException(errorMessage,
>                                                             lineNumber,
> -                                                           sourceURL);
> +                                                           sourceURL,
> +                                                           0);

You can just make this all one line.

[Yury Semikhatsky]

(In reply to comment #101)
> (From update of attachment 76222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76222&action=review
> 
> This patch is very subtle, but I think it looks good.  Sorry this took a while to review.  A couple minor nits below that would be nice to polish up before landing.
> 
> > LayoutTests/ChangeLog:1
> > [phone]  [e-mail]  <[e-mail]@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
> 
> Woah, this email address is crazy.
> 
Fixed.

> > WebCore/ChangeLog:28
> > +        Tests: fast/events/window-onerror1.html
> > +               fast/events/window-onerror10.html
> > +               fast/events/window-onerror11.html
> > +               fast/events/window-onerror2.html
> > +               fast/events/window-onerror3.html
> > +               fast/events/window-onerror4.html
> > +               fast/events/window-onerror5.html
> > +               fast/events/window-onerror6.html
> > +               fast/events/window-onerror7.html
> > +               fast/events/window-onerror8.html
> > +               fast/events/window-onerror9.html
> > +               http/tests/security/window-onerror-exception-in-iframe.html
> > +               userscripts/window-onerror-for-isolated-world-1.html
> > +               userscripts/window-onerror-for-isolated-world-2.html
> 
> This is an impressive set of tests.
> 
> > WebCore/bindings/js/JSErrorHandler.cpp:79
> > +        ref();
> 
> Manual ref counting is sadness.  Please use RefPtr<...> protector(this).
> 
Fixed.

> > WebCore/dom/ScriptExecutionContext.cpp:272
> > +            m_pendingExceptions = new Vector<OwnPtr<PendingException> >();
> > +        m_pendingExceptions->append(new PendingException(errorMessage, lineNumber, sourceURL, callStack));
> 
> These are naked calls to new.  Can we either use adoptPtr or adoptRef?
> 
Done. Added adoptPtr().

> > WebCore/dom/ScriptExecutionContext.cpp:283
> > +    for (size_t i = 0; i < m_pendingExceptions->size(); i++) {
> 
> What happens if m_pendingExceptions changes during this loop?  Does m_inDispatchErrorEvent stop that?
> 
m_pendingExceptions can change only during dispatchErrorEvent call if there are exceptions in the error listeners/handler.


> > WebKit/chromium/src/WebWorkerClientImpl.cpp:388
> >          thisPtr->m_scriptExecutionContext->reportException(errorMessage,
> >                                                             lineNumber,
> > -                                                           sourceURL);
> > +                                                           sourceURL,
> > +                                                           0);
> 
> You can just make this all one line.
Done.

[Yury Semikhatsky]

Committed r73914: <http://trac.webkit.org/changeset/73914>

[WebKit Review Bot]

http://trac.webkit.org/changeset/73914 might have broken Qt Linux Release minimal, Qt Windows 32-bit Release, and Qt Windows 32-bit Debug

[Csaba Osztrogonác]

Comment on attachment 76222 [details]
Patch

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

> WebCore/dom/ScriptExecutionContext.cpp:88
>  ScriptExecutionContext::ScriptExecutionContext()
>  #if ENABLE(DATABASE)
>      : m_hasOpenDatabases(false)
>  #endif
> +    , m_inDispatchErrorEvent(false)

This modification is incorrect if ENABLE(DATABASE) 
is false, and it  broke Qt minimal and Windows build.

[Yury Semikhatsky]

Fixed in http://trac.webkit.org/changeset/73920

(In reply to comment #105)
> (From update of attachment 76222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76222&action=review
> 
> > WebCore/dom/ScriptExecutionContext.cpp:88
> >  ScriptExecutionContext::ScriptExecutionContext()
> >  #if ENABLE(DATABASE)
> >      : m_hasOpenDatabases(false)
> >  #endif
> > +    , m_inDispatchErrorEvent(false)
> 
> This modification is incorrect if ENABLE(DATABASE) 
> is false, and it  broke Qt minimal and Windows build.

[Yury Semikhatsky]

Reopening since the change was rolled out in r73939 due to Chromium page cycler failures.

[Eric Seidel (no email)]

Comment on attachment 76222 [details]
Patch

r- since this was rolled out.

[Yury Semikhatsky]

Created attachment 77495 [details]
Patch

This change differs from the previous one in how window.onerror behaves in case of "error" event dispatched manually by user. If the event has type "error" but is not an instance od ErrorEvent the handler will be treated as a regular handler and will be called with the event object as a single argument. This matches Firefox behavior. See fast/events/window-onerror12.html for more details.

[Yury Semikhatsky]

Created attachment 77497 [details]
Patch

Fixed ChangeLog entries.

[WebKit Review Bot]

Attachment 77495 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7306207

[WebKit Review Bot]

Attachment 77497 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7227221

[Yury Semikhatsky]

Created attachment 77504 [details]
Patch

Fixed Chromium compilation.

[Pavel Feldman]

Comment on attachment 77504 [details]
Patch

Rubber stamping the patch, new bubbling guard looks sane.

[Yury Semikhatsky]

Committed r76216

[Darrell Esau]

Huge THANK YOU to Yury!

Wow - what a battle.  After 5 years -- I've just confirmed that this is working as expected in Chrome 10 beta.

I owe you a beer.

[Charles Kendrick]

This is good stuff guys, but unfortunately WebKit is still well behind Internet Explorer here - you can't get a stack trace in onerror.

If you try new Error().stack, the stack ends at onerror.

If you try "stack walking" approaches like arguments.callee.caller or arguments.caller, you likewise cannot get past onerror.

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 need to continue to recommend that users launch Internet Explorer to get definitive error reporting, which is a shame.

[Darin Adler]

(In reply to comment #117)
> If you try new Error().stack, the stack ends at on error.

Would you be willing to file a bug about this with a test case?

> If you try "stack walking" approaches like arguments.callee.caller or arguments.caller, you likewise cannot get past on error.

Would you be willing to file a bug about this with a test case?

[Charles Kendrick]

Yes, I created a new bug with test code:

   https://bugs.webkit.org/show_bug.cgi?id=55092

Just also wanted to make this problem visible to the CC list here since a lot of effort went into onerror but it's not very useful as it stands.

Note there's a Mozilla bug for this too, I added the test case there as well:

   https://bugzilla.mozilla.org/show_bug.cgi?id=355430

[Timothy Quinn]

(In reply to comment #119)
I appreciate your effort in trying to get this refined. I too agree that the current solution of missing the stack makes this barely useful for inline debugging within a standalone browser.

IE's solution of allowing the call stack crawling is an interesting solution but I would agree that its a bit sloppy. The main reason to do call stack crawling is to get the stack.

I would suggest a hybrid solution similar to what was proposes in Comment #91 From Yuxiang Luo above and add a fourth parameter. The preferred fourth parametr is to pass the actual error object and thus the user can retrieve the error.stack just like in Mozilla.

For WebKit folks, I am new to this community. What is the best way to get this going? Is a new issue the best path?

Thanks! 
- JSD

[Charles Kendrick]

(In reply to comment #120)
> IE's solution of allowing the call stack crawling is an interesting solution but I would agree that its a bit sloppy. The main reason to do call stack crawling is to get the stack.
> 
> I would suggest a hybrid solution similar to what was proposes in Comment #91 From Yuxiang Luo above and add a fourth parameter. The preferred fourth parametr is to pass the actual error object and thus the user can retrieve the error.stack just like in Mozilla.

Just a reminder that the browser providing a stack as a String is vastly, vastly worse than programmatic access to the stack (see my comment #44).

It is so valuable to have direct access to the stack that Isomorphic has actually started on a Firefox extension to provide it, even though we already have mechanisms in place to get the stack as a String for almost any crash.

There's no need to replicate all the details of IE's stack walking APIs.  It can be replaced with a cleaner API (which is what our Firefox extension does).

[Alexey Proskuryakov]

Adding stack trace to the error object is tracked as bug 55092. Let's discuss that idea there.