1342723 - Modify removeTab to handle lazy-browser tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[u462496]

Support bug for bug 906076

Currently, lazy browsers for tabs removed by removeTab are simply inserted into the document to prevent breakage, causing unnecessary resource drain.  This bug is to modify removeTab so lazy browsers will be kept lazy until their removal.

Comment 1

[u462496]

Attachment: 1342723_patch_V1.diff

Experimental

Comment 2

[u462496]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94ca1b52381dc91b956a218deb19a4519023a58d

Comment 3

[u462496]

Attachment: 1342723_patch_V2.diff

Comment 4

[u462496]

Attachment: 1342723_patch_V4.diff

Dao, can you take a look at this and see what you think?

I haven't seen any test failures which I can duplicate.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87f38b8ec790bd6c9805d1acd2535ebea6541bb&selectedJob=80861799

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8843140 [details] [diff] [review]
1342723_patch_V4.diff

>@@ -2635,17 +2632,18 @@
> 
>             var browser = this.getBrowserForTab(aTab);
> 
>             if (!aTab._pendingPermitUnload && !aAdoptedByTab && !aSkipPermitUnload) {
>               // We need to block while calling permitUnload() because it
>               // processes the event queue and may lead to another removeTab()
>               // call before permitUnload() returns.
>               aTab._pendingPermitUnload = true;
>-              let {permitUnload, timedOut} = browser.permitUnload();
>+              let {permitUnload, timedOut} = aTab.linkedPanel ?
>+                  browser.permitUnload() : {permitUnload: true, timedOut: false};

I'd prefer:

let permitUnload = true;
let timedOut = false;
if (aTab.linkedPanel) {
  {permitUnload, timedOut} = browser.permitUnload();
}

>@@ -2777,25 +2777,27 @@
> 
>             // We're going to remove the tab and the browser now.
>             this._tabFilters.delete(aTab);
>             this._tabListeners.delete(aTab);
> 
>             var browser = this.getBrowserForTab(aTab);
>             this._outerWindowIDBrowserMap.delete(browser.outerWindowID);

_outerWindowIDBrowserMap should only contain inserted browsers.

>@@ -2842,17 +2844,17 @@
>             // Release the browser in case something is erroneously holding a
>             // reference to the tab after its removal.
>             this._tabForBrowser.delete(aTab.linkedBrowser);
>             aTab.linkedBrowser = null;
> 
>             // As the browser is removed, the removal of a dependent document can
>             // cause the whole window to close. So at this point, it's possible
>             // that the binding is destructed.
>-            if (this.mTabBox) {
>+            if (panel.parentNode && this.mTabBox) {
>               this.mPanelContainer.removeChild(panel);
>             }

Does 'panel' even exist if the browser wasn't inserted?

>--- a/browser/base/content/test/general/browser_zbug569342.js
>+++ b/browser/base/content/test/general/browser_zbug569342.js
>@@ -1,16 +1,19 @@
> /* Any copyright is dedicated to the Public Domain.
>  * http://creativecommons.org/publicdomain/zero/1.0/
>  */
> 
> var gTab = null;
> 
> function load(url, cb) {
>   gTab = gBrowser.addTab(url);
>+  // Ensure browser has been inserted into the DOM.
>+  gBrowser._insertBrowser(gTab);

Why is this needed here?

>--- a/browser/components/sessionstore/TabState.jsm
>+++ b/browser/components/sessionstore/TabState.jsm
>@@ -92,17 +92,17 @@ var TabStateInternal = {
>     let browser = tab.linkedBrowser;
> 
>     if (tab.pinned) {
>       tabData.pinned = true;
>     }
> 
>     tabData.hidden = tab.hidden;
> 
>-    if (browser.audioMuted) {
>+    if (tab.linkedPanel && browser.audioMuted) {
>       tabData.muted = true;
>       tabData.muteReason = tab.muteReason;
>     }

Can we just remove audioMuted from _browserBindingProperties?

Comment 6

[u462496]

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8843140 [details] [diff] [review]
> 1342723_patch_V4.diff
> I'd prefer:
> 
> let permitUnload = true;
> let timedOut = false;
> if (aTab.linkedPanel) {
>   {permitUnload, timedOut} = browser.permitUnload();
> }

Sure.

> >@@ -2777,25 +2777,27 @@
> > 
> >             // We're going to remove the tab and the browser now.
> >             this._tabFilters.delete(aTab);
> >             this._tabListeners.delete(aTab);
> > 
> >             var browser = this.getBrowserForTab(aTab);
> >             this._outerWindowIDBrowserMap.delete(browser.outerWindowID);
> 
> _outerWindowIDBrowserMap should only contain inserted browsers.

Okay.

> >@@ -2842,17 +2844,17 @@
> >             // Release the browser in case something is erroneously holding a
> >             // reference to the tab after its removal.
> >             this._tabForBrowser.delete(aTab.linkedBrowser);
> >             aTab.linkedBrowser = null;
> > 
> >             // As the browser is removed, the removal of a dependent document can
> >             // cause the whole window to close. So at this point, it's possible
> >             // that the binding is destructed.
> >-            if (this.mTabBox) {
> >+            if (panel.parentNode && this.mTabBox) {
> >               this.mPanelContainer.removeChild(panel);
> >             }
> 
> Does 'panel' even exist if the browser wasn't inserted?

Yes it does.  The entire panel >> browser branch is created in _createBrowser.  Everything is inserted at once when the panel is inserted in _insertBrowser.  (Which now makes me wonder if there is a more appropriate name than _insertBrowser)  

> >--- a/browser/base/content/test/general/browser_zbug569342.js
> >+++ b/browser/base/content/test/general/browser_zbug569342.js
> >@@ -1,16 +1,19 @@
> > /* Any copyright is dedicated to the Public Domain.
> >  * http://creativecommons.org/publicdomain/zero/1.0/
> >  */
> > 
> > var gTab = null;
> > 
> > function load(url, cb) {
> >   gTab = gBrowser.addTab(url);
> >+  // Ensure browser has been inserted into the DOM.
> >+  gBrowser._insertBrowser(gTab);
> 
> Why is this needed here?

There is a case in that test when an about:blank tab is created, thus having lazy-browser, and thus does not get an onload event fired on it. 

> >--- a/browser/components/sessionstore/TabState.jsm
> >+++ b/browser/components/sessionstore/TabState.jsm
> >@@ -92,17 +92,17 @@ var TabStateInternal = {
> >     let browser = tab.linkedBrowser;
> > 
> >     if (tab.pinned) {
> >       tabData.pinned = true;
> >     }
> > 
> >     tabData.hidden = tab.hidden;
> > 
> >-    if (browser.audioMuted) {
> >+    if (tab.linkedPanel && browser.audioMuted) {
> >       tabData.muted = true;
> >       tabData.muteReason = tab.muteReason;
> >     }
> 
> Can we just remove audioMuted from _browserBindingProperties?

Sounds reasonable.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Kevin Jones from comment #6)
> > >@@ -2842,17 +2844,17 @@
> > >             // Release the browser in case something is erroneously holding a
> > >             // reference to the tab after its removal.
> > >             this._tabForBrowser.delete(aTab.linkedBrowser);
> > >             aTab.linkedBrowser = null;
> > > 
> > >             // As the browser is removed, the removal of a dependent document can
> > >             // cause the whole window to close. So at this point, it's possible
> > >             // that the binding is destructed.
> > >-            if (this.mTabBox) {
> > >+            if (panel.parentNode && this.mTabBox) {
> > >               this.mPanelContainer.removeChild(panel);
> > >             }
> > 
> > Does 'panel' even exist if the browser wasn't inserted?
> 
> Yes it does.  The entire panel >> browser branch is created in
> _createBrowser.  Everything is inserted at once when the panel is inserted
> in _insertBrowser.  (Which now makes me wonder if there is a more
> appropriate name than _insertBrowser)  

I think you can just call panel.remove() then without checking panel.parentNode.

> > >--- a/browser/base/content/test/general/browser_zbug569342.js
> > >+++ b/browser/base/content/test/general/browser_zbug569342.js
> > >@@ -1,16 +1,19 @@
> > > /* Any copyright is dedicated to the Public Domain.
> > >  * http://creativecommons.org/publicdomain/zero/1.0/
> > >  */
> > > 
> > > var gTab = null;
> > > 
> > > function load(url, cb) {
> > >   gTab = gBrowser.addTab(url);
> > >+  // Ensure browser has been inserted into the DOM.
> > >+  gBrowser._insertBrowser(gTab);
> > 
> > Why is this needed here?
> 
> There is a case in that test when an about:blank tab is created, thus having
> lazy-browser, and thus does not get an onload event fired on it. 

Sounds like we should reconsider bug 1287330 comment 223.

Comment 8

[u462496]

(In reply to Dão Gottwald [:dao] from comment #7)
> Sounds like we should reconsider bug 1287330 comment 223.

Yes, I see what you mean.

Comment 9

[u462496]

Attachment: 1342723_patch_V5.diff

This will depend on bug 1344212

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8843304 [details] [diff] [review]
1342723_patch_V5.diff

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -2055,18 +2055,17 @@
>         "canGoBack", "canGoForward", "goBack", "goForward", "permitUnload",
>         "reload", "reloadWithFlags", "stop", "loadURI", "loadURIWithFlags",
>         "goHome", "homePage", "gotoIndex", "currentURI", "documentURI",
>         "preferences", "imageDocument", "isRemoteBrowser", "messageManager",
>         "getTabBrowser", "finder", "fastFind", "sessionHistory", "contentTitle",
>         "characterSet", "fullZoom", "textZoom", "webProgress",
>         "addProgressListener", "removeProgressListener", "audioPlaybackStarted",
>         "audioPlaybackStopped", "adjustPriority", "pauseMedia", "stopMedia",
>-        "blockMedia", "resumeMedia", "audioMuted", "mute", "unmute",
>-        "blockedPopups", "mIconURL", "lastURI", "userTypedValue",
>+        "blockMedia", "resumeMedia", "mute", "unmute", "blockedPopups", "lastURI",
>         "purgeSessionHistory", "stopScroll", "startScroll"
>       ]</field>

What does the removal of mIconURL and userTypedValue have to do with this bug?

>             // As the browser is removed, the removal of a dependent document can
>             // cause the whole window to close. So at this point, it's possible
>             // that the binding is destructed.
>-            if (this.mTabBox) {
>+            if (panel.parentNode && this.mTabBox) {
>               this.mPanelContainer.removeChild(panel);
>             }

As mentioned before, you can just change this to panel.remove(). That should work unconditionally.

>--- a/browser/components/sessionstore/TabState.jsm
>+++ b/browser/components/sessionstore/TabState.jsm
>@@ -92,17 +92,17 @@ var TabStateInternal = {
>     let browser = tab.linkedBrowser;
> 
>     if (tab.pinned) {
>       tabData.pinned = true;
>     }
> 
>     tabData.hidden = tab.hidden;
> 
>-    if (browser.audioMuted) {
>+    if (tab.linkedPanel && browser.audioMuted) {
>       tabData.muted = true;
>       tabData.muteReason = tab.muteReason;
>     }

This change shouldn't be necessary anymore.

Comment 11

[u462496]

(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 8843304 [details] [diff] [review]
> What does the removal of mIconURL and userTypedValue have to do with this
> bug?

Like browser.audioMuted: `RemoveTab` listener in SessionStore runs _collectBaseTabData which accesses those properties, causing the browser to be unnecessarily inserted.

> As mentioned before, you can just change this to panel.remove(). That should
> work unconditionally.
> 
> ....
> 
> >-    if (browser.audioMuted) {
> >+    if (tab.linkedPanel && browser.audioMuted) {
> >       tabData.muted = true;
> >       tabData.muteReason = tab.muteReason;
> >     }
> 
> This change shouldn't be necessary anymore.

Ooops, new patch coming up.

Comment 12

[u462496]

Attachment: 1342723_patch_V6.diff

Comment 13

[u462496]

eslint doesn't like the following change:

-              let {permitUnload, timedOut} = browser.permitUnload();
+              let permitUnload = true;
+              let timedOut = false;
+              if (aTab.linkedPanel) {
+                {permitUnload, timedOut} = browser.permitUnload();
+              }

It complains about the "=" in `{permitUnload, timedOut} = browser.permitUnload();`

Do you know why that would be?

Comment 14

[u462496]

(In reply to Kevin Jones from comment #13)
> eslint doesn't like the following change:
> 
> -              let {permitUnload, timedOut} = browser.permitUnload();
> +              let permitUnload = true;
> +              let timedOut = false;
> +              if (aTab.linkedPanel) {
> +                {permitUnload, timedOut} = browser.permitUnload();
> +              }
> 
> It complains about the "=" in `{permitUnload, timedOut} =
> browser.permitUnload();`
> 
> Do you know why that would be?

Actually it breaks the whole darn thing.  Weird, I am pretty sure I successfully tested with that change before updating to latest mozilla-central this morning.  My original change (the one you didn't like) works though.

Comment 15

[u462496]

Attachment: 1342723_patch_P2_V2.diff

I'm uploading a revised patch (see comment 14) in case you want to go ahead and review with my original proposal.

Comment 16

[u462496]

Comment on attachment 8844026 [details] [diff] [review]
1342723_patch_P2_V2.diff

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc9d1569f60673a020f2f0dfbe992be1e5be75d&selectedJob=81943980

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8844026 [details] [diff] [review]
1342723_patch_P2_V2.diff

Is this ready to land?

Comment 18

[u462496]

Yes.  I'm sorry, were you waiting for a review? ?

Comment 19

[Dão Gottwald [:dao]]

You requested feedback and had nothing to add, so I gave feedback+. I was waiting for you to either request review or make further changes to the patch.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12e20ccfb95
Make removeTab not insert lazy browsers. r=dao

Comment 21

[u462496]

(In reply to Dão Gottwald [::dao] from comment #19)
> You requested feedback and had nothing to add, so I gave feedback+. I was
> waiting for you to either request review or make further changes to the
> patch.

Okay, thanks.  Still learning the ropes...

Comment 22

[Dão Gottwald [:dao]]

(In reply to Kevin Jones from comment #21)
> (In reply to Dão Gottwald [::dao] from comment #19)
> > You requested feedback and had nothing to add, so I gave feedback+. I was
> > waiting for you to either request review or make further changes to the
> > patch.
> 
> Okay, thanks.  Still learning the ropes...

Oops, I meant *I* had nothing to add. In other words, if you request feedback and get feedback+, the ball is back in your court.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b12e20ccfb95