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.

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.

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

Updated with some changes from another bug.

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.

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?

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

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.

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

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

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

Moving old M2 P2 bugs to M4.

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

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

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

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.

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.

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

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.

Maybe we should rename BrowserUtils to ToolkitUtils.

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

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

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

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

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

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.