949722 - Assigning to ".onerror" of XHR appends an event listener, rather than overwriting it (only in workers)

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Marcus Cavanaugh [:mcav]]

Attachment: xhr.html

Assigning to ".onerror" of XHRs acts as though you called .addEventListener, rather than overwriting the existing event handler. This only happens inside of Web Workers.

Simple test case attached; I assign the same function to "xhr.onerror" several times; the console.log output shows the handler getting called once for every time I assigned to ".onerror".

I'm seeing this on Nightly (29.0a1 2013-12-11). Happy to provide more info if more clarification is needed.

Comment 1

[Marcus Cavanaugh [:mcav]]

Attachment: worker.js

For clarity, to run the test case: Save both "worker.js" and "xhr.html" somewhere, and then load the HTML file. I loaded it in a local web server (rather than from file://).

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Olli?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Ah, right http://hg.mozilla.org/mozilla-central/diff/2e66e7007223/content/events/src/nsEventListenerManager.cpp#l1.32

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: worker_error_with_tests.diff

https://tbpl.mozilla.org/?tree=Try&rev=558c8059313a

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8347424 [details] [diff] [review]
worker_error_with_tests.diff

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

::: dom/src/events/nsJSEventListener.cpp
@@ +170,5 @@
>      InternalScriptErrorEvent* scriptEvent =
>        aEvent->GetInternalNSEvent()->AsScriptErrorEvent();
> +    if (scriptEvent &&
> +        (scriptEvent->message == NS_LOAD_ERROR ||
> +         scriptEvent->typeString.EqualsLiteral("error"))) {

My main concern is that we have to test the string here now to make sure we don't have an error event, but if you're not worried about the performance then I'm not going to worry.

::: dom/workers/test/errorwarning_worker.js
@@ +38,5 @@
>  onerror = errorHandler;
> +onerror = onerror;
> +if (!onerror || onerror != onerror) {
> +  throw "onerror wasn't set properly";
> +}

This fails before your patch, I presume?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)

> My main concern is that we have to test the string here now to make sure we
> don't have an error event, but if you're not worried about the performance
> then I'm not going to worry.
There is scriptEvent check already. So, no, I'm not worried about perf at all


> This fails before your patch, I presume?
Yes

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb453dc051dd

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8347424 [details] [diff] [review]
worker_error_with_tests.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 928312
User impact if declined: wrong behavior
Testing completed (on m-c, etc.): just landed to m-i
Risk to taking this patch (and alternatives if risky): Shouldn't be risky
String or IDL/UUID changes made by this patch: NA

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cb453dc051dd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a1f8b015361