Bug 51040 – Error event in <script> element shouldn't bubble

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yury Semikhatsky]

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

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 76543 [details]
Patch

[Alexey Proskuryakov]

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

[Yury Semikhatsky]

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

[Alexey Proskuryakov]

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.

[Yury Semikhatsky]

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

[Alexey Proskuryakov]

OK - if they don't support script.onerror at all, then we should check with HTML5 working group about making its behavior match WebKit.

[Yury Semikhatsky]

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

[Yury Semikhatsky]

Adding Ian who may know why it's been decided to make <script onerror> a simple event.

[Alexey Proskuryakov]

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.

[Yury Semikhatsky]

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.

[Tony Gentilcore]

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.

[Yury Semikhatsky]

Created attachment 90209 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #12)
> Could you add a window.onerror = function(msg, file, line) { Fail(); } to the test? 

Done.

[Tony Gentilcore]

The test looks good. What about the bit about SVGScriptElement and possible refactoring?

[Yury Semikhatsky]

Created attachment 90326 [details]
Patch

[Yury Semikhatsky]

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

[Tony Gentilcore]

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.

[Yury Semikhatsky]

Created attachment 90328 [details]
Patch for landing

Fixed ChangeLog entry as Tony suggested.

[Yury Semikhatsky]

Committed r84357: <http://trac.webkit.org/changeset/84357>

[WebKit Review Bot]

http://trac.webkit.org/changeset/84357 might have broken Qt Linux Release
The following tests are not passing:
http/tests/misc/unloadable-script.html