Closed Bug 941459 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(e10s+)

RESOLVED FIXED
mozilla33
Tracking Status
e10s + ---

People

(Reporter: markh, Assigned: martijn.martijn)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(1 file, 8 obsolete files)

_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.
Blocks: fxe10s
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?
This works just fine in B2G, which uses e10s.
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.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
Attached patch 941459.diff (obsolete) — Splinter Review
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
Attached patch 941459.diff (obsolete) — Splinter Review
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
Attachment #8454949 - Attachment is obsolete: true
Assignee: nobody → martijn.martijn
Attached patch 941459.diff (obsolete) — Splinter Review
Still failures, another try, this should work: https://tbpl.mozilla.org/?tree=Try&rev=604ac7882a2e
Attachment #8455013 - Attachment is obsolete: true
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.
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?
Flags: needinfo?(bugs)
Sounds right. We don't support nested oop iframes.
Flags: needinfo?(bugs)
Attached patch 941459.diff (obsolete) — Splinter Review
Ok, now with disabling that test for the e10s case, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=aa56f187d175
Attachment #8455028 - Attachment is obsolete: true
Attached patch 941459.diff (obsolete) — Splinter Review
Sorry, made a stupid mistake with the previous patch, pushed this to try again: https://tbpl.mozilla.org/?tree=Try&rev=95930123be08
Attachment #8455553 - Attachment is obsolete: true
Comment on attachment 8455716 [details] [diff] [review]
941459.diff

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

Try is green now.
Attachment #8455716 - Flags: review?(jmaher)
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);
Attachment #8455716 - Flags: review?(jmaher) → review-
Attached patch 94149.diff (obsolete) — Splinter Review
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
Attachment #8455716 - Attachment is obsolete: true
Attached patch 941459.diff (obsolete) — Splinter Review
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.
Attachment #8456985 - Attachment is obsolete: true
Attachment #8457638 - Flags: review?(jmaher)
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.
Attachment #8457638 - Flags: review?(jmaher) → review+
Attached patch 941459.diff (obsolete) — Splinter Review
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.
Attachment #8457638 - Attachment is obsolete: true
Attachment #8457933 - Flags: review?(jmaher)
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.
Attachment #8457933 - Flags: review?(jmaher) → review+
(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.
Attachment #8457933 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
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
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f701b67c2405
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
See Also: → 1050706
Depends on: 1464628
You need to log in before you can comment on or make changes to this bug.