522351 - Only do multi-process tabs when an attribute explicitly asks for it

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Benjamin Smedberg]

Only do the remote thing with <browser remote="true"> (or <iframe remote="true">)

This will let me debug Fennec and still run DOM inspector.

Comment 1

[Benjamin Smedberg]

Attachment: Only do remote tabs explicitly, rev. 1

This works, but by itself makes the test app non-functional because there are various things in frameloader and subdocumentframe which expect docshell-getting to succeed. I'll put the bits necessary for making this work again in bug 522352

Comment 2

[Boris Zbarsky [:bzbarsky]]

So this allows web pages to set mRemoteFrame to true.  Do we handle that case at least somewhat sanely (e.g. show nothing or some such)?  I guess this can be ok as a quick hack but longer-term we'd want to not expose that to content...

General style note: in content code, we brace one-line if/else bodies.

Still trying to sort through the new logic to convince myself it's ok.

Comment 3

[Benjamin Smedberg]

bz, though web pages could set mRemoteFrame to true, they wouldn't actually be able to create the TabParent because of the docshell check in TryNewProcess, so they would just fail to load. And yeah, this is a quick hack.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  So substantive issues:

1)  Why are we getting rid of the mTriedNewProcess boolean?  Does TryNewProcess
    deal ok with an existing mChildProcess?  It doesn't seem to....
2)  This change at the end of CreateDocShell:

>-    return NS_ERROR_FAILURE;
>+    mDocShell = nsnull;

    seems like it might regress bug 472312, no?

Minor issues:

1)  MaybeCreateDocShell() is probably a better name.
2)  Good catch on the missing return in UpdateBaseWindowPositionAndSize. 
    Somewhat surprised that compiled... maybe on Linux only?
3)  Please brace single line if bodies in content code.
4)  MaybeCreateDocShell() should early-return if mRemoteFrame, right?

Comment 5

[Benjamin Smedberg]

> 2)  This change at the end of CreateDocShell:
> 
> >-    return NS_ERROR_FAILURE;
> >+    mDocShell = nsnull;
> 
>     seems like it might regress bug 472312, no?

Hrm, that sucks. I had CreateDocShell returning void and signaling failure with a null mDocShell. Do I need to re-add an nsresult from MaybeCreateDocShell?

Comment 6

[Boris Zbarsky [:bzbarsky]]

That's probably safer, yes.  :(

Comment 7

[Benjamin Smedberg]

Attachment: Only do remote tabs explicitly, rev. 2

This addresses issues, I think. MaybeCreateDocShell is now documented and the nsresult return value is restored. mTriedNewProcess is replaced by a null mChildProcess check.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 408465 [details] [diff] [review]
Only do remote tabs explicitly, rev. 2

>+++ b/content/base/src/nsFrameLoader.cpp
> nsFrameLoader::ReallyStartLoading()
>-  rv = EnsureDocShell();
>   NS_ENSURE_SUCCESS(rv, rv);

Nix that NS_ENSURE_SUCCESS too.

>+nsFrameLoader::ShouldUseRemoteProcess()
>+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);

PRBool remoteDisabled = nsContentUtils::GetBoolPref("dom.ipc.tabs.disabled");

(defaults to false; you can pass in the default as the second arg explicitly if you want to make that clear.

>+  PRBool remoteEnabled = PR_FALSE;

PRBool remoteEnabled = nsContentUtils::GetBoolPref("dom.ipc.tabs.enabled");

>+nsFrameLoader::MaybeCreateDocShell()
>+    // Don't allow subframe loads in external reference documents
>   if (doc->GetDisplayDocument()) {
>-    // Don't allow subframe loads in external reference documents

Don't move the comment, please.

> nsFrameLoader::CheckForRecursiveLoad(nsIURI* aURI)
>+  rv = MaybeCreateDocShell();
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }
>+  if (!mDocShell) {

Assert !mRemoteFrame before the !mDocShell check?

>+++ b/content/base/src/nsFrameLoader.h
>+   * If we are a IPC frame, set mRemoteFrame. Otherwise, create and

"an IPC"?

r=bzbarsky with those nits

Comment 9

[Benjamin Smedberg]

http://hg.mozilla.org/projects/electrolysis/rev/80adc4f2bad0