Closed Bug 1360940 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: u462496, Assigned: u462496)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: lazytabs
browser.messageManager access was added in bug 1336763.
Blocks: 1336763
Attached patch 1360940_patch_V1.diff (obsolete) — Splinter Review
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.
Attachment #8863280 - Flags: feedback?(mconley)
Attachment #8863280 - Flags: feedback?(dao+bmo)
Comment on attachment 8863280 [details] [diff] [review]
1360940_patch_V1.diff

I think doing "if (!browser.isConnected) { continue; }" early would be clearer.
Attachment #8863280 - Flags: feedback?(dao+bmo) → feedback+
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.
Attachment #8863280 - Flags: feedback?(mconley) → feedback+
:-/
Attached patch 1360940_patch_V2.diff (obsolete) — Splinter Review
Attachment #8863280 - Attachment is obsolete: true
(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
Attachment #8863281 - Flags: review?(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;
}
```
Attachment #8863281 - Flags: review?(mconley) → review-
Assignee: nobody → kevinhowjones
Attachment #8863281 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8863282 - Flags: review?(mconley)
Comment on attachment 8863282 [details] [diff] [review]
1360940_patch_V3.diff

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

Thanks!
Attachment #8863282 - Flags: review?(mconley) → review+
Component: Untriaged → Tabbed Browser
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcc235d3a5d
Don't instantiate lazy browsers when checking if the window can close. r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9bcc235d3a5d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: