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)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: ayg, Assigned: ayg)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
6.79 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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?
Assignee | ||
Comment 2•12 years ago
|
||
(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 | ||
Updated•12 years ago
|
Whiteboard: [autoland-try:-u all]
Updated•12 years ago
|
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Comment 3•12 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 4•12 years ago
|
||
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•12 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•12 years ago
|
Whiteboard: [autoland-in-queue]
Assignee | ||
Comment 6•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Whiteboard: [autoland-try:-u all]
Updated•12 years ago
|
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Comment 7•12 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•12 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•12 years ago
|
Whiteboard: [autoland-in-queue]
Assignee | ||
Updated•12 years ago
|
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+
Assignee | ||
Comment 10•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9053a07fb7a
Flags: in-testsuite+
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla14
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9053a07fb7a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 14•12 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.
Description
•