Modify nsFrameLoader to hold multiple TabParent objects (try to avoid touching nsDocShell). - MessageManager consumers need to be made aware of out-of-date targets. - Simpler: Removing the old frame from the document will kill the old process and discard in-flight messages (similar to approach taken by mossop for e10s). - Harder: Otherwise we could try queuing messages while pausing the old process somehow (maybe even have the OS suspend the process). Session history needs to be retrieved from the old process and stored temporarily in the parent before loading new process, and then history data should be fed to the new process. Initially we could try to load content with different principal in different process. The first prototype may not have session history preserved; it depends on how complete the session restore implementation is. E10s did this in bug 897066 and bug 999239

This currently let you switch to different process in the same Browser window when you enter URL with different origins. It does not choose which process to use, it always creates a new one. It does not manage session history so navigation back/forward is currently not possible. The browser-api may not work normally.

The idea is to cache the TabParent that was being navigated away. Planned but not implemented yet: 1. CreateBrowserOrApp should try to reuse existing TabParent in the cache that loaded the same url 2. Limit the number of cached TabParent and handle memory-pressure event

Switch process and load URI. Put the old TabParent into cache, see Part 1. Have to notify the frameloader-message-manager-will-change observers so they could reattach listeners to the new message manager. Currently only BrowserAPI has been modified but more will be handled in bug 1186843. What do you think about this design?

Test case. Not sure how to test other APIs yet..

Comment on attachment 8643547 [details] [diff] [review] Part 2, Implement nsIFrameLoader::SwitchProcessAndLoadURI Looks rather simple, and good. In BrowserElementParent.js you probably want to check the subject of the notifications.

Comment on attachment 8643544 [details] [diff] [review] Part 1, Cache TabParent >+ for (auto& tab : mCachedTabParent) { >+ tab->Destroy(); >+ } >+ mCachedTabParent.Clear(); Looks error prone. If Destroy at any point starts to do something where scripts might run, the size of mCachedTabParent array might change (if some script loads new pages or so), and this would become a security critical bug. Range-for in C++ is hazard for anything else than very simple iterations. I would do here: nsTArray<nsRefPtr<TabParent>> cachedTabParents; mCachedTabParents.SwapElements(cachedTabParents); for (auto& tab : cachedTabParents) { tab->Destroy(); } to be safe for certain. (I know, I'm occasionally too scared of modern C++ features, but we have had security critical bugs because of old style for-loops were changed to be ranged-for.) >+ >+ nsTArray<nsRefPtr<TabParent>> mCachedTabParent; mCachedTabParents;

Comment on attachment 8643548 [details] [diff] [review] Part 3, test case I guess we should also check that we get a new message manager or so.

Comment on attachment 8643544 [details] [diff] [review] Part 1, Cache TabParent Review of attachment 8643544 [details] [diff] [review]: ----------------------------------------------------------------- First, let me summarize how this approach differs from what we do on desktop when switching processes. Desktop: - Remove <browser> element and add it back to get a new process. - Keep the same <browser> element but get different nsFrameLoader, message manager, and TabParent. - Old nsFrameLoader is destroyed asynchronously so that session store has a chance to send up data to save. - Messages to old TabParent are processed until the "unload" event fires in the child. They can be distinguished from messages to the new TabParent using the targetFrameLoader property on the incoming message. B2G: - Use same <mozbrowser> element and same nsFrameLoader. - Get a different message manager. - nsFrameLoader synchronously disconnects old message manager so that the child doesn't have a chance to send anything up. If you want to get this working with session restore, I think you'll need to make the process more asynchronous so that the child has a chance to send its most recent data up before its message manager is disconnected. It would be much better if you could share the code for this with our existing nsFrameLoader::Destroy code since they do pretty similar things. Also, it seems like we'll probably want to do something to freeze the old process. We probably want to run a "pagehide" event and freeze timeouts and things.

(In reply to Bill McCloskey (:billm) from comment #8) > Comment on attachment 8643544 [details] [diff] [review] > Part 1, Cache TabParent > > Review of attachment 8643544 [details] [diff] [review]: > ----------------------------------------------------------------- > > First, let me summarize how this approach differs from what we do on desktop > when switching processes. > > Desktop: > - Remove <browser> element and add it back to get a new process. > - Keep the same <browser> element but get different nsFrameLoader, message > manager, and TabParent. > - Old nsFrameLoader is destroyed asynchronously so that session store has a > chance to send up data to save. > - Messages to old TabParent are processed until the "unload" event fires in > the child. They can be distinguished from messages to the new TabParent > using the targetFrameLoader property on the incoming message. > > B2G: > - Use same <mozbrowser> element and same nsFrameLoader. > - Get a different message manager. > - nsFrameLoader synchronously disconnects old message manager so that the > child doesn't have a chance to send anything up. > > If you want to get this working with session restore, I think you'll need to > make the process more asynchronous so that the child has a chance to send > its most recent data up before its message manager is disconnected. It would > be much better if you could share the code for this with our existing > nsFrameLoader::Destroy code since they do pretty similar things. > > Also, it seems like we'll probably want to do something to freeze the old > process. We probably want to run a "pagehide" event and freeze timeouts and > things. Thanks for the summary, Bill, it really helps. For freezing the old process, I want to implement it as navigating the old TabChild to about:blank then sends latest session restore data via plain IPDL message so we won't rely on the message manager. Navigating to about:blank should handle "pagehide" and stop the timeeouts etc. automatically.

Fix a crash on b2g

history.back()/.forward() support is expected to be implement together with session history management/restore. For now, always open a new process.

Comment on attachment 8654797 [details] [diff] [review] Implement nsIFrameLoader::SwitchProcessAndLoadURI > NS_IMETHODIMP >+nsFrameLoader::SwitchProcessAndLoadURI(nsIURI* aURI) >+{ >+ nsCOMPtr<nsIURI> URIToLoad = aURI; >+ TabParent* tp = nullptr; I'd prefer this to be nsRefPtr<TabParent> >+ >+ MutableTabContext context; >+ nsCOMPtr<mozIApplication> ownApp = GetOwnApp(); >+ nsCOMPtr<mozIApplication> containingApp = GetContainingApp(); >+ >+ bool rv = true; rv is in general used with nsresult values. Perhaps call it tabContextUpdated, or some such, here? >+nsFrameLoader::SwapRemoteBrowser(nsITabParent* aTabParent) >+{ >+ nsRefPtr<TabParent> newParent = TabParent::GetFrom(aTabParent); >+ if (!newParent || !mRemoteBrowser) { >+ return; >+ } Would it make sense to assert here? Or at least return some error value which the caller can then propagate. nsresult, and return NS_ERROR_DOM_INVALID_STATE_ERR perhaps? >+ nsCOMPtr<nsIObserverService> os = services::GetObserverService(); >+ if (os) { >+ os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this), >+ "frameloader-message-manager-will-change", nullptr); >+ } >+ >+ mRemoteBrowser->CacheFrameLoader(nullptr); >+ mRemoteBrowser->SetOwnerElement(nullptr); >+ mRemoteBrowser->Detach(); >+ mRemoteBrowser->Destroy(); Hmm, this looks odd. Calling destroy here. I would expect TabParent to be really destroyed at that point and not being able to resurrect from the dead later. Could we just call mRemoteBrowser->Detach(); here and let that Detach() to do whatever is needed here. ...but ok, I see this patch doesn't yet implement the TabParent caching, so calling Destroy makes sense for now. >+ if (mMessageManager) { >+ mMessageManager->Disconnect(); >+ mMessageManager = nullptr; >+ } Hmm, so we clear all the message listeners here. I guess that is fine for mozbrowser/app. But we should probably explicitly let this API to be used with mozbrowser/app frames only, and return some error value otherwise. nits fixed, r+

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #13) > >+ mRemoteBrowser->CacheFrameLoader(nullptr); > >+ mRemoteBrowser->SetOwnerElement(nullptr); > >+ mRemoteBrowser->Detach(); > >+ mRemoteBrowser->Destroy(); > Hmm, this looks odd. Calling destroy here. I would expect TabParent to be > really destroyed at that point and not being able to > resurrect from the dead later. > Could we just call mRemoteBrowser->Detach(); here and let that Detach() to > do whatever is needed here. > ...but ok, I see this patch doesn't yet implement the TabParent caching, so > calling Destroy makes sense for now. Yes. The WIP TabParent caching doesn't play well when session history is involved so I striped them out. I'll put a comment here. > >+ if (mMessageManager) { > >+ mMessageManager->Disconnect(); > >+ mMessageManager = nullptr; > >+ } > Hmm, so we clear all the message listeners here. I guess that is fine for > mozbrowser/app. > But we should probably explicitly let this API to be used with > mozbrowser/app frames only, and return some error value otherwise. Make sense.

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13547891&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/dffea8ce8b60