Bug 36375 - Worker's onerror handler should accept ErrorEvent as only argument instead of three arguments
Summary: Worker's onerror handler should accept ErrorEvent as only argument instead of...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-19 10:29 PDT by Yury Semikhatsky
Modified: 2010-04-06 04:22 PDT (History)
13 users (show)

See Also:


Attachments
patch (13.51 KB, patch)
2010-03-23 06:51 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch (13.40 KB, patch)
2010-03-23 06:53 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch (13.66 KB, patch)
2010-03-23 07:29 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch (35.32 KB, patch)
2010-03-30 02:43 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch (38.39 KB, patch)
2010-03-30 03:04 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch (38.88 KB, patch)
2010-04-05 06:08 PDT, Yury Semikhatsky
eric: review-
Details | Formatted Diff | Diff
patch (39.49 KB, patch)
2010-04-06 01:40 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2010-03-19 10:29:54 PDT
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.
Comment 1 Yury Semikhatsky 2010-03-23 06:51:15 PDT
Created attachment 51418 [details]
patch
Comment 2 WebKit Review Bot 2010-03-23 06:53:28 PDT
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.
Comment 3 Yury Semikhatsky 2010-03-23 06:53:31 PDT
Created attachment 51419 [details]
patch

Removed commented code.
Comment 4 WebKit Review Bot 2010-03-23 06:58:54 PDT
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.
Comment 5 Yury Semikhatsky 2010-03-23 07:29:21 PDT
Created attachment 51425 [details]
patch

Fixed style errors.
Comment 6 Jian Li 2010-03-23 12:03:44 PDT
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.
Comment 7 Jian Li 2010-03-23 12:08:25 PDT
ErrorEvent interface defined in "4.6 Runtime script errors" is used to report the exception back to the worker object.
Comment 8 Dmitry Titov 2010-03-23 13:57:24 PDT
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..
Comment 9 Yury Semikhatsky 2010-03-30 02:43:27 PDT
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).
Comment 10 WebKit Review Bot 2010-03-30 02:49:36 PDT
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.
Comment 11 Yury Semikhatsky 2010-03-30 03:04:09 PDT
Created attachment 52018 [details]
patch

Fixed errors found by style checker, added new files to some other build files.
Comment 12 Jian Li 2010-03-31 10:39:18 PDT
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.
Comment 13 Yury Semikhatsky 2010-04-01 01:25:58 PDT
(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.
Comment 14 Yury Semikhatsky 2010-04-02 06:09:27 PDT
(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 15 Andrew Wilson 2010-04-02 11:55:30 PDT
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.
Comment 16 Andrew Wilson 2010-04-02 12:16:37 PDT
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.
Comment 17 Yury Semikhatsky 2010-04-02 14:07:54 PDT
(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.
Comment 18 Yury Semikhatsky 2010-04-05 06:08:23 PDT
Created attachment 52529 [details]
patch

Changed ChangeLog entry.
Comment 19 Andrew Wilson 2010-04-05 09:44:37 PDT
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.
Comment 20 Yury Semikhatsky 2010-04-05 09:47:16 PDT
(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+?
Comment 21 Andrew Wilson 2010-04-05 09:50:00 PDT
I don't have the reviewer bit yet, so I can't r+. We'll have to wait for dimich to chime in.
Comment 22 Yury Semikhatsky 2010-04-05 09:51:23 PDT
(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 23 Pavel Feldman 2010-04-05 09:56:00 PDT
Comment on attachment 52529 [details]
patch

Looks sane.
Comment 24 Yury Semikhatsky 2010-04-05 10:00:32 PDT
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
Comment 25 Yury Semikhatsky 2010-04-05 10:19:21 PDT
Rolling out the change since it broke WebKit Windows build.
Comment 26 WebKit Review Bot 2010-04-05 10:43:14 PDT
http://trac.webkit.org/changeset/57078 might have broken Leopard Intel Release (Tests), Leopard Intel Debug (Tests), and SnowLeopard Intel Release (Tests)
Comment 27 Eric Seidel (no email) 2010-04-05 10:55:15 PDT
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?
Comment 28 Yury Semikhatsky 2010-04-05 11:05:51 PDT
(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 29 Eric Seidel (no email) 2010-04-05 11:10:45 PDT
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.
Comment 30 Eric Seidel (no email) 2010-04-05 11:16:18 PDT
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.
Comment 31 Adam Barth 2010-04-05 11:31:24 PDT
> 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.
Comment 32 Eric Seidel (no email) 2010-04-05 11:47:58 PDT
Filed bug 37102 about the build system failure.
Comment 33 Yury Semikhatsky 2010-04-06 01:40:48 PDT
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.
Comment 34 Yury Semikhatsky 2010-04-06 02:20:10 PDT
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
Comment 35 Csaba Osztrogonác 2010-04-06 03:49:08 PDT
Buildfix for --minimal build landed: http://trac.webkit.org/changeset/57141
Comment 36 WebKit Review Bot 2010-04-06 04:22:55 PDT
http://trac.webkit.org/changeset/57141 might have broken SnowLeopard Intel Release (Tests)