Closed Bug 667612 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Events, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9
Tracking Status
firefox7 + fixed
firefox8 + fixed

People

(Reporter: aaronmt, Assigned: sicking)

References

()

Details

(Keywords: dev-doc-complete, regression, testcase)

Attachments

(2 files, 2 obsolete files)

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]
Attached file testcase (obsolete) —
And addEventListener for 'resize' seems to be causing this for Fennec on the Android device. I'm not seeing it on desktop windows Fennec.
Attached file testcase (obsolete) —
*sigh*, That teaches me not to test. Apparently, when the function is defined after the listener, this error will appear on Fennec Android.
Attachment #543154 - Attachment is obsolete: true
Is this an IPC bug or something?
Keywords: testcase
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.
Given that I see the same error on Firefox with your testcase, this doesn't seem to be anything isolated to IPC behaviour.
Ah, yes. I can see the issue in today's Firefox trunk, but not on Firefox 5, so this looks like a regression.
Component: General → DOM: Events
Product: Fennec → Core
QA Contact: general → events
In the attached testcase, x is undefined at the point of the addEventListener call, so I would expect it to throw...
On the other hand, looks like other UAs do not.

Is this happening on aurora too?
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...
Unless this breaks more sites I think we should evangelize here instead.
AddEventListener must not throw. This is just a bug to fix.
(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?
Fix the bug.
Assignee: nobody → jonas
Attached patch Patch to fixSplinter Review
Attachment #543157 - Attachment is obsolete: true
Attachment #551529 - Flags: review?(Olli.Pettay)
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
Attachment #551529 - Flags: review?(Olli.Pettay) → review+
Attached patch Trunk patchSplinter Review
This also makes addEventListener not return an nsresult since on trunk aListener being null was the only "real" way addEventListener could fail.
Attachment #551558 - Flags: review?(Olli.Pettay)
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.
Attachment #551529 - Flags: approval-mozilla-aurora?
Comment on attachment 551529 [details] [diff] [review]
Patch to fix

Get it in before beta next monday, please!
Attachment #551529 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
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
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 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.
Attachment #551529 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Attachment #551529 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #551558 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/63b8853b1d67
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
I take it all that I'm documenting here is that addEventListnener() now returns as if nothing's wrong if the listener is null?
You need to log in before you can comment on or make changes to this bug.