Last Comment Bug 667612 - Uncaught exception: [Exception..."Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMEventTarget.addEventListener)
: Uncaught exception: [Exception..."Component returned failure code: 0x80004005...
Status: RESOLVED FIXED
: dev-doc-complete, regression, testcase
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla9
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
http://www.neowin.net
Depends on:
Blocks: 658714
  Show dependency treegraph
 
Reported: 2011-06-27 14:06 PDT by Aaron Train [:aaronmt]
Modified: 2013-12-27 14:37 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
testcase (293 bytes, text/html)
2011-06-30 09:12 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase (306 bytes, text/html)
2011-06-30 09:18 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch to fix (3.14 KB, patch)
2011-08-08 12:23 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bugs: review+
jpr: approval‑mozilla‑aurora+
Details | Diff | Review
Trunk patch (22.23 KB, patch)
2011-08-08 13:37 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bugs: review+
Details | Diff | Review

Description Aaron Train [:aaronmt] 2011-06-27 14:06:17 PDT
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 [:mwargers] (not working for Mozilla) 2011-06-30 09:12:14 PDT
Created attachment 543154 [details]
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 [:mwargers] (not working for Mozilla) 2011-06-30 09:18:07 PDT
Created attachment 543157 [details]
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 [:mwargers] (not working for Mozilla) 2011-06-30 09:21:18 PDT
Is this an IPC bug or something?
Comment 4 Josh Matthews [:jdm] 2011-06-30 10:29:55 PDT
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] 2011-06-30 10:31:19 PDT
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 [:mwargers] (not working for Mozilla) 2011-06-30 10:34:50 PDT
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 [:bz] (Out June 25-July 6) 2011-06-30 11:07:23 PDT
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 [:bz] (Out June 25-July 6) 2011-06-30 11:10:16 PDT
On the other hand, looks like other UAs do not.

Is this happening on aurora too?
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-30 12:11:56 PDT
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48e72227c2fa&tochange=e0b805cab438

So not an aurora problem.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-30 12:18:27 PDT
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) PTO Until July 5th 2011-06-30 14:11:53 PDT
Unless this breaks more sites I think we should evangelize here instead.
Comment 12 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-07-01 06:41:47 PDT
AddEventListener must not throw. This is just a bug to fix.
Comment 13 Johnathan Nightingale [:johnath] 2011-07-13 11:59:11 PDT
(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] (high review load, please consider other reviewers) 2011-07-13 12:34:33 PDT
Fix the bug.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-08 12:23:19 PDT
Created attachment 551529 [details] [diff] [review]
Patch to fix
Comment 16 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-08 12:42:40 PDT
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) PTO Until July 5th 2011-08-08 13:37:00 PDT
Created attachment 551558 [details] [diff] [review]
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) PTO Until July 5th 2011-08-08 13:39:13 PDT
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] 2011-08-09 14:15:11 PDT
Comment on attachment 551529 [details] [diff] [review]
Patch to fix

Get it in before beta next monday, please!
Comment 20 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-09 14:22:37 PDT
Fixed on aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/54aa371f173b
Comment 21 Matt Brubeck (:mbrubeck) 2011-08-09 16:51:34 PDT
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) PTO Until July 5th 2011-08-09 17:29:41 PDT
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 :Ehsan Akhgari (out sick) 2011-08-16 11:58:18 PDT
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) PTO Until July 5th 2011-08-16 13:27:59 PDT
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) PTO Until July 5th 2011-08-16 14:49:43 PDT
Checked in to aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/246acc814f72
Comment 26 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-18 17:37:19 PDT
Checked in to inbound

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

Thanks for the review!
Comment 27 Marco Bonardo [::mak] 2011-08-19 03:05:17 PDT
http://hg.mozilla.org/mozilla-central/rev/63b8853b1d67
Comment 28 Eric Shepherd [:sheppy] 2011-11-04 05:59:34 PDT
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) PTO Until July 5th 2011-11-04 11:03:02 PDT
Yup
Comment 30 Eric Shepherd [:sheppy] 2011-11-04 11:56:58 PDT
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.

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