1453519 - Preloaded browsers, remoteness flipping, and RDM don't work well together

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

For the preloaded browser that loads about:newtab, there are several blocks[1][2] that detect cases where we move away from newtab and want to re-run process selection to possibly grow the set of content processes if we haven't yet reached the max.  This implies a potential process flip (from content process A where it was loading newtab to content process B to hold the web content) for this tab.

This machinery ends up not interacting too well with Responsive Design Mode.  In particular, the following steps:

1. Open a new tab
  * Make sure it's the preloaded browser (can verify with `gBrowser.selectedBrowser.hasAttribute("preloadedState")`)
2. Open Responsive Design Mode (Cmd-Opt-M / Ctrl-Shift-M)
3. Navigate to some URL with the address bar

Navigating away from newtab like this triggers the various blocks[1][2] that want a remoteness flip.  Unfortunately, RDM isn't prepare for content to flip around once it is already open.

Here are some possible paths forward:

1. Block opening RDM for preloaded browsers

This would be technically simple to do, but it's somewhat user hostile.  There appears to be a portion of the user base that likes to start from a new tab, open various DevTools, _then_ navigate to the page of interest.  I am also not sure how we'd explain the situation in an error message that would make any sense to a user.

2. Make RDM fully aware of remoteness flipping

We could attempt to make RDM handle this remoteness flipping situation somehow.  I am quite wary of this option, as there's already quite a lot of complexity around remoteness flipping for normal browsing and also in RDM by itself.  Pursuing this path likely means both RDM and remoteness flipping would grow even more tentacles of mystery and confusion.

3. Once RDM is opened for tab, stop trying to flip remoteness

RDM already requires tabs to be running in the content process (it won't allow opening for parent process content like about:preferences).  We could decide to tweak the several blocks that want to re-run process selection for a preloaded browser to avoid that behavior when RDM is open on the tab.  This seems like it could be a fairly well-contained change.

[1]: https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/browser/base/content/browser.js#1044-1050
[2]: https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/toolkit/modules/E10SUtils.jsm#240-246

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:mconley, you appear to have been involved with preloaded browser work.  Do you prefer any of the options I've outlined here?

Comment 2

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

I agree that (1) and (2) are probably not ideal. Unfortunately, I don't think (3) will be future-proof either, as I suspect we'll likely want to split in-content about: pages (like about:newtab, about:plugins, etc) into their own content process.

If a user inits RDM pointed at about:newtab, could we first start by browsing the tab to about:blank instead?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #2)
> I agree that (1) and (2) are probably not ideal. Unfortunately, I don't
> think (3) will be future-proof either, as I suspect we'll likely want to
> split in-content about: pages (like about:newtab, about:plugins, etc) into
> their own content process.
> 
> If a user inits RDM pointed at about:newtab, could we first start by
> browsing the tab to about:blank instead?

Okay, I think that could work here.

I could explicitly handle about:newtab, or I could try to be more future proof.  Is there a canonical way to ask from the parent process "will navigating to URL X trigger a process redirect"?

For this specific case of newtab, `E10SUtils` only appears to know about the redirect when asked content side via `shouldLoadURI`.  `browser.js` encodes the logic inside `_loadURI` as well, but it would need to be extracted from actually loading to answer yes or no without actually loading.

Does make sense for me to adjust one or more of these so I can detect the redirect ahead of time?

Comment 4

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Does make sense for me to adjust one or more of these so I can detect the
> redirect ahead of time?

Yeah, I think augmenting E10SUtils.jsm makes sense, and making it preloaded-browser aware. Good thinking!

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1453519 - Extract process selection logic from _loadURI.

Extract logic around whether a browser needs to change processes, get a new
frameloader because of preloading, etc. from `_loadURI` in `browser.js` to
`E10SUtils.jsm` where it can be shared with other code paths.

The side effect paths (trying to handle in chrome, removing preloaded state) are
left behind in `browser.js` so that the `E10SUtils` version can be a pure
function.

Review commit: https://reviewboard.mozilla.org/r/241814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241814/

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1453519 - Avoid process reselection inside RDM.

Use `E10SUtils` to check for cases where browser code would try to force a
browser to reselect a new process (such as when it is a preloaded browser) so
that we can avoid any process changes once RDM is open.

If such a case applies, navigate to about:blank first to trigger the process
change before starting RDM.

Review commit: https://reviewboard.mozilla.org/r/241816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241816/

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=016c8c1977f4d5f72f04b3f5beb2408d4c51722c

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8973356 [details]
Bug 1453519 - Avoid process reselection inside RDM.

https://reviewboard.mozilla.org/r/241816/#review247908

Looks good for the case you described in comment 0, but won't you have to handle process changes for fission at some point anyway?

::: devtools/client/responsive.html/browser/swap.js:108
(Diff revision 1)
> +      );
> +      if (newFrameloader) {
> +        debug(`Tab will force a new frameloader on navigation, load about:blank first`);
> +        tab.linkedBrowser.loadURI("about:blank", {
> +          flags: Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_HISTORY,
> +        });

Shouldn't you wait for the page load to finish before proceeding?

Comment 9

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

Comment on attachment 8973355 [details]
Bug 1453519 - Extract process selection logic from _loadURI.

https://reviewboard.mozilla.org/r/241814/#review248026

Looks like a clean extraction to me. Thanks!

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8973355 [details]
Bug 1453519 - Extract process selection logic from _loadURI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241814/diff/1-2/

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8973356 [details]
Bug 1453519 - Avoid process reselection inside RDM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241816/diff/1-2/

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8973356 [details]
Bug 1453519 - Avoid process reselection inside RDM.

https://reviewboard.mozilla.org/r/241816/#review247908

Yes, we'll likely need to do something better in that case. As we get closer to Fission, I'd expect events from platform to allow us to more easily detect the process flip, and we can then use those here.

> Shouldn't you wait for the page load to finish before proceeding?

Right, I tried various approaches here before submitting, but nothing seemed to work... I think the trouble is that since we're doing a process flip, we don't get events across the boundary? Unsure. Anyway, I did find that we get the `XULFrameLoaderCreated` event at least, so I'll wait for that.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f3082e7f2e5ab515ee3da1fa8d3f47ba17aef8a&selectedJob=177372610

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8380fcee2487
Extract process selection logic from _loadURI. r=mconley
https://hg.mozilla.org/integration/autoland/rev/fa9f0e1433a9
Avoid process reselection inside RDM. r=ochameau

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8380fcee2487
https://hg.mozilla.org/mozilla-central/rev/fa9f0e1433a9

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8973355 [details]
Bug 1453519 - Extract process selection logic from _loadURI.

Approval Request Comment
[Feature/Bug causing the regression]: Process reselection for the new tab page interacts poorly with RDM
[User impact if declined]: If declined, opening RDM from the new tab page and then navigating to some URL would abruptly exit RDM.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: All patches in this bug
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Some code is extracted from the main loading path in the browser, but this is a mechanical change that is well covered by tests.  The change to RDM itself affects only that tool.
[String changes made/needed]: None

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8973356 [details]
Bug 1453519 - Avoid process reselection inside RDM.

Approval Request Comment

See comment 16.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8973355 [details]
Bug 1453519 - Extract process selection logic from _loadURI.

Fixes some RDM issues in what seems like an easy enough case to hit. Approved for 61.0b4.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b2f564cbc979
https://hg.mozilla.org/releases/mozilla-beta/rev/6cfe31f3c435

Comment 21

[Hani Yacoub, Desktop QA]

Build ID: 20180510160705
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

Verified as fixed on Firefox 61.0b4 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.