667612 - Uncaught exception: [Exception..."Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMEventTarget.addEventListener)

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv71; rv7.0a1) Gecko/20110627 Firefox/7.0a1 Fennec/7.0a1
Device: Nexus One, Nexus S
OS: Android 2.3.4

I get this upon visiting the URL above.

uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMEventTarget.addEventListener]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://cdn.nearbyad.com/javascripts/MBS_mobile.js?426079 :: <TOP_LEVEL> :: line 352"  data: no]

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

And addEventListener for 'resize' seems to be causing this for Fennec on the Android device. I'm not seeing it on desktop windows Fennec.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

*sigh*, That teaches me not to test. Apparently, when the function is defined after the listener, this error will appear on Fennec Android.

Comment 3

[Martijn Wargers (dead)]

Is this an IPC bug or something?

Comment 4

[Josh Matthews [:jdm]]

According to that JS link, the offending bit is:

var supportsOrientationChange = "onorientationchange" in window,
orientationEvent = supportsOrientationChange ? "orientationchange" : "resize";

window.addEventListener(orientationEvent, MBS.mbsCenterAd, false);

...which I see you figured out. As well as the reason the error occurs. Ok, I'm following now.

Comment 5

[Josh Matthews [:jdm]]

Given that I see the same error on Firefox with your testcase, this doesn't seem to be anything isolated to IPC behaviour.

Comment 6

[Martijn Wargers (dead)]

Ah, yes. I can see the issue in today's Firefox trunk, but not on Firefox 5, so this looks like a regression.

Comment 7

[Boris Zbarsky [:bzbarsky]]

In the attached testcase, x is undefined at the point of the addEventListener call, so I would expect it to throw...

Comment 8

[Boris Zbarsky [:bzbarsky]]

On the other hand, looks like other UAs do not.

Is this happening on aurora too?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48e72227c2fa&tochange=e0b805cab438

So not an aurora problem.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So it looks like nsGlobalWindow::AddEventListener used to directly call nsEventListenerManager::AddEventListenerByType.  This last does NOT null-check aListener, but used to call the internal nsEventListener::AddEventListener and ignore the return value, so always returned NS_OK

Now we're returning the return value of nsEventListenerManager::AddEventListener, which is NS_ERROR_FAILURE in this case.

We need to eat this exception somewhere.  I suggest making nsEventListenerManager::AddEventListener just be a no-op when passed null...

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Unless this breaks more sites I think we should evangelize here instead.

Comment 12

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

AddEventListener must not throw. This is just a bug to fix.

Comment 13

[Johnathan Nightingale [:johnath]]

(In reply to comment #11)
> Unless this breaks more sites I think we should evangelize here instead.

(In reply to comment #12)
> AddEventListener must not throw. This is just a bug to fix.

This is tracking firefox 7 - what approach do we want to take here for the FF7 release?

Comment 14

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

Fix the bug.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Comment 16

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

Comment on attachment 551529 [details] [diff] [review]
Patch to fix

>+<script class="testbody" type="text/javascript">
>+
>+xhr = new XMLHttpRequest;
>+w = new Worker("empty.js");
>+window.addEventListener("load", null, false);
>+document.addEventListener("load", null, false);
>+document.body.addEventListener("load", null, false);
>+xhr.addEventListener("load", null, false);
>+w.addEventListener("load", null, false);
>+window.addEventListener("load", undefined, false);
>+document.addEventListener("load", undefined, false);
>+document.body.addEventListener("load", undefined, false);
>+xhr.addEventListener("load", undefined, false);
>+w.addEventListener("load", undefined, false);
>+
>+</script>


Shouldn't you have ok(true, "Test passed");
right before </script>

With that, r=me

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Trunk patch

This also makes addEventListener not return an nsresult since on trunk aListener being null was the only "real" way addEventListener could fail.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 551529 [details] [diff] [review]
Patch to fix

This fixes a regression where functions that used to be no-ops were caused to throw. The patch reverts behavior so that they are a no-op again.

Comment 19

[Johnathan Nightingale [:johnath]]

Comment on attachment 551529 [details] [diff] [review]
Patch to fix

Get it in before beta next monday, please!

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fixed on aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/54aa371f173b

Comment 21

[Matt Brubeck (:mbrubeck)]

Backed out because the test is failing on Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/e835bb5bb615

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Aurora/1312929770.1312931139.1570.gz
45037 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug667612.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIWorker.addEventListener]" nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)" location: "JS frame :: http://mochi.test:8888/tests/content/events/test/test_bug667612.html :: <TOP_LEVEL> :: line 27" data: no] at :0

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Turns out workers go through completely different codepaths on the aurora branch, which never worked. It does appear to work on trunk though where we're using a totally new worker implementation.

Repushed to branch with the offending tests removed:
http://hg.mozilla.org/releases/mozilla-aurora/rev/c2f69b271358

Comment 23

[(no longer active)]

We couldn't decide whether or not we should transplant anything on Aurora after the migration today, so marking as tracking-firefox8+.  Jonas, can you please let us know if we need to do anything on Aurora?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 551529 [details] [diff] [review]
Patch to fix

Good catch! Since the other patch is still is awaiting review on trunk, we'll have to take this one on (the new, to-be-ff-8) aurora.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/246acc814f72

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/63b8853b1d67

Thanks for the review!

Comment 27

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/63b8853b1d67

Comment 28

[Eric Shepherd [:sheppy]]

I take it all that I'm documenting here is that addEventListnener() now returns as if nothing's wrong if the listener is null?

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup

Comment 30

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/DOM/element.addEventListener#Gecko_notes
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMEventTarget

And mentioned on Firefox 9 for developers.