1253584 - Fix and enable browser_NetworkPrioritizer.js for e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Mike Conley (: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

Comment 3

[Dão Gottwald [:dao]]

(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

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 5

[Dão Gottwald [:dao]]

Attachment: 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.

Comment 6

[Mike Conley (: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.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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

Comment 8

[Dão Gottwald [:dao]]

(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?

Comment 9

[Mike Conley (: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.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6a147100bb29

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a147100bb29

Comment 13

[Dão Gottwald [:dao]]

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

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8727785 [details] [diff] [review]
patch v3

e10s related, taking it.

Comment 15

[Carsten Book [:Tomcat]]

landed as https://hg.mozilla.org/releases/mozilla-aurora/rev/23310a1c6232 on aurora

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8727785 [details] [diff] [review]
patch v3

Perf improvement for e10s, this should land for beta 2.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f10eaee51edd

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4dac2eacb3d0

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 22

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-beta/rev/615df7a98c42

Comment 23

[Mayur Patil]

[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