Closed Bug 1345098 Opened 3 years ago Closed 3 years ago

Lazy-browser-tabs: Deal with code which would unnecessarily/prematurely bind lazy-browsers

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: u462496, Assigned: u462496)

References

Details

Attachments

(1 file, 1 obsolete file)

Support bug for bug 906076.

Tabbrowser tabs' lazy-browsers have a built-in mechanism that will automatically insert the the browser into the document when certain properties are accessed (_browserBindingProperties).  There are call sites out there which could bind tabs' lazy-browsers unnecessarily.  One example of many is when the user tries to open about:preferences or about:addons from the UI, all tabs are polled for tab.linkedBrowser.currentURI in order to see see if there is a tab already hosting that page.  This could potentially insert all browsers at once before they are needed.

This bug is for identifying the scenarios which would unnecessarily bind lazy-browsers and deal with them.

This bug will be closely related to bug 1345090, in that SessionStore or TabState data may be supplied in proxy of some of the browser binding properties which will satisfy the caller's needs and keep the browser lazy.  In other cases we may have to modify caller code explicitly to take another route if the browser is lazy.
Blocks: lazytabs
Duplicate of this bug: 1289370
When devtools > memory is activated, a frame script is injected into every browser.  Currently this action will force all lazy browsers to be inserted removing them from their lazy state.  Do we want to pass over tabs with lazy browsers?  Or do all frame scripts need to be loaded at once, ie, can they be loaded lazily as well?  I am not familiar with the devtools APIs.

A downside to letting lazy browsers be inserted in a situation like this is that if there is a high volume of tabs in the lazy state, the user will experience unexpected choking while they are all being inserted.

Dao, can you or anyone else comment on this, and/or recommend a pier who can advise on this?
Flags: needinfo?(dao+bmo)
(In reply to Kevin Jones from comment #2)
> When devtools > memory is activated, a frame script is injected into every
> browser.  Currently this action will force all lazy browsers to be inserted
> removing them from their lazy state.  Do we want to pass over tabs with lazy
> browsers?  Or do all frame scripts need to be loaded at once, ie, can they
> be loaded lazily as well?  I am not familiar with the devtools APIs.

As an engineer on the DevTools team, I am happy to help modify DevTools as needed to special case lazy tabs so we don't trigger them to load in the specific case of opening DevTools.

However, there might be other places outside of DevTools where a similar thing happens...?  Not sure if you need a general solution for anyone who might try this, or if we can special case for DevTools and call that "good enough".
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> 
> As an engineer on the DevTools team, I am happy to help modify DevTools as
> needed to special case lazy tabs so we don't trigger them to load in the
> specific case of opening DevTools.

That would be great, help in relieving the load from someone familiar with the territory would be much appreciated.

> However, there might be other places outside of DevTools where a similar
> thing happens...?  Not sure if you need a general solution for anyone who
> might try this, or if we can special case for DevTools and call that "good
> enough".

There are other places indeed.  In some cases the "general solution" has been to return proxy values where appropriate for properties called on linkedBrowser which would otherwise instantiate the browser.  Examples are `currentURI`, `isRemoteBrowser`, `audioMuted` and `permitUnload` so far.

In the case mentioned here where `browser.messageManager` is accessed (which triggers browser insertion), I don't think there is a way to spoof that, and cases such as these need to be handled on a case-by-case basis.  However if I am missing something and you see a way to supply a proxy value to handle this, I'm all ears.

I note here also, that closing the devtools pane causes the same result (which begs the question, why are we injecting frame scripts because we're closing devtools?).  It actually triggers the same code (resource://devtools/server/main.js:1027:9).

Applying the latest patch in bug 1345090 (https://bug1345090.bmoattachments.org/attachment.cgi?id=8852675) should be enough to restore to a group of lazy-browser tabs, and reproduce the issues mentioned.  There is a diagnostic dump embedded in the patch which will alert when a browser has been inserted (and if you uncomment the line just below it, you can get a stack trace).
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> As an engineer on the DevTools team, I am happy to help modify DevTools as
> needed to special case lazy tabs so we don't trigger them to load in the
> specific case of opening DevTools...

Opened bug 1352157
Depends on: 1352183
Gonna reply in bug 1352157.
Flags: needinfo?(dao+bmo)
Kevin, do you still have a plan for this bug?

> This bug is for identifying the scenarios which would unnecessarily bind
> lazy-browsers and deal with them.

Would it help if we landed some diagnostic code that would log to the console when and why a browser was inserted so that Nightly users could file bugs on it?
(In reply to Dão Gottwald [::dao] from comment #7)
> Would it help if we landed some diagnostic code that would log to the
> console when and why a browser was inserted so that Nightly users could file
> bugs on it?

Yes, I had thought something like that would be good.  Do you have any suggestions or examples of this I could look at?  I have my own ideas but they would probably be pretty hacky.

On another note, do you think this bug should continue to block bug 906076?  (As well as other blocking bugs such as bug 1354596)  I'm wondering if bug 906076 should be closed, or considered a meta bug or?
(In reply to Kevin Jones from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > Would it help if we landed some diagnostic code that would log to the
> > console when and why a browser was inserted so that Nightly users could file
> > bugs on it?
> 
> Yes, I had thought something like that would be good.  Do you have any
> suggestions or examples of this I could look at?  I have my own ideas but
> they would probably be pretty hacky.

You could log 'new Error().stack' in _createLazyBrowser's fallback setter and getter.

> On another note, do you think this bug should continue to block bug 906076? 
> (As well as other blocking bugs such as bug 1354596)  I'm wondering if bug
> 906076 should be closed, or considered a meta bug or?

It's effectively been a meta bug for quite some time. :)
(In reply to Dão Gottwald [::dao] from comment #9)
> You could log 'new Error().stack' in _createLazyBrowser's fallback setter
> and getter.

Should that be displayed as error, warning, or info?
Info should be sufficient.
Attachment #8860452 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #7)
> Kevin, do you still have a plan for this bug?

Just to be clear, all examples mentioned in the bug description have been dealt with in other bugs.  At this point in time I am unaware of any code which will prematurely insert the lazy browser.
Comment on attachment 8860452 [details] [diff] [review]
log premature lazy-browser insertion

Just thinking, I wonder if we should include tab position and/or title in the message
Comment on attachment 8860452 [details] [diff] [review]
log premature lazy-browser insertion

>+                    let message =
>+                      `lazy-browser has been prematurely inserted by caller '${name}':\n`

nit: `[bug 1345098] Lazy browser prematurely inserted via '${name}' property access:\n`

Please put this behind an AppConstants.NIGHTLY_BUILD check.
Attachment #8860452 - Flags: review?(dao+bmo)
Attachment #8860452 - Attachment is obsolete: true
Attachment #8860701 - Flags: review?(dao+bmo)
Attachment #8860701 - Flags: review?(dao+bmo) → review+
Assignee: nobody → kevinhowjones
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04f9a535958e
Log console message for property access that triggers lazy browser insertion. r=dao
https://hg.mozilla.org/mozilla-central/rev/04f9a535958e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
If this isn't where this goes, please let me know.

The messages in the browser console referenced this bug.

I updated Nightly today and after opening there were three messages. Here they are:

12:05:28.209 [bug 1345098] Lazy browser prematurely inserted via 'messageManager' property access:
getter@chrome://browser/content/tabbrowser.xml:2126:45
getMessageManagerForRecipient@resource://gre/modules/ExtensionParent.jsm:227:7
receiveMessage@resource://gre/modules/ExtensionParent.jsm:193:22
_callHandlers/responses<@resource://gre/modules/MessageChannel.jsm:587:16
_callHandlers@resource://gre/modules/MessageChannel.jsm:585:21
_handleMessage/deferred.promise<@resource://gre/modules/MessageChannel.jsm:645:7
_handleMessage@resource://gre/modules/MessageChannel.jsm:642:24
receiveMessage@resource://gre/modules/MessageChannel.jsm:165:5
init@chrome://toggle-gifs/content/content.js?1494003916082:987:12
@chrome://toggle-gifs/content/content.js?1494003916082:1001:1
@chrome://toggle-gifs/content/content.js?1494003916082:6:2
 1 tabbrowser.xml:2126:23

=======

12:05:38.456 [bug 1345098] Lazy browser prematurely inserted via 'messageManager' property access:
getter@chrome://browser/content/tabbrowser.xml:2126:45
getMessageManagerForRecipient@resource://gre/modules/ExtensionParent.jsm:227:7
receiveMessage@resource://gre/modules/ExtensionParent.jsm:193:22
_callHandlers/responses<@resource://gre/modules/MessageChannel.jsm:587:16
_callHandlers@resource://gre/modules/MessageChannel.jsm:585:21
_handleMessage/deferred.promise<@resource://gre/modules/MessageChannel.jsm:645:7
_handleMessage@resource://gre/modules/MessageChannel.jsm:642:24
receiveMessage@resource://gre/modules/MessageChannel.jsm:165:5
init@chrome://toggle-gifs/content/content.js?1494003916082:987:12
@chrome://toggle-gifs/content/content.js?1494003916082:1001:1
@chrome://toggle-gifs/content/content.js?1494003916082:6:2
_createPreloadBrowser@chrome://browser/content/tabbrowser.xml:1914:13
_handleNewTab@chrome://browser/content/tabbrowser.xml:6471:11
addTab/<@chrome://browser/content/tabbrowser.xml:2332:17
waitForSyncCallback@resource://services-common/async.js:98:7
makeSpinningCallback/callback.wait@resource://services-common/async.js:168:27
promiseSpinningly@resource://services-common/async.js:234:12
observe@resource://services-sync/engines/bookmarks.js:979:11
observe@resource://services-common/observers.js:132:7
notify@resource://services-common/observers.js:89:5
onStartup@resource://services-sync/service.js:323:7
@resource://services-sync/service.js:1399:1
lazyImport/getter/<@resource://services-sync/main.js:19:7
ensureLoaded@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/Weave.js:80:5
notify@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/Weave.js:138:13
 1 tabbrowser.xml:2126:23

====

12:05:46.283 [bug 1345098] Lazy browser prematurely inserted via 'messageManager' property access:
getter@chrome://browser/content/tabbrowser.xml:2126:45
getMessageManagerForRecipient@resource://gre/modules/ExtensionParent.jsm:227:7
receiveMessage@resource://gre/modules/ExtensionParent.jsm:193:22
_callHandlers/responses<@resource://gre/modules/MessageChannel.jsm:587:16
_callHandlers@resource://gre/modules/MessageChannel.jsm:585:21
_handleMessage/deferred.promise<@resource://gre/modules/MessageChannel.jsm:645:7
_handleMessage@resource://gre/modules/MessageChannel.jsm:642:24
receiveMessage@resource://gre/modules/MessageChannel.jsm:165:5
waitForSyncCallback@resource://services-common/async.js:98:7
makeSpinningCallback/callback.wait@resource://services-common/async.js:168:27
onNotify@resource://services-sync/service.js:816:7
WrappedNotify@resource://services-sync/util.js:161:21
WrappedLock@resource://services-sync/util.js:117:16
WrappedCatch@resource://services-sync/util.js:92:16
login@resource://services-sync/service.js:832:12
sync/<@resource://services-sync/service.js:1055:14
WrappedCatch@resource://services-sync/util.js:92:16
sync@resource://services-sync/service.js:1051:5
waitForSyncCallback@resource://services-common/async.js:98:7
makeSpinningCallback/callback.wait@resource://services-common/async.js:168:27
promiseSpinningly@resource://services-common/async.js:234:12
observe@resource://services-sync/engines/bookmarks.js:979:11
observe@resource://services-common/observers.js:132:7
notify@resource://services-common/observers.js:89:5
onStartup@resource://services-sync/service.js:323:7
@resource://services-sync/service.js:1399:1
lazyImport/getter/<@resource://services-sync/main.js:19:7
ensureLoaded@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/Weave.js:80:5
notify@jar:file:///C:/Program%20Files/Nightly/omni.ja!/components/Weave.js:138:13
 1 tabbrowser.xml:2126:23
My startup is *far* from instant. I just checked the browser console and I'm still getting these. 

Actually, just the bottom two. Both seem to mention Weave.js and sync. So I'm guessing Sync is implicated in delazifying tabs.  (One of the entries indicates "214 repeats".)

Am I in the wrong place? Should I file another bug? Leave a comment elsewhere?
(In reply to Caspy7 from comment #20)
> My startup is *far* from instant. I just checked the browser console and I'm
> still getting these. 
> 
> Actually, just the bottom two. Both seem to mention Weave.js and sync. So
> I'm guessing Sync is implicated in delazifying tabs.  (One of the entries
> indicates "214 repeats".)
> 
> Am I in the wrong place? Should I file another bug? Leave a comment
> elsewhere?

You should file a new bug.
Depends on: 1368193
I have filed bug 1368193 and set it as a dependency.
(hope I did that right)
No longer depends on: 1368193
You need to log in before you can comment on or make changes to this bug.