Modify removeTab to handle lazy-browser tabs

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: u462496, Assigned: u462496)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8841289 [details] [diff] [review]
1342723_patch_V1.diff

Experimental
(Assignee)

Updated

a year ago
Blocks: 906076
(Assignee)

Updated

a year ago
Assignee: nobody → kevinhowjones
(Assignee)

Comment 3

a year ago
Created attachment 8841403 [details] [diff] [review]
1342723_patch_V2.diff
Attachment #8841289 - Attachment is obsolete: true
(Assignee)

Comment 4

a year ago
Created attachment 8843140 [details] [diff] [review]
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
Attachment #8841403 - Attachment is obsolete: true
Attachment #8843140 - Flags: feedback?(dao+bmo)
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?
Attachment #8843140 - Flags: feedback?(dao+bmo)
(Assignee)

Comment 6

a year ago
(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.
(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.
(Assignee)

Comment 8

a year ago
(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.
(Assignee)

Comment 9

a year ago
Created attachment 8843304 [details] [diff] [review]
1342723_patch_V5.diff

This will depend on bug 1344212
Attachment #8843140 - Attachment is obsolete: true
Attachment #8843304 - Flags: feedback?(dao+bmo)
(Assignee)

Updated

a year ago
Depends on: 1344212
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.
Attachment #8843304 - Flags: feedback?(dao+bmo)
(Assignee)

Comment 11

a year ago
(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.
(Assignee)

Comment 12

a year ago
Created attachment 8843400 [details] [diff] [review]
1342723_patch_V6.diff
Attachment #8843304 - Attachment is obsolete: true
Attachment #8843400 - Flags: feedback?(dao+bmo)
Attachment #8843400 - Flags: feedback?(dao+bmo) → feedback+
(Assignee)

Comment 13

a year ago
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?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 14

a year ago
(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.
(Assignee)

Comment 15

a year ago
Created attachment 8844026 [details] [diff] [review]
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.
Attachment #8843400 - Attachment is obsolete: true
Attachment #8844026 - Flags: feedback?(dao+bmo)
Flags: needinfo?(dao+bmo)
Attachment #8844026 - Flags: feedback?(dao+bmo) → feedback+
Comment on attachment 8844026 [details] [diff] [review]
1342723_patch_P2_V2.diff

Is this ready to land?
Attachment #8844026 - Flags: review+
(Assignee)

Comment 18

a year ago
Yes.  I'm sorry, were you waiting for a review? ?
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

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12e20ccfb95
Make removeTab not insert lazy browsers. r=dao
(Assignee)

Comment 21

a year ago
(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...
(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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b12e20ccfb95
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1349501
You need to log in before you can comment on or make changes to this bug.