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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

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

Comment 1

9 years ago
Created attachment 406747 [details] [diff] [review]
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
Attachment #406747 - Flags: review?(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.
(Assignee)

Comment 3

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

Comment 5

9 years ago
> 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?
That's probably safer, yes.  :(
(Assignee)

Comment 7

9 years ago
Created attachment 408465 [details] [diff] [review]
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.
Attachment #406747 - Attachment is obsolete: true
Attachment #408465 - Flags: review?(bzbarsky)
Attachment #406747 - Flags: review?(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
Attachment #408465 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

9 years ago
http://hg.mozilla.org/projects/electrolysis/rev/80adc4f2bad0
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.