Last Comment Bug 737087 - Error event fired on <script> load failure is not per spec/other browsers
: Error event fired on <script> load failure is not per spec/other browsers
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla14
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-19 10:31 PDT by :Aryeh Gregor (away until August 15)
Modified: 2014-05-13 22:42 PDT (History)
6 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.82 KB, patch)
2012-03-19 11:57 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch v2, test failures fixed (6.79 KB, patch)
2012-03-20 11:23 PDT, :Aryeh Gregor (away until August 15)
jonas: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-03-19 10:31:41 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2012-03-19 11:38:22 PDT
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?
Comment 2 :Aryeh Gregor (away until August 15) 2012-03-19 11:57:02 PDT
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.
Comment 3 Mozilla RelEng Bot 2012-03-19 12:02:05 PDT
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 4 Boris Zbarsky [:bz] 2012-03-19 12:03:44 PDT
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.
Comment 5 Mozilla RelEng Bot 2012-03-19 18:31:33 PDT
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
Comment 6 :Aryeh Gregor (away until August 15) 2012-03-20 11:23:02 PDT
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.
Comment 7 Mozilla RelEng Bot 2012-03-20 12:15:12 PDT
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 Mozilla RelEng Bot 2012-03-20 19:46:27 PDT
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
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-01 22:49:32 PDT
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.
Comment 10 :Aryeh Gregor (away until August 15) 2012-04-02 08:00:54 PDT
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?
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-02 09:53:08 PDT
Ah, yes, looks good.
Comment 12 :Aryeh Gregor (away until August 15) 2012-04-03 07:16:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9053a07fb7a
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-04 04:44:22 PDT
https://hg.mozilla.org/mozilla-central/rev/f9053a07fb7a
Comment 14 Nickolay_Ponomarev 2012-04-07 03:56:56 PDT
FYI: I noted this on https://developer.mozilla.org/en/DOM/DOM_event_reference/error#note-script , while cleaning up that page.

Note You need to log in before you can comment on or make changes to this bug.