Closed Bug 1364847 Opened 5 years ago Closed 4 years ago

WebExtension based add-ons should never mass insert browser elements for lazy browsers.

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: gkrizsanits, Unassigned, Mentored)

References

(Depends on 1 open bug)

Details

(Whiteboard: triaged)

See bug 1363240. Currently after a session restore all the bg tabs have only a stub for the browser element, called lazy browsers. Normally the first time they get selected we insert a real browser element and possibly start up a content process. But if an add-on requests certain operations on these lazy browsers before that (messageManager getter for example), we might end up having to insert them earlier, which if happens in a tight loop for hundreds of tabs can cause performance issues. Even worse, if it's combined with closing tabs right after insertion we might end up spamming short lived content processes.

Lazy browsers is not currently on release, the target is 55 but there is a chance that it will be pushed back until 57.
Blocks: 1363240
Hi Gabor, can you please provide/confirm a few details?

 - Does this happen without any extensions enabled?
 - Or only with specific extensions (that use a specific api?)?  If so, example extension?
 - Other than accessing tab.linkedBrowser.messageManager, anything else we need to avoid?
 - Is there a flag/property we can safely read to confirm the tab is still in lazy state?
(In reply to Tomislav Jovanovic :zombie from comment #1)
>  - Is there a flag/property we can safely read to confirm the tab is still
> in lazy state?

tab.linkedPanel or tab.linkedBrowser.isConnected
(In reply to Kevin Jones from comment #2)
> (In reply to Tomislav Jovanovic :zombie from comment #1)
> >  - Is there a flag/property we can safely read to confirm the tab is still
> > in lazy state?
> 
> tab.linkedPanel or tab.linkedBrowser.isConnected

That is:

If lazy:

tab.linkedPanel == false
tab.linkedBrowser.isConnected == false
(In reply to Tomislav Jovanovic :zombie from comment #1)
> Hi Gabor, can you please provide/confirm a few details?
> 
>  - Does this happen without any extensions enabled?

No.

>  - Or only with specific extensions (that use a specific api?)?  If so,
> example extension?

See original example in: bug 1363240

>  - Other than accessing tab.linkedBrowser.messageManager, anything else we
> need to avoid?

I think a good starting point is this switch here: http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/browser/base/content/tabbrowser.xml#2140

Anything that falls in the linked default case is a problem.

I think Kris has an idea/plan about how to fix this based on his comment in the original bug, so you might want to talk to him if you're planning to work on this.
I was planning on working on this soon. Currently, most of our code that deals with tab browsers is abstracted, and handled in a couple of places:

http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/toolkit/components/extensions/ExtensionTabs.jsm#51-511
http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/browser/components/extensions/ext-utils.js#476-591

So we should just need to change those to avoid triggering the insertion of lazy browsers.

It would also be nice to get some test helpers in place so we can't trigger it by accident, or at least without an explicit exception.
this below blockers... so working out timing
Flags: needinfo?(amckay)
Whiteboard: triaged
I'll happily review patches for this in the meantime.
Mentor: kmaglione+bmo
Component: WebExtensions: Untriaged → WebExtensions: General
Setting to a P2 but deliberately leaving unassigned for the moment. There's a bunch of other P1s we need to get at first, although I know this is a concern for e10s multi.
Flags: needinfo?(amckay)
Priority: -- → P1
Priority: P1 → P2
Duplicate of this bug: 1386041
Depends on: 1377734
I haven't been able to get any of the tab apis to trigger this.  console logging exists in the case where it would happen so we should hear about it if it does.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
Product: Toolkit → WebExtensions
Depends on: 1770651
You need to log in before you can comment on or make changes to this bug.