Closed Bug 1453519 Opened Last year Closed Last year

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

Categories

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

defect

Tracking

(firefox59 wontfix, firefox60 wontfix, firefox61 verified, firefox62 verified)

VERIFIED FIXED
Firefox 62
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified
firefox62 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files)

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
:mconley, you appear to have been involved with preloaded browser work.  Do you prefer any of the options I've outlined here?
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(jryans)
(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?
Flags: needinfo?(jryans) → needinfo?(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!
Flags: needinfo?(mconley)
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?
Attachment #8973356 - Flags: review?(poirot.alex) → review+
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!
Attachment #8973355 - Flags: review?(mconley) → review+
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.
Pushed by jryans@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/8380fcee2487
https://hg.mozilla.org/mozilla-central/rev/fa9f0e1433a9
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Status: RESOLVED → VERIFIED
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
Attachment #8973355 - Flags: approval-mozilla-beta?
Comment on attachment 8973356 [details]
Bug 1453519 - Avoid process reselection inside RDM.

Approval Request Comment

See comment 16.
Attachment #8973356 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1448859
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.
Attachment #8973355 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8973356 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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.
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.