Closed Bug 1138638 Opened 5 years ago Closed 4 years ago

browser_mozLoop_sharingListeners.js doesn't work in e10s mode

Categories

(Hello (Loop) :: Client, defect, P1)

defect

Tracking

(e10s+, firefox47 verified, firefox48 verified)

VERIFIED FIXED
mozilla48
Tracking Status
e10s + ---
firefox47 --- verified
firefox48 --- verified

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [e10s])

Attachments

(1 file, 2 obsolete files)

When I landed bug 1131574, the e10s bc1 tests broke. For now, I've disabled the test for e10s mode, as it looks more like a unit test issue than a code issue (I verified with manual testing the code was working as expected).

34 INFO TEST-UNEXPECTED-FAIL | browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js | window id should be valid - null != null - JS frame :: chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js :: test_singleListener :: line 70
Stack trace:
    chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js:test_singleListener:70
    self-hosted:next:620
    Tester_execTest@chrome://mochikit/content/browser-test.js:688:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:611:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:752:59
35 INFO TEST-UNEXPECTED-FAIL | browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js | Tab contentWindow IDs shouldn't be the same - null != null - JS frame :: chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js :: test_singleListener :: line 77
Stack trace:
    chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js:test_singleListener:77
    self-hosted:next:620
    Tester_execTest@chrome://mochikit/content/browser-test.js:688:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:611:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:752:59
36 INFO TEST-UNEXPECTED-FAIL | browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js | window id should be valid - null != null - JS frame :: chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js :: test_multipleListener :: line 90
Stack trace:
    chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js:test_multipleListener:90
    self-hosted:next:620
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
37 INFO TEST-UNEXPECTED-FAIL | browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js | window id should be valid - null != null - JS frame :: chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js :: test_multipleListener :: line 96
Stack trace:
    chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js:test_multipleListener:96
    self-hosted:next:620
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
38 INFO TEST-UNEXPECTED-FAIL | browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js | Tab contentWindow IDs shouldn't be the same - null != null - JS frame :: chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js :: test_multipleListener :: line 106
Stack trace:
    chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js:test_multipleListener:106
    self-hosted:next:620
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
39 INFO TEST-UNEXPECTED-FAIL | browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js | Second listener should have updated - null != null - JS frame :: chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js :: test_multipleListener :: line 118
Stack trace:
    chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_sharingListeners.js:test_multipleListener:118
    self-hosted:InterpretGeneratorResume:712
    self-hosted:next:620
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
SUITE-END | took 11s
Putting as high rank as we should try and fix it whilst we're here before it turns into tech-debt.

On irc Mike said:

"""
note that this is where the outerWindowID is sent up to the parent: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js#599
so there is a small pocket of time when we're loading this script where there is no outerWindowID defined
"""

Running the test manually it certainly looked like it could be a timing issue.
Rank: 15
Priority: -- → P1
Blocks: e10s-tests
tracking-e10s: --- → +
Flags: firefox-backlog+
Whiteboard: [e10s]
Rank: 15 → 31
Priority: P1 → P3
Whiteboard: [e10s] → [e10s][fx41]
Blocks: 1099241
No longer blocks: 1115336
Whiteboard: [e10s][fx41] → [e10s]
Blocks: loop-e10s
Attached patch Bad workaround (obsolete) — Splinter Review
I need some help here please. If I add in this timer delay, then the test pretty much passes in e10s mode (there's one failure in test_newtabLocation which I haven't looked at yet).

However, without the timer, then the code to get the window id (gBrowser.selectedBrowser.outerWindowID) returns null and the tests fail in multiple locations.

I tried adding a wait for load, but it still didn't work (unless I did it wrong).

I can't see any obvious effects of the tests here, but I could do with someone with a bit more knowledge taking this on.
Attachment #8734316 - Flags: feedback?(mdeboer)
Attachment #8734316 - Flags: feedback?(mconley)
Comment on attachment 8734316 [details] [diff] [review]
Bad workaround

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

::: browser/extensions/loop/bootstrap.js
@@ +557,5 @@
> +        timer.initWithCallback(function () {
> +          Cu.reportError("broadcasting " + gBrowser.selectedBrowser.outerWindowID);
> +          this.LoopAPI.broadcastPushMessage("BrowserSwitch",
> +            gBrowser.selectedBrowser.outerWindowID);
> +        }.bind(this), 1000, Ci.nsITimer.TYPE_ONE_SHOT);

Well, you might consider this another hack, but hear me out ;-)

If we don't have an `outerWindowID` here, this means that the 'Browser:Init' message didn't arrive yet - https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js#635.

So if you add the following code here, things should start working:

```js
let browser = gBrowser.selectedBrowser;
return new Promise(resolve => {
  if (browser.outerWindowID) {
    resolve(browser.outerWindowID);
    return;
  }
  browser.messageManager.addMessageListener("Browser:Init", function initListener() {
    browser.messageManager.removeMessageListener("Browser:Init", initListener);
    resolve(browser.outerWindowID);
  }).then(outerWindowID => this.LoopAPI.broadcastPushMessage("BrowserSwitch", outerWindowID));
```
Attachment #8734316 - Flags: feedback?(mdeboer)
Attachment #8734316 - Flags: feedback?(mconley)
Attachment #8734316 - Flags: feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #3)
```js
let browser = gBrowser.selectedBrowser;
return new Promise(resolve => {
  if (browser.outerWindowID) {
    resolve(browser.outerWindowID);
    return;
  }
  browser.messageManager.addMessageListener("Browser:Init", function initListener() {
    browser.messageManager.removeMessageListener("Browser:Init", initListener);
    resolve(browser.outerWindowID);
  });
}).then(outerWindowID => this.LoopAPI.broadcastPushMessage("BrowserSwitch", outerWindowID));
```
Bumping up priority range, as this could adversely affect us in e10s mode I think.
Assignee: nobody → standard8
Rank: 31 → 19
Priority: P3 → P1
Thanks Mike, this now seems to work locally, going to push it to try.
Attachment #8734316 - Attachment is obsolete: true
Updated patch - should fix the issues with the failures in e10s mode. Going to push to try again.
Attachment #8736663 - Attachment is obsolete: true
Comment on attachment 8736840 [details] [diff] [review]
Make Loop's browser sharing work properly in e10s mode and enable the mochitest.

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

Ok, try server says this is good, lets go for it.
Attachment #8736840 - Flags: review?(mdeboer)
Comment on attachment 8736840 [details] [diff] [review]
Make Loop's browser sharing work properly in e10s mode and enable the mochitest.

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

Thanks! r=me with pref fiddling removed.

::: browser/extensions/loop/chrome/test/mochitest/browser_mozLoop_sharingListeners.js
@@ +49,5 @@
>  }
>  
>  function promiseNewTabLocation() {
>    BrowserOpenTab();
>    let tab = gBrowser.selectedTab;

Perhaps you can add a `let browser = tab.linkedBrowser;` here and replace the refs below.

@@ +80,5 @@
>    createdTabs = [];
>  }
>  
> +add_task(function* test_setup() {
> +  Services.prefs.setBoolPref("loop.remote.autostart", true);

Isn't this already the default?
Attachment #8736840 - Flags: review?(mdeboer) → review+
Flagging to remind myself to import this back into the Loop repo once its confirmed to have landed successfully. Also to uplift to Aurora.
Flags: needinfo?(standard8)
https://hg.mozilla.org/mozilla-central/rev/a1df259b9ea3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Landed in the Loop repo:

https://github.com/mozilla/loop/commit/62fe3f00fc69a28457408e9ce170897d7eca6921
Flags: needinfo?(standard8)
Comment on attachment 8736840 [details] [diff] [review]
Make Loop's browser sharing work properly in e10s mode and enable the mochitest.

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello, e10s
[User impact if declined]: Tab sharing may happen erratically in some e10s cases.
[Describe test coverage new/current, TreeHerder]: Gets another test working in e10s mode.
[Risks and why]: Low, small changed covered by test.
[String/UUID change made/needed]: None
Attachment #8736840 - Flags: approval-mozilla-aurora?
Comment on attachment 8736840 [details] [diff] [review]
Make Loop's browser sharing work properly in e10s mode and enable the mochitest.

Improves Hello + e10s, Aurora47+
Attachment #8736840 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We recently done a smoketest on Firefox 47 beta 4 across platforms using Hello version 1.3.2 with e10s enabled and can confirm that we did not encounter any issues while sharing.
Based on our recent testing done on Firefox 48 beta 9 using Hello version 1.4.3 we can close this as verified fixed since we did not encounter any issues regarding sharing.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.