First browser should be dynamically created to allow honoring of browser.tabs.remote pref

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Felipe, Assigned: billm)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
Firefox 23
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [e10s])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
The browser.tabs.remote pref controls if the remote="true" tag should be added to the browser element to load it in the content process. This has to be decided on creation time, and having the first element hardcoded in tabbrowser.xml makes this not possible.

I've sent this patch to tryserver and there weren't any problem with tests or talos, the numbers were the same. I'm giving it a second run now.

The alternative option would be to have platform support to switch between remote/non-remote dinamically, which might be necessary in the future anyways, but this is trickier and much harder to do, I was told. Even with that, I think the change in this patch is a good change as it makes things more flexible.

I thought the change would be scary but it's just the browser element that needs to be removed. The browserStack can remain coded there, as well as the first <tab> which is not part of the same hierarchy, so this should present no issues to overlays as I understand it.
(Reporter)

Comment 1

7 years ago
Created attachment 569555 [details] [diff] [review]
Patch

Thoughts? Who wants to review?
Attachment #569555 - Flags: review?(gavin.sharp)
Attachment #569555 - Flags: review?(dao)
Comment on attachment 569555 [details] [diff] [review]
Patch

>             b.setAttribute("autoscrollpopup", this._autoScrollPopup.id);

This is duplicated in the tabbrowser constructor.

>+          if (Services.prefs.getPrefType("browser.tabs.remote") == Services.prefs.PREF_BOOL &&
>+            Services.prefs.getBoolPref("browser.tabs.remote")) {
>+            browser.setAttribute("remote", "true");
>+          }

indentation is off (second line needs two more spaces)

>       <constructor>
>         <![CDATA[
>-          this.mCurrentBrowser = this.mPanelContainer.childNodes[0].firstChild.firstChild;
>+          let browser = this._createBrowserElement();
>+          browser.setAttribute("type", "content-primary");
>+          browser.setAttribute("disablehistory", "true");

Does this run much earlier than prepareForStartup? If not, setting disablehistory here seems pointless. The attribute was in the markup "so that we don't have to load global history before bringing up a window".
(Reporter)

Comment 3

7 years ago
(In reply to Dão Gottwald [:dao] from comment #2)
> >             b.setAttribute("autoscrollpopup", this._autoScrollPopup.id);
> 
> This is duplicated in the tabbrowser constructor.

Sorry, I don't see where this is duplicated. Did you confuse it with the autocompletepopup that I moved to the _createBrowserElement func?

> 
> >+          if (Services.prefs.getPrefType("browser.tabs.remote") == Services.prefs.PREF_BOOL &&
> >+            Services.prefs.getBoolPref("browser.tabs.remote")) {
> >+            browser.setAttribute("remote", "true");
> >+          }
> 
> indentation is off (second line needs two more spaces)

ops

> 
> >       <constructor>
> >         <![CDATA[
> >-          this.mCurrentBrowser = this.mPanelContainer.childNodes[0].firstChild.firstChild;
> >+          let browser = this._createBrowserElement();
> >+          browser.setAttribute("type", "content-primary");
> >+          browser.setAttribute("disablehistory", "true");
> 
> Does this run much earlier than prepareForStartup? If not, setting
> disablehistory here seems pointless. The attribute was in the markup "so
> that we don't have to load global history before bringing up a window".

Yeah, it runs earlier. (don't know how to tell "much earlier"), but the constructors run before BrowserStartup, in this order: tabbrowser, browser, tabbrowser-tabs. If I remove the line setting the attribute there's a sessionstore test that fails (browser_394759_basic.js)
(In reply to Felipe Gomes (:felipe) from comment #3)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > >             b.setAttribute("autoscrollpopup", this._autoScrollPopup.id);
> > 
> > This is duplicated in the tabbrowser constructor.
> 
> Sorry, I don't see where this is duplicated. Did you confuse it with the
> autocompletepopup that I moved to the _createBrowserElement func?

I meant this line:
>          this.mCurrentBrowser.setAttribute("autoscrollpopup", this._autoScrollPopup.id);

This one can be moved as well:
>          this.mCurrentBrowser.droppedLinkHandler = handleDroppedLink;

> > >       <constructor>
> > >         <![CDATA[
> > >-          this.mCurrentBrowser = this.mPanelContainer.childNodes[0].firstChild.firstChild;
> > >+          let browser = this._createBrowserElement();
> > >+          browser.setAttribute("type", "content-primary");
> > >+          browser.setAttribute("disablehistory", "true");
> > 
> > Does this run much earlier than prepareForStartup? If not, setting
> > disablehistory here seems pointless. The attribute was in the markup "so
> > that we don't have to load global history before bringing up a window".
> 
> Yeah, it runs earlier. (don't know how to tell "much earlier"), but the
> constructors run before BrowserStartup, in this order: tabbrowser, browser,
> tabbrowser-tabs. If I remove the line setting the attribute there's a
> sessionstore test that fails (browser_394759_basic.js)

Do you understand the failure? It's not rare that tests are fragile and just need to be fixed... or even that the underlying code is wrong, which we should at least file a bug on.
Attachment #569555 - Flags: review?(gavin.sharp)

Updated

7 years ago
Attachment #569555 - Flags: review?(dao)
Created attachment 737672 [details] [diff] [review]
rebased patch

I rebased Felipe's patch. I fixed the handleDroppedLink thing.

Moving autoScrollPopup seems a bit complicated, since this._autoScrollPopup is created dynamically using a method on the <browser> element. I could move the code to do this inside _createBrowserElement, but then we'd have to pass an argument telling it whether it's the first one (so that it can create the _autoScrollPopup). I'm not sure this is worth it, so I left the code as-is.

I also tried removing the disablehistory attribute. However, that triggered some NS_ASSERTION failures in the session restore code. I'll file a bug about it.
Attachment #569555 - Attachment is obsolete: true
Attachment #737672 - Flags: review?(gavin.sharp)
Whiteboard: [e10s]
Blocks: 653064
(Reporter)

Comment 6

5 years ago
Gavin and I talked about a suggestion to make these changes have less impact in the current single-process case. Instead of removing the starting browser from the XUL structure, can we leave it there and then, only for the browser.tabs.remote=true case, we do a removeChild on it and create a new one with remote="true"?

I don't recall if I tried this in the past and it didn't work for some reason, or if I never tried it.
Created attachment 742036 [details] [diff] [review]
alternate patch

This implements the alternative Felipe and I discussed. While it does isolate the behavior change to the case of tabs.remote=true, it also makes that case somewhat more complicated (because there is a browser there initially that is then replaced), which itself carries some risk. So maybe we should avoid this unless we find out there is a need for it.
Created attachment 742044 [details] [diff] [review]
original patch, rebased again

Let's just go with this for now. In reviewing this stuff I noticed a few things:
- what the heck is the "message" attribute for?
- we should probably factor out the tabprogresslistener addition from these two code paths into a helper
- _autoscrollPopup and _fastFind handling seems like it could be simplified

but none of those are critical issues and we can followup as needed.
Attachment #737672 - Attachment is obsolete: true
Attachment #737672 - Flags: review?(gavin.sharp)
Attachment #742044 - Flags: review+
Destroying and recreating the browser may or may not have the effect of forgetting whether a popup window should have scrollbars or not.
https://hg.mozilla.org/mozilla-central/rev/714e20f9b4e5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
FWIW, this has broken several extensions. Confirmed are MEGA EXTENSION [1], GMail Manager [2] and Browser Backgrounds [3]. When these extensions are enabled, it becomes impossible to open or close new tabs (and the error console), making the browser essentially unusable. I'm told that this is the result of bad programming in the extensions and that they could be easily fixed, but I figure it's at least worth pointing out here for posterity.

[1] https://addons.mozilla.org/en-US/firefox/addon/meganz/
[2] https://addons.mozilla.org/en-US/firefox/addon/gmail-manager/
[3] https://addons.mozilla.org/en-US/firefox/addon/browser-backgrounds/
Emanuel, thanks for the heads up. Would you mind filing a new Firefox::Extension compatibility bug with exactly that comment's contents, and marking it as "blocking" this one?
Depends on: 866537
Done: I filed bug 866537.
(Reporter)

Comment 15

5 years ago
I backed out this until we better understand what change in behavior this patch caused to break these add-ons and the browser together. From the mozillazine thread [1] it looks like there was some change in the firing of DOMContentLoaded listeners attached to gBrowser, so it's worth understanding it as it could cause some of our features to be broken too. 

http://hg.mozilla.org/integration/mozilla-inbound/rev/3b3d66182d26

Apologies if the back out is unnecessarily but we can always land it again. Since with at least some add-ons it causes the browser to be unusable it might be too much even for Nightly.

We could try comment 6 to move ahead with the needs for this bug

[1] http://forums.mozillazine.org/viewtopic.php?p=12826625#p12826625
Assignee: felipc → wmccloskey
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [e10s] → [e10s][leave open]
I got some help from bz, and now I think I understand why this broke. The core of the problem is that, when calling browser._createAutoScrollPopup(), which is a method defined in XBL, we're trying to use an XBL binding before it's been attached to the dynamically-created <browser> element.

Here's a more detailed description. With the patch applied and no addons (the working case), the browser initialization process looks something like this:

- While doing layout for the main chrome window, we also attach XBL bindings for any elements involved. This includes the <tabbrowser> element.
- After layout is done, the constructor for <tabbrowser> runs.
- It creates a <browser> element.
- Then it appends the <browser> element as a child of the browserStack. The appendChild call triggers more layout, which causes an XBL binding to get attached to the <browser> element.
- Later on, we call the _createAutoScrollPopup() method on the newly created <browser>. That method is defined in the XBL that was just attached, so the call succeeds.

For comparison, here's what happens with the addon installed (the broken case).

- The addon has a browser.xul overlay that includes a <script> tag. The script immediately accesses gBrowser.
- The getter for gBrowser causes us to do getElementById("content") to get the <tabbrowser>. When we do that, we force the XBL binding for the tabbrowser to be attached and its constructor run.
- The <tabbrowser> constructor creates a new <browser> and appends it to the browserStack. We don't do any layout in this case because the PresShell hasn't been initialized yet--we're too early in the loading process.
- A little later, we try to run the _createAutoScrollPopup() method on the newly created <browser>. The XBL bindings for <browser> haven't been attached yet, so this fails.
- Consequently, the _autoScrollPopup property of the <tabbrowser> is never set. It gets used all over the place (like when creating new tabs), so all these places now fail.

To condense this a little more, the problem is that we're trying to use an XBL binding before it's been attached to the dynamically-created <browser> element. We normally would attach during layout, but the addon forces the <tabbrowser> constructor to run so early that layout can't happen yet.

For background, these are the situations in which an XBL binding gets attached to an element (from bz over irc):

1) cloneNode on a node with a binding
2) frame is constructed
3) The node is in a document when it's first js-wrapped

Case (1) isn't relevant here because there's no cloning involved. Case (2) usually happens during layout. Case (3) doesn't happen here because the JS wrapper for the <browser> is created right away, before we add the element to the document.

Boris's suggestion is to use innerHTML to add the browser element. That way it would be in the document before the JS wrapper is created, so case (3) would apply. I'm trying to get this to work, but there's something weird going on with the namespaces. Hopefully I'll have something tomorrow.

Also, the idea for removing the initial browser and then re-creating it wouldn't work. The new browser still wouldn't have its XBL binding attached, so we'd have the same problem.
Good analysis! Accesses of gBrowser too early have been a frequent cause of similar trouble in the past (see e.g. bug 294815).

Not really something to dive into for this bug, but I wonder if we can fix the more general problem by forcing binding attachment at time of document insertion for js-wrapped elements, somehow?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17)
> Good analysis! Accesses of gBrowser too early have been a frequent cause of
> similar trouble in the past (see e.g. bug 294815).

Also bug 463384. Look for "The binding was constructed too early" in the patch. The same hack might be applicable here.
Created attachment 743767 [details] [diff] [review]
simple patch

I couldn't get the innerHTML thing to work, and it was really gross anyway. This patch implements Gavin and Felipe's alternate proposal, although a little more simply. It still has the problem that addons that use gBrowser too soon will break if the e10s pref is set, but I guess we can deal with that later.

I looked at bug 463384, but I didn't understand what the hack was that made it work.
Attachment #742044 - Attachment is obsolete: true
Attachment #743767 - Flags: review?(gavin.sharp)
(In reply to Bill McCloskey (:billm) from comment #19)
> I looked at bug 463384, but I didn't understand what the hack was that made
> it work.

It added an init method that's called from the tabbrowser constructor as well as from browser.js in case it failed the first time due to the constructor running too early.
Comment on attachment 743767 [details] [diff] [review]
simple patch

Since this behavior is only needed while we support both modes, and this patch only impacts the non-default e10s-enabled case, this seems like a reasonable approach. I worry about side effects to removing/re-adding the browser like the one Neil mentions, but since this is a temporary measure we can deal with issues as they come up.
Attachment #743767 - Flags: review?(gavin.sharp) → review+
Keywords: addon-compat
https://hg.mozilla.org/mozilla-central/rev/803504735035
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.