Closed
Bug 1138638
Opened 9 years ago
Closed 8 years ago
browser_mozLoop_sharingListeners.js doesn't work in e10s mode
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(e10s+, firefox47 verified, firefox48 verified)
VERIFIED
FIXED
mozilla48
People
(Reporter: standard8, Assigned: standard8)
References
(Blocks 1 open bug)
Details
(Whiteboard: [e10s])
Attachments
(1 file, 2 obsolete files)
4.91 KB,
patch
|
mikedeboer
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
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
Updated•9 years ago
|
Blocks: e10s-tests
tracking-e10s:
--- → +
Updated•9 years ago
|
Flags: firefox-backlog+
Whiteboard: [e10s]
Updated•9 years ago
|
Rank: 15 → 31
Priority: P1 → P3
Whiteboard: [e10s] → [e10s][fx41]
Updated•9 years ago
|
Whiteboard: [e10s][fx41] → [e10s]
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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+
Comment 4•8 years ago
|
||
(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)); ```
Assignee | ||
Comment 5•8 years ago
|
||
Bumping up priority range, as this could adversely affect us in e10s mode I think.
Assignee: nobody → standard8
Rank: 31 → 19
Priority: P3 → P1
Assignee | ||
Comment 6•8 years ago
|
||
Thanks Mike, this now seems to work locally, going to push it to try.
Assignee | ||
Updated•8 years ago
|
Attachment #8734316 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
Updated patch - should fix the issues with the failures in e10s mode. Going to push to try again.
Assignee | ||
Updated•8 years ago
|
Attachment #8736663 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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+
Assignee | ||
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1df259b9ea3
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 13•8 years ago
|
||
Landed in the Loop repo: https://github.com/mozilla/loop/commit/62fe3f00fc69a28457408e9ce170897d7eca6921
Flags: needinfo?(standard8)
Assignee | ||
Comment 14•8 years ago
|
||
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?
status-firefox47:
--- → affected
Flags: qe-verify+
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+
Comment 16•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/59db0f178e71
Comment 17•8 years ago
|
||
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.
Comment 18•8 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•