Bug 36375 – Worker's onerror handler should accept ErrorEvent as only argument instead of three arguments

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 51418 [details]
patch

[WebKit Review Bot]

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.

[Yury Semikhatsky]

Created attachment 51419 [details]
patch

Removed commented code.

[WebKit Review Bot]

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.

[Yury Semikhatsky]

Created attachment 51425 [details]
patch

Fixed style errors.

[Jian Li]

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.

[Jian Li]

ErrorEvent interface defined in "4.6 Runtime script errors" is used to report the exception back to the worker object.

[Dmitry Titov]

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..

[Yury Semikhatsky]

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).

[WebKit Review Bot]

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.

[Yury Semikhatsky]

Created attachment 52018 [details]
patch

Fixed errors found by style checker, added new files to some other build files.

[Jian Li]

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.

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

(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

[Andrew Wilson]

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.

[Andrew Wilson]

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.

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

Created attachment 52529 [details]
patch

Changed ChangeLog entry.

[Andrew Wilson]

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.

[Yury Semikhatsky]

(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+?

[Andrew Wilson]

I don't have the reviewer bit yet, so I can't r+. We'll have to wait for dimich to chime in.

[Yury Semikhatsky]

(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.

[Pavel Feldman]

Comment on attachment 52529 [details]
patch

Looks sane.

[Yury Semikhatsky]

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

[Yury Semikhatsky]

Rolling out the change since it broke WebKit Windows build.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57078 might have broken Leopard Intel Release (Tests), Leopard Intel Debug (Tests), and SnowLeopard Intel Release (Tests)

[Eric Seidel (no email)]

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?

[Yury Semikhatsky]

(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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Adam Barth]

> 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.

[Eric Seidel (no email)]

Filed bug 37102 about the build system failure.

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

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

[Csaba Osztrogonác]

Buildfix for --minimal build landed: http://trac.webkit.org/changeset/57141

[WebKit Review Bot]

http://trac.webkit.org/changeset/57141 might have broken SnowLeopard Intel Release (Tests)