The default bug view has changed. See this FAQ.

Error event fired on <script> load failure is not per spec/other browsers

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Events
--
minor
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

({dev-doc-complete})

Trunk
mozilla14
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Test cases:

data:text/html,<!doctype html>
no error
<script>onerror = function(e) {
document.documentElement.textContent =
"error"
}</script>
<script src=nonexistent></script>

Outputs "error" in Gecko, "no error" in other browsers (IE10 Developer Preview, Chrome 19 dev, Opera Next 12.00 alpha).

data:text/html,<!doctype html>
<script>addEventListener("error", function(e) {
document.documentElement.textContent =
e.bubbles
}, true)</script>
<script src=nonexistent></script>

Outputs "true" in Gecko, "false" in other browsers.

data:text/html,<!doctype html>
<script src=nonexistent
onerror="document.documentElement.textContent = typeof event + ' ' + event">
</script>

Outputs "string Error loading script" in Gecko, "string " in WebKit, "object [object Event]" in IE/Opera.

Spec:

"""
If the src attribute's value 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.
"""
http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting-1.html#prepare-a-script

This implies that on script load failure, a regular old error Event needs to be fired, which shouldn't bubble.  This matches IE/Opera, and mostly WebKit, except that the last test-case seems to be weird in WebKit.
Are there other cases in which the event _should_ bubble?  We dispatch it in nsScriptElement::ScriptEvaluated covering all failure cases (failure to resolve the URI, failure to fetch, etc), and it pretty carefully only flags "load" as not bubbling, but maybe it should just make that line unconditional?
Created attachment 607239 [details] [diff] [review]
Patch v1

(In reply to Boris Zbarsky (:bz) from comment #1)
> Are there other cases in which the event _should_ bubble?  We dispatch it in
> nsScriptElement::ScriptEvaluated covering all failure cases (failure to
> resolve the URI, failure to fetch, etc), and it pretty carefully only flags
> "load" as not bubbling, but maybe it should just make that line
> unconditional?

Do you mean ScriptEvaluated or ScriptAvailable?  This patch fixes ScriptAvailable, which seems to fix the bug.  Glancing at the code, it seems as though the error event we're interested in comes from nsScriptLoader::OnStreamComplete, which calls nsScriptLoader::FireScriptAvailable if the load fails, which calls nsScriptElement::ScriptAvailable.  nsScriptElement::ScriptEvaluated seems to be called only after the script is evaluated, and it will trigger an exception when nsScriptLoader::EvaluateScript returns an error.  But I don't think we even get to that code path if the resource load fails.

I believe that the error event should bubble in nsScriptElement::ScriptEvaluated, at least if that's what gets hit when there's an uncaught exception.  I don't see anything in the spec about this.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #607239 - Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-u all]

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 3

5 years ago
Autoland Patchset:
	Patches: 607239
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=ef4faafaebe3
Try run started, revision ef4faafaebe3. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=ef4faafaebe3
Comment on attachment 607239 [details] [diff] [review]
Patch v1

> Do you mean ScriptEvaluated or ScriptAvailable?

I meant the former.  I'd missed that the former can fire events too.

> at least if that's what gets hit when there's an uncaught exception.

Uncaught exceptions directly call the error handler on the window, since they pass a totally different set of arguments.  But yes, in addition to that they can trigger the error event from ScriptEvaluated, I think.

The error event from ScriptEvaluated will also happen if the script got moved to a different document while loading or if the document no longer has an associated window or if there are various problems with evaluating the script, looks like...

This is probably something Jonas should review.
Attachment #607239 - Flags: review?(bzbarsky) → review?(jonas)

Comment 5

5 years ago
Try run for ef4faafaebe3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ef4faafaebe3
Results (out of 217 total builds):
    exception: 1
    success: 164
    warnings: 38
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-ef4faafaebe3

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Created attachment 607632 [details] [diff] [review]
Patch v2, test failures fixed

This fixes failures in test_bug696301-1.html and test_bug696301-2.html, which expected a script load failure to call onerror() with three arguments instead of firing a non-bubbling event.  This just changes the type of error expected.
Attachment #607239 - Attachment is obsolete: true
Attachment #607239 - Flags: review?(jonas)
Attachment #607632 - Flags: review?
Whiteboard: [autoland-try:-u all]

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 7

5 years ago
Autoland Patchset:
	Patches: 607632
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=e2e2de8ce990
Try run started, revision e2e2de8ce990. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=e2e2de8ce990

Comment 8

5 years ago
Try run for e2e2de8ce990 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e2e2de8ce990
Results (out of 226 total builds):
    exception: 8
    success: 179
    warnings: 25
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-e2e2de8ce990

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Attachment #607632 - Flags: review? → review?(jonas)
Comment on attachment 607632 [details] [diff] [review]
Patch v2, test failures fixed

Review of attachment 607632 [details] [diff] [review]:
-----------------------------------------------------------------

so the only change here is that we no longer make the event bubble? If so it sounds good to me.
Attachment #607632 - Flags: review?(jonas) → review+
More than that -- currently Gecko passes a string instead of an object in some of these cases.  See comment #0.  Can I assume it's still okay with you?
Ah, yes, looks good.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9053a07fb7a
Flags: in-testsuite+
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/f9053a07fb7a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 14

5 years ago
FYI: I noted this on https://developer.mozilla.org/en/DOM/DOM_event_reference/error#note-script , while cleaning up that page.
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.