Closed Bug 1167594 Opened 5 years ago Closed 5 years ago

Make it possible for the preloaded browser to be remote

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: mconley, Assigned: ursula)

References

Details

Attachments

(1 file, 4 obsolete files)

When creating the preloaded browser for about:newtab, we never check to see whether or not about:newtab can be remote before creating the browser.

That means that the preloaded browser will always come out as non-remote, which will break things when preloading once about:newtab can run remotely.

We should make it so that createPreloadBrowser for about:newtab (which is the only consumer of the preloaded browser, as far as I can see) can determine whether or not the browser should be remote for the current window.
Hey Tim - I'm pretty sure you worked on the preloading stuff. Can I assume that the preloaded browser in tabbrowser.xml is only being used for about:newtab? That's what it looks like from a glance, but I wanted to make sure.
Flags: needinfo?(ttaubert)
Assignee: nobody → ursulasarracini
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #1)
> Hey Tim - I'm pretty sure you worked on the preloading stuff. Can I assume
> that the preloaded browser in tabbrowser.xml is only being used for
> about:newtab? That's what it looks like from a glance, but I wanted to make
> sure.

Yes.
Flags: needinfo?(ttaubert)
Attachment #8609421 - Flags: review?(mconley)
Comment on attachment 8609421 [details] [diff] [review]
Make it possible for the preloaded browser to be remote

Review of attachment 8609421 [details] [diff] [review]:
-----------------------------------------------------------------

Congrats on your first patch, Ursula! Good stuff!

Please see my comments below.

::: browser/base/content/tabbrowser.xml
@@ +1569,5 @@
>  
>              // Attach the nsIFormFillController now that we know the browser
>              // will be used. If we do that before and the preloaded browser
>              // won't be consumed until shutdown then we leak a docShell.
> +            if (browser && this.hasAttribute("autocompletepopup") && !browser.isRemoteBrowser) {

Let's break these conditions up over several lines, like:

if (browser &&
    this.hasAttribute("autocompletepopup") &&
    !browser.isRemoteBrowser) {
}

I think we should also explain in the comment block above that remote browsers take care of attaching their nsIFormFillControllers themselves, so we can just skip that part.

@@ +1600,5 @@
>              if (this._preloadedBrowser || !this._isPreloadingEnabled()) {
>                return;
>              }
>  
> +            let browser = this._createBrowser({remote: true});

I think we're going to want to keep isPreloadBrowser: true around, and only pass remote: true in the case that E10SUtils.canLoadURIInProcess("about:newtab", Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) is true.

So that logic that you've put into _createBrowser to make it remote, I think we should move it in here.

Also, instead of directly using "about:newtab" here, I suggest using the BROWSER_NEW_TAB_URL const that's hanging around. I know it's not immediately obvious that this was available, but I noticed it when looking at other things using about:newtab in tabbrowser.xml. We should probably be consistent there.
Attachment #8609421 - Flags: review?(mconley) → review-
Attachment #8609421 - Attachment is obsolete: true
Attachment #8609493 - Flags: review?(mconley)
Comment on attachment 8609493 [details] [diff] [review]
Make it possible for the preloaded browser to be remote

Review of attachment 8609493 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! Just some trailing whitespace problems.

::: browser/base/content/tabbrowser.xml
@@ +1570,5 @@
>              // Attach the nsIFormFillController now that we know the browser
>              // will be used. If we do that before and the preloaded browser
>              // won't be consumed until shutdown then we leak a docShell.
> +            // Also, we do not need to take care of attaching nsIFormFillControllers
> +            // in the case that the browser is remote, as remote browsers take 

trailing whitespace

@@ +1572,5 @@
>              // won't be consumed until shutdown then we leak a docShell.
> +            // Also, we do not need to take care of attaching nsIFormFillControllers
> +            // in the case that the browser is remote, as remote browsers take 
> +            // care of that themselves.
> +            if (browser && 

trailing whitespace

@@ +1573,5 @@
> +            // Also, we do not need to take care of attaching nsIFormFillControllers
> +            // in the case that the browser is remote, as remote browsers take 
> +            // care of that themselves.
> +            if (browser && 
> +                this.hasAttribute("autocompletepopup") && 

trailing whitespace
Attachment #8609493 - Flags: review?(mconley) → review-
Attachment #8609493 - Attachment is obsolete: true
Attachment #8609505 - Flags: review?(mconley)
Comment on attachment 8609505 [details] [diff] [review]
Make it possible for the preloaded browser to be remote

Review of attachment 8609505 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +1607,5 @@
>              }
>  
> +            let remote = gMultiProcessBrowser &&
> +                         E10SUtils.canLoadURIInProcess(BROWSER_NEW_TAB_URL, Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT);
> +            let browser = this._createBrowser({remote: true});

Oh - we forgot to re-add isPreloadBrowser to the object being passed to _createBrowser. And we'll need to set remote to be the value of remote, not just true by default.

Next one will be r+, I swear. :)
Attachment #8609505 - Flags: review?(mconley) → review-
Attachment #8609505 - Attachment is obsolete: true
Attachment #8609512 - Flags: review?(mconley)
Attachment #8609512 - Attachment is obsolete: true
Attachment #8609512 - Flags: review?(mconley)
Attachment #8609516 - Flags: review?(mconley)
Comment on attachment 8609516 [details] [diff] [review]
Make it possible for the preloaded browser to be remote

Review of attachment 8609516 [details] [diff] [review]:
-----------------------------------------------------------------

This looks right to me - thanks Ursula!

Now let's get a try push up.
Attachment #8609516 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/77bdf0178eae
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.