Closed Bug 641969 Opened 9 years ago Closed 9 years ago

SpecialPowers.setBoolPref throws if called from within the test runner suite (but not when running the test standalone)

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ehsan, 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.
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?
Oh, oops, you're right.
Yes, Jonas is right in comment 2.

Is there anything in the SpecialPowers framework which depends on the testrunner?
Ehsan, what is throwing that exception?
(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...
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.
hmm, are you getting the error when some iframe has 
been already teared down?
It doesn't look like so.
I'll debug this tomorrow.
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.
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
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?
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.
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.
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?
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.
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?
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.
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.
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
I'll fix SpecialPowers.
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
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 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+
The patch does fix the problem for me!
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
(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: 9 years ago
Resolution: --- → FIXED
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.)
Depends on: 684690
Sorry, I should have noticed the about: problem. That is clearly a bug.
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.