1352183 - Preserve lazy browsers' lazy state during shutdown

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[u462496]

Code runs during shutdown which access tabbrowser._browserBindingProperties on lazy browsers, thus causing the browsers to unnecessarily (and undesirably) be inserted into the document.

Comment 1

[u462496]

Attachment: patch V1

Modify code which would insert lazy browsers on shutdown.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8853092 [details] [diff] [review]
patch V1

>             let filter = this._tabFilters.get(tab);
>             let listener = this._tabListeners.get(tab);
> 
>-            browser.webProgress.removeProgressListener(filter);
>-            filter.removeProgressListener(listener);
>-            listener.destroy();
>+            if (filter) {
>+              browser.webProgress.removeProgressListener(filter);
>+            }
>+            if (listener) {
>+              filter.removeProgressListener(listener);
>+              listener.destroy();
>+            }

Please move the second if-block inside the first one since it depends on |filter| (although in reality filter should never be null when listener is non-null).

You could move |let listener = this._tabListeners.get(tab)|, |this._tabFilters.delete(tab)| and |this._tabListeners.delete(tab)| inside the if-block as well.

>   flush(browser) {
>+    let tabbrowser = browser.ownerGlobal.gBrowser;
>+    if (!tabbrowser.getTabForBrowser(browser).linkedPanel) {
>+      return;
>+    }

Same question as earlier in bug 1345090: Can you use browser.isConencted here?

Comment 3

[u462496]

Attachment: 1352183_patch_V2.diff

Made changes as requested.

Can you please tell me how browser.isConnected works?  It seems to work in the patch, but I observe that if the browser is inserted directly in addTab (createLazyBrowser == false), __sometimes__ browser.isConnected == true before the browser actually gets inserted.  While is __seems__ that if the browser is not inserted directly (createLazyBrowser == false) that browser.isConnected == false until it finally gets inserted, it still makes me wonder if this is a reliable test.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Kevin Jones from comment #3)
> Created attachment 8853374 [details] [diff] [review]
> 1352183_patch_V2.diff
> 
> Made changes as requested.
> 
> Can you please tell me how browser.isConnected works?

It _should_ mean that the node is in the document. I'm not aware of any special cases.

If it works unreliably, we might have to file a bug on that and avoid using it for now.

Does browser.ownerDocument.contains(browser) work better?

Comment 5

[u462496]

(In reply to Dão Gottwald [::dao] from comment #4)
> (In reply to Kevin Jones from comment #3)
> > Created attachment 8853374 [details] [diff] [review]
> > 1352183_patch_V2.diff
> > 
> > Made changes as requested.
> > 
> > Can you please tell me how browser.isConnected works?
> 
> It _should_ mean that the node is in the document. I'm not aware of any
> special cases.
> 
> If it works unreliably, we might have to file a bug on that and avoid using
> it for now.
> 
> Does browser.ownerDocument.contains(browser) work better?

browser.ownerDocument.contains(browser) just keeps returning false even after the browser is inserted.

Wouldn't it only return true if browser is a js property in the object browser.ownerDocument?  Does that mean it would also indicate that the browser has become a descendent in the DOM?  It doesn't seem that way to me, but maybe I'm not understanding the DOM correctly.

Comment 6

[u462496]

(In reply to Kevin Jones from comment #5)

> Wouldn't it only return true if browser is a js property in the object
> browser.ownerDocument?  Does that mean it would also indicate that the
> browser has become a descendent in the DOM?  It doesn't seem that way to me,
> but maybe I'm not understanding the DOM correctly.

Ugh!!! Never mind.

Don't understand why it is returning false though.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Kevin Jones from comment #6)
> Don't understand why it is returning false though.

Probably because our browsers are anonymous content.

Comment 8

[u462496]

Oh yes.

Should we just use the code I originally had for now then?

Comment 9

[Dão Gottwald [:dao]]

Yep, fine for now, but at some point we should try to figure out what's going on with isConnected.

Comment 10

[u462496]

Attachment: 1352183_patch_V3.diff

Comment 11

[Dão Gottwald [:dao]]

Should I land this?

Comment 12

[u462496]

(In reply to Dão Gottwald [::dao] from comment #11)
> Should I land this?

Please do.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/438624f92fbc
Preserve lazy browsers' lazy state when closing the window. r=dao

Comment 14

[Dão Gottwald [:dao]]

Backed out because of eslint failure:

https://treeherder.mozilla.org/logviewer.html#?job_id=87914012&repo=mozilla-inbound&lineNumber=4408

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8827728b7f48e593f4c58ccf5d39539071bf91

TabStateFlusher.flush needs to consistently return a promise.

Comment 15

[u462496]

(In reply to Dão Gottwald [::dao] from comment #14)
> Backed out because of eslint failure:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=87914012&repo=mozilla-
> inbound&lineNumber=4408
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 7c8827728b7f48e593f4c58ccf5d39539071bf91
> 
> TabStateFlusher.flush needs to consistently return a promise.

I'm going to have to re-think this.

Comment 16

[u462496]

Attachment: 1352183_patch_V4.diff

If this passes your scrutiny, I think you should be able to land it.

Comment 17

[u462496]

Attachment: 1352183_patch_V4_2.diff

Very sorry, please try this one...

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8853717 [details] [diff] [review]
1352183_patch_V4_2.diff

>   flushWindow(window) {
>-    let browsers = window.gBrowser.browsers;
>-    let promises = browsers.map((browser) => this.flush(browser));
>+    let promises = [];
>+    for (let browser of window.gBrowser.browsers) {
>+      let tabbrowser = browser.ownerGlobal.gBrowser;
>+      if (tabbrowser.getTabForBrowser(browser).linkedPanel) {

Just use window.gBrowser instead of tabbrowser.

Comment 19

[u462496]

Attachment: 1352183_patch_V5.diff

Comment 20

[u462496]

Attachment: 1352183_patch_V5.diff

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c155a02faeb
Preserve lazy browsers' lazy state when closing the window. r=dao

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c155a02faeb