Closed
Bug 641969
Opened 13 years ago
Closed 13 years ago
SpecialPowers.setBoolPref throws if called from within the test runner suite (but not when running the test standalone)
Categories
(Testing :: Mochitest, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ehsan.akhgari, Assigned: ted)
References
Details
(Whiteboard: fixed-in-bs)
Attachments
(1 file)
When writing my test for bug 640321, I came across a strange thing. I'm using SpecialPowers.setBoolPref. If I run the test standalone (as in, make mochitest-plain TEST_PATH=editor/libeditor/html/tests/test_bug640321.html), the test passes just fine. If I run the test in the testrunner (as in, make mochitest-plain TEST_PATH=editor/libeditor/html), however, SpecialPowers.setBoolPref throws: Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsISyncMessageSender.sendSyncMessage]" nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)" location: "JS frame :: chrome://specialpowers/content/specialpowers.js :: <TOP_LEVEL> :: line 97" data: no] In order to reproduce this, apply attachment 519521 [details] [diff] [review] and run the test with make mochitest-plain TEST_PATH=editor/libeditor/html. I haven't taken the SpecialPowers stuff out of my patch yet, in the hope that this gets resolved before I land my patch. If it doesn't, I'll have to switch to using enablePrivilege before landing.
Assignee | ||
Comment 1•13 years ago
|
||
bug 508664 would fix this in a different way (by forcing TEST_PATH=path/to/test_foo.html to run inside the iframe).
Ted, I think you got it backwards. Wouldn't that make it just always fail?
Assignee | ||
Comment 3•13 years ago
|
||
Oh, oops, you're right.
Reporter | ||
Comment 4•13 years ago
|
||
Yes, Jonas is right in comment 2. Is there anything in the SpecialPowers framework which depends on the testrunner?
Comment 5•13 years ago
|
||
Ehsan, what is throwing that exception?
Reporter | ||
Comment 6•13 years ago
|
||
(In reply to comment #5) > Ehsan, what is throwing that exception? I really don't have any idea. And that's why I've been unable to debug this myself. :( I've taken a look into SendMessageSync, and nothing seems to be returning that error code...
Comment 7•13 years ago
|
||
From nsInProcessTabChildGlobal:
>69 NS_IMETHOD SendSyncMessage()
>70 {
>71 return mMessageManager ? mMessageManager->SendSyncMessage()
>72 : NS_ERROR_NULL_POINTER;
>73 }
That's the only place I can see it coming from.
Comment 8•13 years ago
|
||
hmm, are you getting the error when some iframe has been already teared down?
Comment 9•13 years ago
|
||
It doesn't look like so. I'll debug this tomorrow.
Reporter | ||
Comment 10•13 years ago
|
||
OK, I found out more about why this happens. To reproduce it, you should also apply the patch in attachment 514074 [details] [diff] [review] (in addition to attachment 519521 [details] [diff] [review]). What the test in attachment 514074 [details] [diff] [review] does is that it opens a new tab, does some stuff with it, and then closes it. When the new tab is opened, a new nsInProcessTabChildGlobal object is Inited, and the specialpowers script gets loaded on it. When that tab is closed, nsInProcessTabChildGlobal::Disconnect is called, which cleans up mMessageManager on the second object. When we proceed in the test suite and reach to the test suite of bug 640321, the sendSyncMessage call ends up on the second nsInProcessTabChildGlobal object for some reason, which has a null mMessageManager, which causes the error message in comment 0.
Assignee | ||
Comment 11•13 years ago
|
||
I don't know where message manager bugs actually go, so I'm putting this in IPC.
Component: Mochitest → IPC
Product: Testing → Core
QA Contact: mochitest → ipc
Comment 12•13 years ago
|
||
This might not be a messageManager bug, but a SpecialPowers bug. Haven't debugged yet. I wonder if SpecialPowers ends up installing itself to wrong window. ctalbert?
Comment 13•13 years ago
|
||
And I think that is the case. http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/specialpowers/content/specialpowers.js#132 Nothing seems to check in which tab the new content window is in.
Comment 14•13 years ago
|
||
extensions-content.js uses different mechanism to install things to windows. It adds an event listener, and that way it guaranteed that the window is in the same tab as the TabChildGlobal.
Assignee | ||
Comment 15•13 years ago
|
||
I don't understand why that code is doing something different. I thought the content-document-created notifications were created explicitly to solve this problem? Why does it have to worry about which tab a window is in?
Comment 16•13 years ago
|
||
Because SpecialPowers objects run in TabChild contexts, and there are several them, one for each tab. So if some script in TabChild 1 adds some method to a window under TabChild 2, and then TabChild 1 is destroyed, the method sure can't really do anything reasonable.
Assignee | ||
Comment 17•13 years ago
|
||
Okay, so the mixing of message manager and the content-document-created is the problem here. So, if we hit this problem in code that you and sicking reviewed, I feel like other people are going to hit this problem. Is there a way we could fix the APIs here so that we could only get content-document-created notifications for windows in the tab that our content script is running in? Or at least an API to make it easy to test that?
Comment 18•13 years ago
|
||
Yeah, I should have noticed the problem when reviewing :( The solution is to use an event listener in message manager scripts, not content-document-created.
Assignee | ||
Comment 19•13 years ago
|
||
I filed bug 642457, I think this API is too easy to misuse with e10s, and we have a better solution, and everyone should be writing code assuming e10s, since it's the future direction of Firefox.
Assignee | ||
Comment 20•13 years ago
|
||
In the meantime, it sounds like we should fix SpecialPowers to use the DOMWindowCreated event instead of the observer service notification.
Component: IPC → Mochitest
Product: Core → Testing
QA Contact: ipc → mochitest
Assignee | ||
Comment 21•13 years ago
|
||
I'll fix SpecialPowers.
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
Assignee | ||
Comment 22•13 years ago
|
||
This fixes the SpecialPowers content script to use DOMWindowCreated instead of the observer service notifications. I copy/pasted some code from extensions-content.js here. The Harness_sanity tests all pass with this patch applied, but I'll push it to try for good measure. (This patch contains some minor formatting fixup, js2-mode hates trailing whitespace, and it warns about missing semicolons and trailing commas.)
Attachment #519943 -
Flags: review?(Olli.Pettay)
Comment 23•13 years ago
|
||
Comment on attachment 519943 [details] [diff] [review] fix SpecialPowers to use DOMWindowCreated events instead of observer service notifications. >+function attachSpecialPowersToWindow(aSubject) { s/aSubject/aWindow/
Attachment #519943 -
Flags: review?(Olli.Pettay) → review+
Reporter | ||
Comment 24•13 years ago
|
||
The patch does fix the problem for me!
Assignee | ||
Comment 25•13 years ago
|
||
I pushed this to the build-system repo just to have somewhere to put it: http://hg.mozilla.org/projects/build-system/rev/c52cc6c8f4a8 (also we have some other SpecialPowers work, so it'll be useful to have a repo to work in)
Whiteboard: fixed-in-bs
Reporter | ||
Comment 26•13 years ago
|
||
(In reply to comment #25) > I pushed this to the build-system repo just to have somewhere to put it: > http://hg.mozilla.org/projects/build-system/rev/c52cc6c8f4a8 Can I cherry-pick this fix for bug 640321 on m-c when the tree reopens?
bs is going to merge to m-c right after it reopens.
http://hg.mozilla.org/mozilla-central/rev/c52cc6c8f4a8
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 29•13 years ago
|
||
Why are about: URLs excluded? http://hg.mozilla.org/mozilla-central/rev/c52cc6c8f4a8#l1.109 (With the current patch I have for bug 543435, specialpowers.js checks the URL of the initial about:blank for iframes and fails to check the URL of the doc that the iframe will navigate to.)
Comment 30•13 years ago
|
||
Sorry, I should have noticed the about: problem. That is clearly a bug.
Comment 31•13 years ago
|
||
In bug 681392 I am leaning towards getting rid of the about: exclusion completely.
You need to log in
before you can comment on or make changes to this bug.
Description
•