Closed
Bug 1253584
Opened 8 years ago
Closed 8 years ago
Fix and enable browser_NetworkPrioritizer.js for e10s
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 48
People
(Reporter: dao, Assigned: dao)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
19.72 KB,
patch
|
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8726725 -
Flags: review?(mconley)
Comment 2•8 years ago
|
||
Comment on attachment 8726725 [details] [diff] [review] patch Review of attachment 8726725 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Dao! ::: browser/modules/test/browser_NetworkPrioritizer.js @@ +78,2 @@ > > + window_A.gBrowser.removeTab(tab_A2); The cleanest way of removing tabs right now (since there are messages in flight from it even after the browser has gone away), is by using: yield BrowserTestUtils.removeTab(tab_A2); @@ +97,3 @@ > > + // Cleanup > + window_A.gBrowser.removeTab(tab_A3); Same as above, re: BrowserTestUtils.removeTab @@ +129,3 @@ > > + // Cleanup > + gBrowser.removeTab(tab_A2); Same as above, re: BrowserTestUtils.removeTab
Attachment #8726725 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #2) > The cleanest way of removing tabs right now (since there are messages in > flight from it even after the browser has gone away), is by using: > > yield BrowserTestUtils.removeTab(tab_A2); I disagree. Consider this case: + window_A.gBrowser.removeTab(tab_A2); + // Next tab is auto selected + yield isWindowState(window_A, [0, -10]); We expect the other tab to be selected when removeTab is done, and we further expect the network priority to be updated immediately. The other messages are irrelevant here and waiting for them makes the test laxer than we want it to be. Same for BrowserTestUtils.switchTab, which I intentionally didn't use. Using these helpers whenever we want remove or switch tabs in tests seems like a harmful anti-pattern. In fact it might be a good idea to rename them such that they raise a red flag when used in the wrong context...
Comment 4•8 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #3) > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #2) > > The cleanest way of removing tabs right now (since there are messages in > > flight from it even after the browser has gone away), is by using: > > > > yield BrowserTestUtils.removeTab(tab_A2); > > I disagree. Consider this case: > > + window_A.gBrowser.removeTab(tab_A2); > + // Next tab is auto selected > + yield isWindowState(window_A, [0, -10]); > > We expect the other tab to be selected when removeTab is done, and we > further expect the network priority to be updated immediately. The other > messages are irrelevant here and waiting for them makes the test laxer than > we want it to be. > > Same for BrowserTestUtils.switchTab, which I intentionally didn't use. Using > these helpers whenever we want remove or switch tabs in tests seems like a > harmful anti-pattern. In fact it might be a good idea to rename them such > that they raise a red flag when used in the wrong context... I see. In that case, can you please add a comment to the test mentioning that you're testing for synchronous behaviour?
Assignee | ||
Comment 5•8 years ago
|
||
I found an e10s bug: Switching a browser's remoteness resets the priority to 'normal'. I implemented a fix for this and added a better test specifically for this case. Also, browser.xml shouldn't use Ci because that's not defined there. It only happens to be defined in browser.js, which browser.xml can't depend on. Same goes for remote-browser.xml, but fixing that is beyond this bug's scope.
Attachment #8726725 -
Attachment is obsolete: true
Attachment #8726846 -
Flags: review?(mconley)
Comment 6•8 years ago
|
||
Comment on attachment 8726846 [details] [diff] [review] patch v2 Review of attachment 8726846 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for spotting and fixing this bug, Dao. Just minor issues - the most serious one being the use of <xul:browser> as the key to a WeakMap instead of the browser.permanentKey. We might want to get this bugfix uplifted to 46, so I'm adding tracking. ::: browser/modules/NetworkPrioritizer.jsm @@ +35,5 @@ > > // Variables > var _lastFocusedWindow = null; > var _windows = []; > +var _priorityBackup = new WeakMap; Nit - a quick DXR through the codebase shows us predominantly using: new WeakMap(); We should probably do the same thing here for consistency. Can you also write a comment explaining what priorityBackup is being used for? @@ +69,5 @@ > > // Methods that impact a browser. Put into single object for organization. > var BrowserHelper = { > onOpen: function NP_BH_onOpen(aBrowser) { > + _priorityBackup.set(aBrowser, Ci.nsISupportsPriority.PRIORITY_NORMAL); We should always use permanentKey's instead of the browsers themselves when storing in WeakMaps, as the permanentKey's will survive remote frameloader swaps. ::: browser/modules/test/browser_NetworkPrioritizer.js @@ +136,5 @@ > + gBrowser.removeTab(tab2); > + yield setPriority(tab1, oldPriority); > +}); > + > +add_task(function*() { Thanks for adding this new test! ::: toolkit/content/widgets/browser.xml @@ +750,5 @@ > ]]> > </setter> > </property> > > <method name="adjustPriority"> Can you please add some documentation here to distinguish between adjustPriority and setPriority? Same goes for remote-browser.xml.
Attachment #8726846 -
Flags: review?(mconley) → review+
Comment 7•8 years ago
|
||
[Tracking Requested - why for this release]: Requesting tracking because Dao has found an e10s bug where we don't persist network prioritization when a browser switches remoteness. This is a minor bug.
status-firefox46:
--- → affected
tracking-firefox46:
--- → ?
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) > We should always use permanentKey's instead of the browsers themselves when > storing in WeakMaps, as the permanentKey's will survive remote frameloader > swaps. What's a remote frameloader swap? Does it have something to do with tabbrowser.xml's _swapBrowserDocShells? How exactly is this relevant for NetworkPrioritizer.jsm?
Flags: needinfo?(mconley)
Comment 9•8 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #8) > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) > > We should always use permanentKey's instead of the browsers themselves when > > storing in WeakMaps, as the permanentKey's will survive remote frameloader > > swaps. > > What's a remote frameloader swap? Does it have something to do with > tabbrowser.xml's _swapBrowserDocShells? How exactly is this relevant for > NetworkPrioritizer.jsm? You're mapping browsers to a backup of their network priority. In the event that a tab is torn out to a new window, or brought in as a new tab from another window, a docshell swap (or in the case of e10s, a remote frameloader swap) will occur. This means that the browser that was originally mapped to the backup value has been closed and no longer exists. The permanentKey is also swapped when docShells or remote frameloaders are swapped, so it's the safer option to map things to browsers.
Flags: needinfo?(mconley)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8726846 -
Attachment is obsolete: true
Updated•8 years ago
|
tracking-e10s:
--- → +
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6a147100bb29
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8727785 [details] [diff] [review] patch v3 Approval Request Comment [Feature/regressing bug #]: e10s [User impact if declined]: When a non-remote tab (e.g. about:newtab) becomes remote (e.g. when going to any web page!), its network priority gets lost. Could have significant perf impact. [Describe test coverage new/current, TreeHerder]: has test [Risks and why]: fairly straightforward fix, very little room for fallout outside of NetworkPrioritizer.jsm [String/UUID change made/needed]: none
Attachment #8727785 -
Flags: approval-mozilla-beta?
Attachment #8727785 -
Flags: approval-mozilla-aurora?
Comment on attachment 8727785 [details] [diff] [review] patch v3 e10s related, taking it.
Attachment #8727785 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•8 years ago
|
||
landed as https://hg.mozilla.org/releases/mozilla-aurora/rev/23310a1c6232 on aurora
Comment 16•8 years ago
|
||
Comment on attachment 8727785 [details] [diff] [review] patch v3 Perf improvement for e10s, this should land for beta 2.
Attachment #8727785 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•8 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #15) > landed as https://hg.mozilla.org/releases/mozilla-aurora/rev/23310a1c6232 on > aurora This is definitely not the right commit for this bug.
Comment 18•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f10eaee51edd
Comment 20•8 years ago
|
||
Backed out for browser_NetworkPrioritizer.js failures. https://hg.mozilla.org/releases/mozilla-beta/rev/eab422873fad https://treeherder.mozilla.org/logviewer.html#?job_id=880824&repo=mozilla-beta TEST-UNEXPECTED-FAIL | browser/modules/test/browser_NetworkPrioritizer.js | Exception thrown - at chrome://mochitests/content/browser/browser/modules/test/browser_NetworkPrioritizer.js:50 - SyntaxError: yield expression must be parenthesized
Flags: needinfo?(dao)
Comment 21•8 years ago
|
||
Tracking to make sure we land this. I'm not sure we absolutely need this for beta 46, but it sounds like a good idea. Will this help diagnose e10s perf issues?
Assignee | ||
Comment 22•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/615df7a98c42
Flags: needinfo?(dao)
Comment 23•8 years ago
|
||
[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing Actual Results: As expected
You need to log in
before you can comment on or make changes to this bug.
Description
•