Closed Bug 695292 Opened 9 years ago Closed 8 years ago

port SimpleTest.executeSoon to SpecialPowers

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: jmaher, Unassigned)

References

Details

(Whiteboard: [specialpowers])

Attachments

(1 file, 1 obsolete file)

this is one of the last remaining instances of enablePrivilege in the mochitest harness.  In general this works, but the few cases that fail seem to be results of executing the code in the parent.SpecialPowers instance instead of the test instance.

For example, test_reftests_with_caret.html loads a bunch of subpages and these subpages don't get a specialPowers object attached to it, so we have code that will use the parent.SpecialPowers instead.  When we run SpecialPowers.executeSoon, we end up executing the code in the parent scope (test_reftests_with_caret) and not in the subpage scope.
content/html/content/test/test_bug500885.html
content/html/content/test/test_bug500885.html uses SpecialPowers.parent, test with this fix.
I'd previous discussed working on this, but I think for the moment I'm going to focus my energy on removing enablePrivilege from content/* and dom/*, which seems like the real long pole at the moment. :-)
tracking bug to fix all content tests:
https://bugzilla.mozilla.org/show_bug.cgi?id=682389

those patches need to be updated to us pushPrefEnv instead of set*Pref.
I think this is easy to fix now. Pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=23acfc856762
Gah, doing executeSoon with SpecialPowers breaks stuff, because exceptions thrown in the callback function don't hit the content window.onerror.

It's not clear to me why we can't just replace this nsIThreadManager nonsense with setTimeout(func, 0). Doing a try push to see what breaks:

https://tbpl.mozilla.org/?tree=Try&rev=c046fa9f9dbe
Probably because that has a minimum resolution of 15ms on Windows, so that adds a lot of delay.
Yeah the point of executeSoon is the "soon" part.  :-)  Also, on every platform we clamp up timeout values to a minimum of 4(?)ms, which means that this will cause extensive delays across the board.
(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> Yeah the point of executeSoon is the "soon" part.  :-)  Also, on every
> platform we clamp up timeout values to a minimum of 4(?)ms, which means that
> this will cause extensive delays across the board.

This is not true AFAICT. Per spec the delay is only for _nested_ setTimeout.
(In reply to Ted Mielczarek [:ted] from comment #7)
> Probably because that has a minimum resolution of 15ms on Windows, so that
> adds a lot of delay.

I also don't think timer resolution affects us in the timeout=0 case. When the timer is added, we insert it in the sorted timeout list. If it ends up in the front of the list, we notify it immediately:
http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp#362
Okay, so this is only an issue with executeSoon chaining. I'm not sure how much that's used in our tests (although I assume it is used a bit).
Attaching the patch that I pushed. This looks mostly green, but the test_0024_check_incompat_billboard_license.xul failure looks real.
Blocks: 757046
Almost. Fixed one more issue and pushed again (with a very limited trychooser profile):

https://tbpl.mozilla.org/?tree=Try&rev=e2c0de37b2f6
Finally green on try: https://tbpl.mozilla.org/?tree=Try&rev=e2c0de37b2f6

Flagging blake for review. In theory, this is the last big dependency to killing enablePrivilege as we know it. Huzzah!

Patch description (commit message) follows.

----

If we just naively use the thread manager to dispatch the event via SpecialPowers,
we cannot avoid the XPCWrappedJS for the runnable being in chrome scope (even if
we pass a content object, we make the call in chrome scope, so we pass a cross-
compartment wrapper). This means that the machinery in
nsXPCWrappedJSClass::CheckForException ends up calling the error reporter for the
SpecialPowers scope, which isn't what we want.

Messing around with the implementation of CheckForException is playing with fire.
So we work around this with an explicit Cu API. :-(
Attachment #652321 - Flags: review?(mrbkap)
Attachment #631398 - Attachment is obsolete: true
Attachment #652321 - Flags: review?(mrbkap) → review+
Prolly a good idea to *widen* the trychooser syntax rather than narrow it - backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/83c15a039a09 because whatever is causing Cu to be inherited from somewhere up above on other platforms, it's not defined on Android, so you were busted on M1, M2, M7 and M8 (I presume that's all the chunks that have anything using SpecialPowers, though if it were me, I'd run all of them on try rather than just those four).
Thanks for the analysis philor. Defined Cu at the top of the file and pushed for a new android try run:
https://tbpl.mozilla.org/?tree=Try&rev=d4109c5c7f1c

(In reply to Phil Ringnalda (:philor) from comment #17)
> Prolly a good idea to *widen* the trychooser syntax rather than narrow it -

I do a lot of try pushes, so I've been trying to use narrower profiles to reduce load on our overburdened builders. It doesn't catch _everything_, but the inbound builders do, so it seems to me like a good compromise given the current limited resources. Should I continue with this practice?
https://hg.mozilla.org/mozilla-central/rev/1e6ae60ea418
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.