695292 - port SimpleTest.executeSoon to SpecialPowers

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

content/html/content/test/test_bug500885.html

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

content/html/content/test/test_bug500885.html uses SpecialPowers.parent, test with this fix.

Comment 3

[Bobby Holley (:bholley)]

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. :-)

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 5

[Bobby Holley (:bholley)]

I think this is easy to fix now. Pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=23acfc856762

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[(not currently active) Ted Mielczarek]

Probably because that has a minimum resolution of 15ms on Windows, so that adds a lot of delay.

Comment 8

[(no longer active)]

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.

Comment 9

[Bobby Holley (:bholley)]

(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.

Comment 10

[Bobby Holley (:bholley)]

(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

Comment 11

[(not currently active) Ted Mielczarek]

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).

Comment 12

[Bobby Holley (:bholley)]

Attachment: Don't use enablePrivilege in SimpleTest.executesoon. v2

Attaching the patch that I pushed. This looks mostly green, but the test_0024_check_incompat_billboard_license.xul failure looks real.

Comment 13

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=076ff134562a

Comment 14

[Bobby Holley (:bholley)]

Almost. Fixed one more issue and pushed again (with a very limited trychooser profile):

https://tbpl.mozilla.org/?tree=Try&rev=e2c0de37b2f6

Comment 15

[Bobby Holley (:bholley)]

Attachment: Port SimpleTest.executeSoon() to SpecialPowers. v3

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. :-(

Comment 16

[Bobby Holley (:bholley)]

Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/e6761635f9cf

Green on try per comment 14.

Comment 17

[Phil Ringnalda (:philor)]

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).

Comment 18

[Bobby Holley (:bholley)]

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?

Comment 19

[Bobby Holley (:bholley)]

Green on android. Pushed back to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e6ae60ea418

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e6ae60ea418