941459 - [e10s] pushPrefEnv, popPrefEnv in specialPowersAPI.js not e10s friendly

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

_applyPrefs (which is used by other functions) does 'content.window.setTimeout(...);' which is failing when e10s is enabled with either 'content.window is undefined' or 'cannot ipc non-cpow object', depending on how the tests are run.

Comment 1

[(not currently active) Ted Mielczarek]

This seems odd to me, because this API was designed for e10s use. (In fact Martijn has fixed numerous tests to use it to work on B2G.) Is there some specific weirdness about desktop e10s that makes this not work?

Comment 2

[Martijn Wargers (dead)]

This works just fine in B2G, which uses e10s.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

b2g doesn't run all tests - specialpowersAPI.js seems to be loaded in various different ways (in content process frame scripts, by the browser-test harness, etc.), presumably the cases where it's loaded in chrome are what's failing.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #3)
> presumably the cases where it's loaded in
> chrome are what's failing.

Exactly.  browser_bug705422.js has at the top of the test:

  SpecialPowers.pushPrefEnv({"set": [["network.cookie.cookieBehavior", 0]]}, initTest);

As this is a browser-chrome test, that test file is executed in the parent process and content.window is referencing a content window in the content process.  Thus, when specialPowersAPI.js does:

      content.window.setTimeout(function () {...}, 0);

things go a little pear-shaped.  IIUC, the reference to content.window.setTimeout works (assuming content.window is non-null) due to CPOWs, but the function itself can't be passed around this way.

Comment 5

[Martijn Wargers (dead)]

Attachment: 941459.diff

This makes browser_bug705422.js pass with e10s enabled. I see a leak afterwards, but that seems to happen with any browser-chrome test with e10s enabled.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6cedb28bca8c

Comment 6

[Martijn Wargers (dead)]

Attachment: 941459.diff

Ok, the previous one caused failures. I think this should work fine (it did, at least with limited testing, locally).
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=772150daa300

Comment 7

[Martijn Wargers (dead)]

Attachment: 941459.diff

Still failures, another try, this should work: https://tbpl.mozilla.org/?tree=Try&rev=604ac7882a2e

Comment 8

[Martijn Wargers (dead)]

Ok, everything green, except this test on Linux M-e10s:
TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_event_target_iframe_oop.html | Test timed out.
The timeout happens because of this js error shown in the log:
INFO -  System JS : ERROR chrome://global/content/BrowserElementChild.js:7 - SyntaxError: let is a reserved identifier

I can reproduce this locally, but I have no idea why this is happening. I thought that nested oop iframes don't work, see bug 1033984. So it's kind of weird to see this test being carried out in e10s, currently.

Comment 9

[Martijn Wargers (dead)]

So my question is, should this test, test_event_target_iframe_oop.html, perhaps be disabled for e10s mochitests (only for e10s), because nested oop iframes aren't working correctly, anyhow?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Sounds right. We don't support nested oop iframes.

Comment 11

[Martijn Wargers (dead)]

Attachment: 941459.diff

Ok, now with disabling that test for the e10s case, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=aa56f187d175

Comment 12

[Martijn Wargers (dead)]

Attachment: 941459.diff

Sorry, made a stupid mistake with the previous patch, pushed this to try again: https://tbpl.mozilla.org/?tree=Try&rev=95930123be08

Comment 13

[Martijn Wargers (dead)]

Comment on attachment 8455716 [details] [diff] [review]
941459.diff

Review of attachment 8455716 [details] [diff] [review]:
-----------------------------------------------------------------

Try is green now.

Comment 14

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

Comment on attachment 8455716 [details] [diff] [review]
941459.diff

Review of attachment 8455716 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/specialpowers/content/specialpowersAPI.js
@@ +737,5 @@
> +        if (typeof setTimeout != 'undefined')
> +          setTimeout(callback, 0);
> +        // For mochitest-plain
> +        else
> +          content.window.setTimeout(callback, 0);

I would prefer if you had a local function that did all this checking:
_set_timeout: function(callback) {
  // for mochitest-browser
  if (typeof setTimeout != 'undefined')
    setTimeout(callback, 0);
  // for mochitest-plain
  else
    content.window.setTimeout(callback, 0);
}

then all these calls you converted can be to:
_set_timeout(callback);

Comment 15

[Martijn Wargers (dead)]

Attachment: 94149.diff

Ok, I put it all into 1 function and used that.
I actually have to use "if (window" check instead of "typeof setTimeout != 'undefined'", because with the latter I got some weird chrome js error (which didn't interfere with the test results):
System JS : ERROR resource://gre/modules/Timer.jsm:30 - TypeError: aCallback is null

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6dfaf632aeed

Comment 16

[Martijn Wargers (dead)]

Attachment: 941459.diff

The failures in e10s mochitest in layout/base/tests/test_bug394057.html and layout/style/test/test_dont_use_document_colors.html in the previous try run were happening because the code in _delayCallbackTwice didn't work correctly. I couldn't use this._setTimeout there.
Apparently, those mochitest files get involved with that function, because they use pushPrefEnv multiple times.

Comment 17

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

Comment on attachment 8457638 [details] [diff] [review]
941459.diff

Review of attachment 8457638 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for digging into this!

::: testing/specialpowers/content/specialpowersAPI.js
@@ +742,3 @@
>    _delayCallbackTwice: function(callback) {
> +     function delayedCallback() {
> +       function delayAgain(aCallback) {

please add a comment here why delayAgain isn't able to use _setTimeout.

@@ +746,5 @@
> +        if (typeof window != 'undefined')
> +          setTimeout(aCallback, 0);
> +        // For mochitest-plain
> +        else
> +	        content.window.setTimeout(aCallback, 0);

the indentation for this line isn't consistent.

Comment 18

[Martijn Wargers (dead)]

Attachment: 941459.diff

Ok, added this comment:
         // Using this._setTimeout doesn't work here
         // It causes failures in mochtests that use
         // multiple pushPrefEnv calls
I can't really explain why I couldn't use aThis._setTimeout, apparently, I couldn't pass through 'this' there. I thought I should be able to.
To be honest, I don't even know well, what this _delayCallbackTwice function is doing and why it needs a nested timer. It seems to be involved with mochitests that use multiple pushPrefEnv calls.

Comment 19

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

Comment on attachment 8457933 [details] [diff] [review]
941459.diff

Review of attachment 8457933 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the indentation fixed.  thanks for adding the comment.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +749,5 @@
> +         if (typeof window != 'undefined')
> +           setTimeout(aCallback, 0);
> +         // For mochitest-plain
> +         else
> +	         content.window.setTimeout(aCallback, 0);

please fix the indentation of this line.

Comment 20

[Martijn Wargers (dead)]

Attachment: 941459.diff (for check-in)

(In reply to Joel Maher (:jmaher) from comment #19)
> please fix the indentation of this line.

Sorry, that was caused by a stray tab character, which should up fine in my editor. Fixed now.

Comment 21

[Martijn Wargers (dead)]

I thought I had the last patch checked with the tryserver run in bug 945781, but I can't seem to find the patch in there. So I did a (hopefully) final tryserver run for this patch: https://tbpl.mozilla.org/?tree=Try&rev=d3b25efb883d

Comment 22

[Martijn Wargers (dead)]

There are all kinds of issues with the tryserver now, see bug 1040308, but the tryserver did run at least partially and all the tests were run on at least one platform (except windows, but I wouldn't expect windows specific problems from this patch anyhow) and they are all green.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f701b67c2405

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f701b67c2405