999239 - [e10s] Switching between remote and non-remote URLs loses history

Status: VERIFIED FIXED

(Core :: General, defect, P2)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

When you visit a web site and then go to about:config, you'll lose the entire history for the tab. That's because we remove the <browser> element from the DOM, change the remote attribute, and add it back. When that happens, the remote process is killed and the docshell and history along with it.

One thing that affects this is that pretty soon we'll be switching preferences to load about:preferences in a tab rather than using the dialog box.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch

I'm in the middle of writing tests and comments for this but since it involves changes to docshell I'd like to get some quick feedback that it looks ok. Fundamentally I want a way to be able to catch attempts to load URLs in the wrong process. This adds a new hook to docshell that lets JS decide whether to proceed or not, we can use this to cancel the load and retarget it to the right process.

I also don't really know if I should be creating a new nsIWebBrowserChromeX interface or just adding it to nsIWebBrowserChrome3.

Comment 2

[Dave Townsend [:mossop]]

Forgot to add, this is building on top of a patch in bug 961867

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch

Updated with some changes from another bug.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8483057 [details] [diff] [review]
patch

Adding to the existing interface is fine; we should merge the nsIWebBrowserChrome* anyway.

I'm assuming the cost of doing this check on every load is OK, in general.

The API is a bit weird since it lets you pass in the POST data... but then you can't do anything useful with it if there is any.  I'm not sure whether we should add some assertions that it's null in the cases where we disallow the load or something.  And change the API to not take postdata.

Also, the dump() should probably go away.

I only really read the docshell bits and the shouldLoadURI() impl.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Though.... web content can't link to non-remote stuff, so the scenario described when the bug was filed could be a pure UI thing, not a docshell thing.  It seems a bit weird to have the UI tell docshell to load the URI only to turn around and have docshell ask the UI whether it should _really_ load it

So you really only need this hook for a docshell that has a chrome:// or about: thing loaded with an http:// link in there, right?  Could we make that clearer?

Or do we want to catch silly stuff extensions do?  They might be surprised if their loads aren't happening in the places where they ask them to happen, no?

Comment 7

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> The API is a bit weird since it lets you pass in the POST data... but then
> you can't do anything useful with it if there is any.  I'm not sure whether
> we should add some assertions that it's null in the cases where we disallow
> the load or something.  And change the API to not take postdata.

Can you explain why we can't do anything useful with it?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, what would you do with it?  You could try to ship it across the IPC boundary, if it happens to only contain streams that we can send across IPC, I guess, but I'm not sure we can do that at all right now.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Well, what would you do with it?  You could try to ship it across the IPC
> boundary, if it happens to only contain streams that we can send across IPC,
> I guess, but I'm not sure we can do that at all right now.

My original idea was to serialise it into a blob which we can send across then the other side could use nsIWebNavigation.loadURI to load it properly so I tried to make it match that. Though I never ended up doing that because of sessionstore problems and not really seeing any case where postdata and headers are important for this transition so I'll just remove it from the API for now.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> My original idea was to serialise it into a blob

Hmm.  How well would that work if the postdata contained a file stream for a 10GB file?  Do we have the infrastructure to convert that to a File that we ship across sanely?

Comment 11

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> > My original idea was to serialise it into a blob
> 
> Hmm.  How well would that work if the postdata contained a file stream for a
> 10GB file?  Do we have the infrastructure to convert that to a File that we
> ship across sanely?

Probably not. Ultimately I'm becoming more and more sure that we need something better for transitioning tabs between processes but this might have to be enough for now.

Comment 12

[Chris Peterson [:cpeterson]]

Moving old M2 P2 bugs to M4.

Comment 13

[Chris Peterson [:cpeterson]]

Move old M2's low-priority bugs to M6 milestone.

Comment 14

[Chris Peterson [:cpeterson]]

oops: old M2 P2 bugs should have been moved to new M4 milestone, not M6.

Comment 15

[Dave Townsend [:mossop]]

Attachment: Current WIP

Almost there, just a few test failures to contend with. Still has a bunch of logging that can be removed. Most recent try run is here: https://tbpl.mozilla.org/?tree=Try&rev=b927d135da8d

Comment 16

[Dave Townsend [:mossop]]

Attachment: backend pieces

These are the complete backend pieces for adding the hook to docshell for asking frontend whether to proceed with the load or whether frontend wants to take it over.

Comment 17

[Dave Townsend [:mossop]]

Attachment: Frontend pieces

The frontend checks whether the load is happening in the right process and if not redirects it using session store to restore the history into the browser.

sr from gavin for adding the Browser.jsm module which both browser.js and content.js use. Not sure the name is right but we're going to need a place to put code that both these processes can call.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dave Townsend [:mossop] from comment #17)
> sr from gavin for adding the Browser.jsm module which both browser.js and
> content.js use. Not sure the name is right but we're going to need a place
> to put code that both these processes can call.

I think BrowserUtils.jsm has been used for this? It's in toolkit though...

Comment 19

[Dave Townsend [:mossop]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #18)
> (In reply to Dave Townsend [:mossop] from comment #17)
> > sr from gavin for adding the Browser.jsm module which both browser.js and
> > content.js use. Not sure the name is right but we're going to need a place
> > to put code that both these processes can call.
> 
> I think BrowserUtils.jsm has been used for this? It's in toolkit though...

yeah and this stuff I'm landing relies on browser only stuff so I think it makes sense for a new module.

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Maybe we should rename BrowserUtils to ToolkitUtils.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8494148 [details] [diff] [review]
backend pieces

>+    // Check if the webbrowser chrome wants the load to proceed, this can be

Super-nit: ';', not ',' there.

r=me, but I think we should probably do this before doing the beforeunload bits, no?  Because if ShouldLoadURI returns false, we're not going to actually unload....

Comment 22

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] from comment #21)
> Comment on attachment 8494148 [details] [diff] [review]
> backend pieces
> 
> >+    // Check if the webbrowser chrome wants the load to proceed, this can be
> 
> Super-nit: ';', not ',' there.
> 
> r=me, but I think we should probably do this before doing the beforeunload
> bits, no?  Because if ShouldLoadURI returns false, we're not going to
> actually unload....

I'm not particularly bothered either way, but my reasoning was that this was to be the final hook before actually starting the load. If we move it earlier then a page that the user attempts to navigate to a chrome page somehow wouldn't show any onbeforeunload prompt at all, because we'd just recreate the browser and push it to the new URL. You might call that a regression, though perhaps a welcome one.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> If we move it earlier then a page that the user attempts to navigate to a chrome page
> somehow wouldn't show any onbeforeunload prompt at all

How is this different from the user clicking a link with target="_blank" on the page, from the user's point of view?

Comment 24

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] from comment #23)
> > If we move it earlier then a page that the user attempts to navigate to a chrome page
> > somehow wouldn't show any onbeforeunload prompt at all
> 
> How is this different from the user clicking a link with target="_blank" on
> the page, from the user's point of view?

That opens a new tab so the existing page is still there to view (and presumably save whatever data is waiting there in whatever webapp they have). The code in this patch throws away their existing webpage (though remembers whatever session store can get)

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8494149 [details] [diff] [review]
Frontend pieces

OK. Isn't this more like E10SUtils.jsm, though?

That shouldLoadURI can actually trigger a load is non-obvious from its name. Can you factor out the sendAsyncMessage to another helper? Something like:

function loadAsyncIfNeeded(aDocShell, aURI, aReferrer) {
  if (shouldLoadURI(...))
    return false;
  messageManager.sendAsyncMessage("Browser:LoadURI", ...);
  return true;
}

sr=me with those changes

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8494149 [details] [diff] [review]
Frontend pieces

Review of attachment 8494149 [details] [diff] [review]:
-----------------------------------------------------------------

awesome cleanup!

::: browser/base/content/tabbrowser.xml
@@ +1532,5 @@
>              t.setAttribute("onerror", "this.removeAttribute('image');");
>              t.className = "tabbrowser-tab";
>  
>  #ifdef MAKE_E10S_WORK
> +            let remote = gMultiProcessBrowser && Browser.shouldBrowserBeRemote(aURI);

This looks like the final solution for this, right? I think we could remove the ifdef and always use this path

Comment 27

[Dave Townsend [:mossop]]

Landed with moving the check to before onbeforeunload: https://hg.mozilla.org/integration/fx-team/rev/bd3fa77c80f0

Comment 28

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/bd3fa77c80f0

Comment 29

[Bogdan Maris, Desktop QA]

Reproduced this issue on a Nightly build before the fix and verified that the issue is fixed on Windows 7 64bit, Mac OS X 10.9.5 and Ubuntu 14.04 64bit using latest Nightly.