1360940 - browser.js/CanCloseWindow() causes all lazy browsers to be instantiated

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[u462496]

A recent change in browser.js/CanCloseWindow() accesses browser.messageManager while iterating over all browsers, thus causing all lazy browsers to be instantiated.

This change appeared some time since Nightly build 20170428030259.  I haven't been able to locate the bug as yet.

Comment 1

[u462496]

browser.messageManager access was added in bug 1336763.

Comment 2

[u462496]

Attachment: 1360940_patch_V1.diff

I believe lazy browsers can be ignored in this case, browser.permitUnload for lazy browsers will always return { permitUnload: true, timedOut: false }.

Feel free to review if it's a go.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8863280 [details] [diff] [review]
1360940_patch_V1.diff

I think doing "if (!browser.isConnected) { continue; }" early would be clearer.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8863280 [details] [diff] [review]
1360940_patch_V1.diff

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

Thanks for spotting this, Kevin.

I concur with dao - it'd be good to just get the !isConnected case out of the way before worrying about the pmm bits, so an early continue would make sense here.

Comment 5

[u462496]

:-/

Comment 6

[u462496]

Attachment: 1360940_patch_V2.diff

Comment 7

[u462496]

(In reply to Mike Conley (:mconley) - PTO on April 28th. from comment #4)
> Comment on attachment 8863280 [details] [diff] [review]
> 1360940_patch_V1.diff
> 
> Review of attachment 8863280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for spotting this, Kevin.
> 
> I concur with dao - it'd be good to just get the !isConnected case out of
> the way before worrying about the pmm bits, so an early continue would make
> sense here.

You can thank Avada:

https://bugzilla.mozilla.org/show_bug.cgi?id=906076#c249

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8863281 [details] [diff] [review]
1360940_patch_V2.diff

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

Looks great! Just a style nit.

::: browser/base/content/browser.js
@@ +6429,5 @@
>    let timedOutProcesses = new WeakSet();
>  
>    for (let browser of gBrowser.browsers) {
> +    // Don't instantiate lazy browsers.
> +    if (!browser.isConnected) { continue; }

This looks great - but let's break this up, like:

```C++
if (!browser.isConnected) {
  continue;
}
```

Comment 9

[u462496]

Attachment: 1360940_patch_V3.diff

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8863282 [details] [diff] [review]
1360940_patch_V3.diff

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

Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcc235d3a5d
Don't instantiate lazy browsers when checking if the window can close. r=mconley

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/9bcc235d3a5d