Fix and enable browser_NetworkPrioritizer.js for e10s

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 48
Points:
---

Firefox Tracking Flags

(e10s+, firefox46+ fixed, firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8726725 [details] [diff] [review]
patch
Attachment #8726725 - Flags: review?(mconley)
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

3 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...
(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

3 years ago
Created attachment 8726846 [details] [diff] [review]
patch v2

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 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+
[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

3 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)
(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

3 years ago
Created attachment 8727785 [details] [diff] [review]
patch v3
Attachment #8726846 - Attachment is obsolete: true

Updated

3 years ago
tracking-e10s: --- → +

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a147100bb29
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(Assignee)

Comment 13

3 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+
landed as https://hg.mozilla.org/releases/mozilla-aurora/rev/23310a1c6232 on aurora
status-firefox47: affected → fixed
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+
(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.
status-firefox47: fixed → affected

Comment 18

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/f10eaee51edd
status-firefox46: affected → fixed
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
status-firefox46: fixed → affected
Flags: needinfo?(dao)
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?
tracking-firefox46: ? → +
(Assignee)

Comment 22

3 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/615df7a98c42
status-firefox46: affected → fixed
Flags: needinfo?(dao)

Comment 23

2 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.