Closed Bug 737087 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: ayg, Assigned: ayg)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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?
Attached patch Patch v1 (obsolete) — Splinter Review
(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]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
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)
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
Whiteboard: [autoland-in-queue]
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]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
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
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
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?
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
Closed: 12 years ago
Resolution: --- → FIXED
FYI: I noted this on https://developer.mozilla.org/en/DOM/DOM_event_reference/error#note-script , while cleaning up that page.
You need to log in before you can comment on or make changes to this bug.