Implement process switching for browser-element

RESOLVED FIXED in Firefox 43

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

unspecified
FxOS-S5 (21Aug)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5+, firefox43 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Assignee: nobody → kchen
(Assignee)

Updated

4 years ago
Depends on: 1172889
(Assignee)

Comment 1

4 years ago
Posted patch Prototype WIP (obsolete) — Splinter Review
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.
(Assignee)

Updated

4 years ago
Summary: Prototyping process-isolation of New Security Model → Implement process switching for browser-element
Priority: -- → P1
blocking-b2g: --- → 2.5+
(Assignee)

Comment 2

4 years ago
Posted patch Part 1, Cache TabParent (obsolete) — Splinter Review
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
Attachment #8643544 - Flags: feedback?(wmccloskey)
(Assignee)

Comment 3

4 years ago
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?
Attachment #8643547 - Flags: feedback?(bugs)
(Assignee)

Comment 4

4 years ago
Posted patch Part 3, test case (obsolete) — Splinter Review
Test case. Not sure how to test other APIs yet..
Attachment #8643548 - Flags: feedback?(bugs)
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.
Attachment #8643547 - Flags: feedback?(bugs) → feedback+
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.
Attachment #8643548 - Flags: feedback?(bugs) → feedback+
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.
Attachment #8643544 - Flags: feedback?(wmccloskey) → feedback+
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Updated

4 years ago
Target Milestone: --- → FxOS-S5 (21Aug)
(Assignee)

Comment 10

4 years ago
Posted patch WIP patch (obsolete) — Splinter Review
Attachment #8632681 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Posted patch WIP patch (obsolete) — Splinter Review
Fix a crash on b2g
Attachment #8649132 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
history.back()/.forward() support is expected to be implement together with session history management/restore. For now, always open a new process.
Attachment #8643544 - Attachment is obsolete: true
Attachment #8643547 - Attachment is obsolete: true
Attachment #8643548 - Attachment is obsolete: true
Attachment #8649191 - Attachment is obsolete: true
Attachment #8654797 - Flags: review?(bugs)
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+
Attachment #8654797 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

4 years ago
(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
Flags: needinfo?(kchen)
(Assignee)

Updated

4 years ago
Flags: needinfo?(kchen)
https://hg.mozilla.org/mozilla-central/rev/29c7667aeb8f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1180087
You need to log in before you can comment on or make changes to this bug.