Bug 51040 - Error event in <script> element shouldn't bubble
Summary: Error event in <script> element shouldn't bubble
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks: 8519 58981
  Show dependency treegraph
 
Reported: 2010-12-14 09:38 PST by Yury Semikhatsky
Modified: 2011-04-20 05:45 PDT (History)
12 users (show)

See Also:


Attachments
Patch (4.45 KB, patch)
2010-12-14 09:50 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (4.62 KB, patch)
2011-04-19 09:01 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (16.21 KB, patch)
2011-04-20 02:55 PDT, Yury Semikhatsky
tonyg: review+
tonyg: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (15.48 KB, patch)
2011-04-20 04:36 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-12-14 09:38:11 PST
According to section 4.3.1 of HTML5 spec(http://www.w3.org/TR/html5/scripting-1.html#script):

"If the src attribute's value[of <sctipt> element] is the empty string or if it could not be resolved, then the user agent must queue a task to fire a simple event named error at the element, and abort these steps."

...

"Executing a script block: When the steps above require that the script block be executed, the user agent must act as follows:

If the load resulted in an error (for example a DNS error, or an HTTP 404 error)
Executing the script block must just consist of firing a simple event named error at the element."


"firing a simple event" means that the event is not bubbling(http://www.w3.org/TR/html5/webappapis.html#fire-a-simple-event).
Comment 1 Yury Semikhatsky 2010-12-14 09:42:02 PST
fast/events/onerror-bubbling.html was introduced in http://trac.webkit.org/changeset/14007 and it's not clear why the event was expected to bubble for scripts.
Comment 2 Yury Semikhatsky 2010-12-14 09:50:19 PST
Created attachment 76543 [details]
Patch
Comment 3 Alexey Proskuryakov 2010-12-14 10:55:30 PST
>  it's not clear why the event was expected to bubble for scripts.

ChangeLog says: "The DOM spec says the event should bubble, and that's how it works in Firefox." Is that incorrect? Did you test IE and Firefox?
Comment 4 Yury Semikhatsky 2010-12-14 13:59:15 PST
(In reply to comment #3)
> >  it's not clear why the event was expected to bubble for scripts.
> 
> ChangeLog says: "The DOM spec says the event should bubble, and that's how it works in Firefox." Is that incorrect? 

I think the author is talking about this section http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-eventgroupings-htmlevents which contains the following sentence:

"
error
The error event occurs when an image does not load properly or when an error occurs during script execution. This event is valid for OBJECT elements, BODY elements, and FRAMESET element.
Bubbles: Yes
Cancelable: No
Context Info: None
"
Note that SCRIPT element is not mentioned while HTML5 spec explicitly describes its behavior. 


> Did you test IE and Firefox?
Both old and new version of the test fail in IE9 and FF3.6.12 They don't seem to invoke onerror handler on the script elements.
Comment 5 Alexey Proskuryakov 2010-12-14 14:06:12 PST
Is it possible to make a test that works in IE and Firefox? A regression test that only works in WebKit isn't very future proof.

If no other browser ever invokes onerror on a script element, them maybe HTML5 should be changed to match WebKit as the only implementation.
Comment 6 Yury Semikhatsky 2010-12-14 14:21:25 PST
(In reply to comment #5)
> Is it possible to make a test that works in IE and Firefox? A regression test that only works in WebKit isn't very future proof.
> 
I'm not sure I'm following you here. The test doesn't pass in IE/FF since they don't support script.onerror, in their case we could produce a result that would state that the feature is not implemented in these browsers. Can you elaborate on what you mean by "making a test that works in IE and Firefox"?

> If no other browser ever invokes onerror on a script element, them maybe HTML5 should be changed to match WebKit as the only implementation.
Comment 7 Alexey Proskuryakov 2010-12-14 14:41:25 PST
OK - if they don't support script.onerror at all, then we should check with HTML5 working group about making its behavior match WebKit.
Comment 8 Yury Semikhatsky 2010-12-15 00:09:41 PST
(In reply to comment #7)
> OK - if they don't support script.onerror at all, then we should check with HTML5 working group about making its behavior match WebKit.

One more problem with that is that according to the spec window.onerror handler is a function accepting three arguments(message, filename, line number), "error" event listeners on window will exect ErrorEvent as a parameter while the error event fired for <script> elements is a simple event, i.e. it's an instance of Event and as such doesn't contain additional data such as message, url and line number which would have to be provided to window.onerror should script.onerror bubble.  At the same time script.onerror is supposed to be called on script load errors(for example a DNS error, or an HTTP 404 error)  and there is no additional file name/line number in that case.

window.onerror is supported in IE and Firefox and we hardly can change its specification.
Comment 9 Yury Semikhatsky 2010-12-15 00:19:20 PST
Adding Ian who may know why it's been decided to make <script onerror> a simple event.
Comment 10 Alexey Proskuryakov 2010-12-15 10:59:17 PST
I'm also wondering about SVGScriptElement's error event. It doesn't bubble to document element's onerror in Opera, but does in WebKit. I don't know what SVG spec says, but if we can HTML and SVG <script> have compatible semantics, that would probably be good.
Comment 11 Yury Semikhatsky 2010-12-15 13:03:06 PST
I think the behavior of the error event of SVGScriptElement should be consistent with that of HTMLScriptElement. Description of the SVG script element is quite vague (http://www.w3.org/TR/SVG11/script.html#ScriptElement):"A ‘script’ element is equivalent to the ‘script’ element in HTML"


(In reply to comment #10)
> I'm also wondering about SVGScriptElement's error event. It doesn't bubble to document element's onerror in Opera, but does in WebKit. I don't know what SVG spec says, but if we can HTML and SVG <script> have compatible semantics, that would probably be good.
Comment 12 Tony Gentilcore 2011-04-18 07:33:12 PDT
Could you add a window.onerror = function(msg, file, line) { Fail(); } to the test? As you pointed out, load events bubbling up to window.onerror is the real problem with WebKit's current behavior. Based on behavior of all the other major UAs, I suspect that web devs would be surprised to catch a loading error here as opposed to a scripting error. Nor is it a sane API for the arguments to change. For this reason, I also think the spec is intentional about specifying no bubbling for script loading errors.

Regarding SVGScriptElement, I agree it makes sense to make it match HTMLScriptElement in this regard. In fact, it might be nice if createScriptErrorEvent() were moved to ScriptElement so that it could be shared by SVGScriptElement, HTMLScriptElement and HTMLScriptRunner.

AP, unless you still see issues, I'm happy to review this change.
Comment 13 Yury Semikhatsky 2011-04-19 09:01:07 PDT
Created attachment 90209 [details]
Patch
Comment 14 Yury Semikhatsky 2011-04-19 09:01:41 PDT
(In reply to comment #12)
> Could you add a window.onerror = function(msg, file, line) { Fail(); } to the test? 

Done.
Comment 15 Tony Gentilcore 2011-04-19 09:19:31 PDT
The test looks good. What about the bit about SVGScriptElement and possible refactoring?
Comment 16 Yury Semikhatsky 2011-04-20 02:55:30 PDT
Created attachment 90326 [details]
Patch
Comment 17 Yury Semikhatsky 2011-04-20 02:57:03 PDT
(In reply to comment #15)
> The test looks good. What about the bit about SVGScriptElement and possible refactoring?

Done. ScriptElement::dispatchErrorEvent is not virtual any more and has common implementation for both SVG and HTML. The error event doesn't bubble.
Comment 18 Tony Gentilcore 2011-04-20 03:24:54 PDT
Comment on attachment 90326 [details]
Patch

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

> Source/WebCore/svg/SVGScriptElement.cpp:57
> +    else if (attr->name() == HTMLNames::onerrorAttr)

Adding support for the onerror attribute on SVG scripts should be called out in the ChangeLog.
Comment 19 Yury Semikhatsky 2011-04-20 04:36:47 PDT
Created attachment 90328 [details]
Patch for landing

Fixed ChangeLog entry as Tony suggested.
Comment 20 Yury Semikhatsky 2011-04-20 04:52:48 PDT
Committed r84357: <http://trac.webkit.org/changeset/84357>
Comment 21 WebKit Review Bot 2011-04-20 05:45:46 PDT
http://trac.webkit.org/changeset/84357 might have broken Qt Linux Release
The following tests are not passing:
http/tests/misc/unloadable-script.html