According to the spec (http://www.whatwg.org/specs/web-workers/current-work/#runtime-script-errors): WorkerGlobalScope object's onerror attribute is an EventListener that is supposed to be invoked with ErrorEvent instance. In the current implementation onerror attribute is supposed to be a function that accepts three params(matching fields of ErrorEvent). Once this is fixed we could try and use EventListener::handleEvent for reporting the error and get rid of EventListener::reportError.
Created attachment 51418 [details] patch
Attachment 51418 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/workers/WorkerContext.cpp:39: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 51419 [details] patch Removed commented code.
Attachment 51419 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/workers/WorkerContext.cpp:39: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 51425 [details] patch Fixed style errors.
I have talked with Hixie about WorkerGlobalScope.onerror and he pointed me to the following link as the reference for dealing with non-handled error: http://www.whatwg.org/specs/web-apps/current-work/#concept-error-nothandled The link to the HTML 5 Web App spec says the following: "This section only applies to user agents that support scripting in general and JavaScript in particular. Whenever an uncaught runtime script error occurs in one of the scripts associated with a Document, the user agent must report the error using the onerror event handler attribute of the script's global object. If the error is still not handled after this, then the error should be reported to the user. When the user agent is required to report an error error using the event handler attribute onerror, it must run these steps, after which the error is either handled or not handled: If the value of onerror is a Function The function must be invoked with three arguments. The three arguments passed to the function are all DOMStrings; the first must give the message that the UA is considering reporting, the second must give the absolute URL of the resource in which the error occurred, and the third must give the line number in that resource on which the error occurred. If the function returns false, then the error is handled. Otherwise, the error is not handled. Any uncaught exceptions thrown or errors caused by this function must be reported to the user immediately after the error that the function was called for, without using the report an error algorithm again. Otherwise The error is not handled." Per my understanding, we want to treat onerror handling in WorkerGlobalScope same as in Document. If onerror is set to a function, we need to invoke it with 3 arguments. Would you please ping Hixie to see if this is still true nowadays.
ErrorEvent interface defined in "4.6 Runtime script errors" is used to report the exception back to the worker object.
Comment on attachment 51425 [details] patch Good catch Jian! I didn't realize as well the Workers simply 'borrow' the onerror attribute semantics from 'window', for consistency. The spec could have been more explicit on this. Indeed, it seems the spec still implies the 3-argument function should be a value of the onerror attribute of a global scope object. Yury, could you please clarify this? I'm removing r? from the patch for now..
Created attachment 52016 [details] patch According to Hixie WorkerGlobalScope.onerror should be a 3-argument function for consistency with window.onerror API. I changed the code. Now it provides custom binding for onerror event listener in worker context(probably I should change this bug title or submit this patch in a new bug then).
Attachment 52016 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/bindings/js/JSWorkerContextErrorHandler.cpp:35: Alphabetical sorting problem. [build/include_order] [4] WebCore/bindings/v8/V8WorkerContextErrorHandler.h:42: Code inside a namespace should not be indented. [whitespace/indent] [4] File not a recognized type to check. Skipping: "WebCore/WebCore.pro" WebCore/bindings/js/JSWorkerContextErrorHandler.h:38: Code inside a namespace should not be indented. [whitespace/indent] [4] File not a recognized type to check. Skipping: "WebCore/WebCore.gypi" File not a recognized type to check. Skipping: "WebCore/WebCore.xcodeproj/project.pbxproj" Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 52018 [details] patch Fixed errors found by style checker, added new files to some other build files.
Introducing a new subclass V8WorkerContextErrorHandler that derives from V8WorkerContextEventListener is quite confusing and more complicated. Can we merge the logic in V8WorkerContextErrorHandler::callListenerFunction into V8WorkerContextEventListener::callListenerFunction? We can check if event is ErrrorEvent or not and then dispatch the function with right parameters accordingly. For example, if (event->isErrorEvent()) { ... } else { ... } We need to do the similar thing for JSC bindings. In this way, we do not need to change the code generator scripts.
(In reply to comment #12) > Introducing a new subclass V8WorkerContextErrorHandler that derives from > V8WorkerContextEventListener is quite confusing and more complicated. Can we > merge the logic in V8WorkerContextErrorHandler::callListenerFunction into > V8WorkerContextEventListener::callListenerFunction? We can check if event is > ErrrorEvent or not and then dispatch the function with right parameters > accordingly. For example, > if (event->isErrorEvent()) { > ... > } else { > ... > } > According to http://www.whatwg.org/specs/web-apps/current-work/#event-handler-attributes: Parameter list FormalParameterList If the attribute is the onerror attribute of the Window object Let the function have three arguments, named event, source, and fileno. Otherwise Let the function have a single argument called event. so always treating onerror handler as ternary function would break other onerror handlers(e.g. Worker.onerror) which are regular EventListeners accepting single ErrorEvent argument.
(In reply to comment #13) > (In reply to comment #12) > > Introducing a new subclass V8WorkerContextErrorHandler that derives from > > V8WorkerContextEventListener is quite confusing and more complicated. Can we > > merge the logic in V8WorkerContextErrorHandler::callListenerFunction into > > V8WorkerContextEventListener::callListenerFunction? We can check if event is > > ErrrorEvent or not and then dispatch the function with right parameters > > accordingly. For example, > > if (event->isErrorEvent()) { > > ... > > } else { > > ... > > } > > > According to > http://www.whatwg.org/specs/web-apps/current-work/#event-handler-attributes: > > Parameter list FormalParameterList > If the attribute is the onerror attribute of the Window object > Let the function have three arguments, named event, source, and fileno. > Otherwise > Let the function have a single argument called event. > > so always treating onerror handler as ternary function would break other > onerror handlers(e.g. Worker.onerror) which are regular EventListeners > accepting single ErrorEvent argument. ping
Comment on attachment 52018 [details] patch So I'm a little foggy on exactly why we're adding a custom WorkerContextErrorHandler class, especially since the functionality in this class doesn't seem to be exercised by the tests currently. This patch seems to fix two completely distinct problems: throwing an error in the onerror handler, and some other problem related to parameters passed to an onerror handler. Can you add a test that fails under current WebKit, but which passes with your new error handler support included? I think that would make it clear exactly what case this patch is addressing. My main concern is that I think the intent of the spec is that error reporting from WorkerContext should be identical to error reporting from window context, and yet I don't see any special-case code like this being invoked for DOMWindow errors, so either we're reading the spec incorrectly (or the spec is wrong) or we need to make a similar change to DOMWindow. > +++ b/WebCore/bindings/scripts/CodeGeneratorJS.pm > @@ -1484,7 +1484,12 @@ sub GenerateImplementation > + if ($interfaceName eq "WorkerContext" and $name eq "onerror") { > + $implIncludes{"JSWorkerContextErrorHandler.h"} = 1; > + push(@implContent, " imp->set$implSetterFunctionName(createJSWorkerContextErrorHandler(exec, value, thisObject));\n"); Does this (and the V8 code generator change) do the right thing for SharedWorkers also? You should add a test for this case for SharedWorkers to verify.
Jian explained to me that this patch has morphed to be primarily code cleanup with no functionality change (other than the fix for throwing exceptions from an onerror handler). If so, then shared-worker-script-error.html is probably sufficient for testing this change. I would update the ChangeLog to make it clear that we're just cleaning up the code by moving reportError() out of the general EventListener class since it's only used from Worker context (I guess - I spent a few minutes trying to figure out how window reports its errors but failed) but that no other functionality changes are happening.
(In reply to comment #15) > (From update of attachment 52018 [details]) > So I'm a little foggy on exactly why we're adding a custom > WorkerContextErrorHandler class, especially since the functionality in this > class doesn't seem to be exercised by the tests currently. This patch seems to > fix two completely distinct problems: throwing an error in the onerror handler, > and some other problem related to parameters passed to an onerror handler. > The change to the parameters passing I was going to make at the beginning turned out to contradict to the spec so I rolled it out. > Can you add a test that fails under current WebKit, but which passes with your > new error handler support included? fast/workers/resources/worker-script-error.html should test this. I modified it so that it expects exception in onerror handler to be reported to the Worker object(it's already implemented in Chromium). Tell me if that is not enough. > My main concern is that I think the intent > of the spec is that error reporting from WorkerContext should be identical to > error reporting from window context, and yet I don't see any special-case code > like this being invoked for DOMWindow errors, so either we're reading the spec > incorrectly (or the spec is wrong) or we need to make a similar change to > DOMWindow. > You don't see any specific code for DOMWindow onerror listeners because onerror is not implemented for DOMWindow object in WebKit. Approach with V8WorkerContextErrorHandler can be easily extended to support same binding for DOMWindow once it supports onerror listeners. > > +++ b/WebCore/bindings/scripts/CodeGeneratorJS.pm > > @@ -1484,7 +1484,12 @@ sub GenerateImplementation > > + if ($interfaceName eq "WorkerContext" and $name eq "onerror") { > > + $implIncludes{"JSWorkerContextErrorHandler.h"} = 1; > > + push(@implContent, " imp->set$implSetterFunctionName(createJSWorkerContextErrorHandler(exec, value, thisObject));\n"); > > Does this (and the V8 code generator change) do the right thing for > SharedWorkers also? You should add a test for this case for SharedWorkers to > verify. Yes, it does work for shared workers too. fast/workers/resources/shared-worker-script-error.js should cover this case. (In reply to comment #16) > Jian explained to me that this patch has morphed to be primarily code cleanup > with no functionality change (other than the fix for throwing exceptions from > an onerror handler). That's right. > I would update the ChangeLog to make it clear that we're just cleaning up the > code by moving reportError() out of the general EventListener class since it's > only used from Worker context (I guess - I spent a few minutes trying to figure > out how window reports its errors but failed) but that no other functionality > changes are happening. Ok, I tried to put this information into WebCore/ChangeLog but it must be rephrased in a more clear way. I'll update ChangeLogs.
Created attachment 52529 [details] patch Changed ChangeLog entry.
One small nit: It looks like people are trying to keep the changelog entries < 150 characters wide, so you might want to do the same with your new changelog entry. I don't know if there's an explicit style rule about this, though, so I'm not sure if it's worth changing. I think this change looks good otherwise. It'll be great to have the error reporting working properly.
(In reply to comment #19) > One small nit: It looks like people are trying to keep the changelog entries < > 150 characters wide, so you might want to do the same with your new changelog > entry. I don't know if there's an explicit style rule about this, though, so > I'm not sure if it's worth changing. > > I think this change looks good otherwise. It'll be great to have the error > reporting working properly. So, is it r+?
I don't have the reviewer bit yet, so I can't r+. We'll have to wait for dimich to chime in.
(In reply to comment #21) > I don't have the reviewer bit yet, so I can't r+. We'll have to wait for dimich > to chime in. Oh, that's not a problem, I'll ask pfeldman to look at it.
Comment on attachment 52529 [details] patch Looks sane.
Committing to http://svn.webkit.org/repository/webkit/trunk ... C WebCore/bindings/v8/V8WorkerContextEventListener.h => WebCore/bindings/js/JSWorkerContextErrorHandler.h C WebCore/bindings/v8/V8WorkerContextEventListener.h => WebCore/bindings/v8/V8WorkerContextErrorHandler.cpp C WebCore/bindings/v8/V8WorkerContextEventListener.h => WebCore/bindings/v8/V8WorkerContextErrorHandler.h M LayoutTests/ChangeLog M LayoutTests/fast/workers/worker-script-error-expected.txt M LayoutTests/fast/workers/worker-script-error.html M WebCore/Android.jscbindings.mk M WebCore/ChangeLog M WebCore/GNUmakefile.am M WebCore/WebCore.gypi M WebCore/WebCore.pro M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/bindings/js/JSEventListener.cpp M WebCore/bindings/js/JSEventListener.h A WebCore/bindings/js/JSWorkerContextErrorHandler.cpp M WebCore/bindings/scripts/CodeGeneratorJS.pm M WebCore/bindings/scripts/CodeGeneratorV8.pm M WebCore/bindings/v8/V8WorkerContextEventListener.cpp M WebCore/bindings/v8/V8WorkerContextEventListener.h M WebCore/dom/EventListener.h M WebCore/workers/WorkerContext.cpp M WebCore/workers/WorkerContext.h Committed r57078
Rolling out the change since it broke WebKit Windows build.
http://trac.webkit.org/changeset/57078 might have broken Leopard Intel Release (Tests), Leopard Intel Debug (Tests), and SnowLeopard Intel Release (Tests)
I do not understand why this broke all the builds like this. I suspect that our build system is somehow broken such that earlier changes to HTMLProgressElement did not get re-built until just now?
(In reply to comment #27) > I do not understand why this broke all the builds like this. I suspect that > our build system is somehow broken such that earlier changes to > HTMLProgressElement did not get re-built until just now? That might be the case, I don't see how this change could broke fast/dom/prototype-inheritance-2.html.
Comment on attachment 52529 [details] patch The "break" i'm referring to is the 5 layout tests. The compile failures were definitely caused by this bug Marking r- since this was rolled out.
HTMLProgressElement was added 3 weeks ago: http://trac.webkit.org/changeset/55980/trunk/WebCore/page/DOMWindow.idl Why it's just now showing up in builds after this change I do not understand. CCing Sam Weinig who is a JS bindings expert.
> Why it's just now showing up in builds after this change I do not understand. > CCing Sam Weinig who is a JS bindings expert. The define changed recently. I've update the expectations, but we still need to fix the dependencies.
Filed bug 37102 about the build system failure.
Created attachment 52616 [details] patch Added JSWorkerContextErrorHandler.cpp to JSBindingsAllInOne.cpp to fix WebKit Windows build. Layout test failures detected by build bots turned unrelated to this patch so it's the only modification to the previous patch.
Committing to http://svn.webkit.org/repository/webkit/trunk ... C WebCore/bindings/v8/V8WorkerContextEventListener.h => WebCore/bindings/js/JSWorkerContextErrorHandler.h C WebCore/bindings/v8/V8WorkerContextEventListener.h => WebCore/bindings/v8/V8WorkerContextErrorHandler.cpp C WebCore/bindings/v8/V8WorkerContextEventListener.h => WebCore/bindings/v8/V8WorkerContextErrorHandler.h M LayoutTests/ChangeLog M LayoutTests/fast/workers/worker-script-error-expected.txt M LayoutTests/fast/workers/worker-script-error.html M WebCore/Android.jscbindings.mk M WebCore/ChangeLog M WebCore/GNUmakefile.am M WebCore/WebCore.gypi M WebCore/WebCore.pro M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/bindings/js/JSBindingsAllInOne.cpp M WebCore/bindings/js/JSEventListener.cpp M WebCore/bindings/js/JSEventListener.h A WebCore/bindings/js/JSWorkerContextErrorHandler.cpp M WebCore/bindings/scripts/CodeGeneratorJS.pm M WebCore/bindings/scripts/CodeGeneratorV8.pm M WebCore/bindings/v8/V8WorkerContextEventListener.cpp M WebCore/bindings/v8/V8WorkerContextEventListener.h M WebCore/dom/EventListener.h M WebCore/workers/WorkerContext.cpp M WebCore/workers/WorkerContext.h Committed r57134
Buildfix for --minimal build landed: http://trac.webkit.org/changeset/57141
http://trac.webkit.org/changeset/57141 might have broken SnowLeopard Intel Release (Tests)