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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: u462496, Assigned: u462496)

Tracking

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

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 906076
(Assignee)

Comment 1

2 years ago
browser.messageManager access was added in bug 1336763.
(Assignee)

Comment 2

2 years ago
Created attachment 8863280 [details] [diff] [review]
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.
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+
(Assignee)

Comment 5

2 years ago
:-/
(Assignee)

Comment 6

2 years ago
Created attachment 8863281 [details] [diff] [review]
1360940_patch_V2.diff
Attachment #8863280 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
(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
(Assignee)

Updated

2 years ago
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)

Comment 9

2 years ago
Created attachment 8863282 [details] [diff] [review]
1360940_patch_V3.diff
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+

Updated

2 years ago
Component: Untriaged → Tabbed Browser
Keywords: checkin-needed

Comment 11

2 years ago
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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9bcc235d3a5d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.