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
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: