1630737 - Main-thread in the parent process blocked while viewing Amazon when using several recommended add-ons from AMO

Status: RESOLVED DUPLICATE of bug 1631933

(WebExtensions :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Originally reported here: https://www.reddit.com/r/firefox/comments/g2dett/firefox_75_slow_with_many_tabs_browser_issue_or/fnld9h7/

Here's a profile: https://perfht.ml/3a8E264

Here's the list of add-ons the user reports having:

    Privacy Badger
    ClearURLs
    FoxyTab
    Dark Background and Light Text
    Decentraleyes

Maybe related, see: https://gitlab.com/KevinRoebert/ClearUrls/-/issues/455

Specifically these STR:

Create new Firefox profile
Install the following addons (ClearUrls, Decentraleyes, Dark Background and Light Text, and Privacy Badger)
Hold Ctrl+T until 500+ new tabs are created. 
Visit Amazon.com & immediately try scrolling around, page will freeze/stutter for several seconds.
Disable/Remove any one of these four addons (ClearUrls, Decentraleyes, Privacy Badge, or Dark Background)
Visit/Refresh Amazon page again, scrolling will be smooth, no initial freeze/stutter.

and

Firefox option > General > Restore previous session
Exit Firefox
Relaunch Firefox to have all 500 tabs in suspended/unloaded state
Visit Amazon.com, page still freeze/stutter scrolling  for first few seconds.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Any idea what might be happening here, zombie?

Comment 2

[Andrew Swan [:aswan]]

From a quick glance, if I have about:newtab opened in the 500 tabs mentioned in comment 0, each of them is loading a screenshot as a very large data: url. Each of those ~500 loads is noticed by the nsIContentPolicy in WebRequestContent.js and sent to the parent process.
This sounds like maybe a duplicate of bug 1490932, though maybe we want to do some sort of special-case thing for about:newtab since we know that loads from there will never be visible to extensions? Maybe its as simple as not loading WebRequestContent.js in the privileged content process?

Comment 3

[bman541]

Issue occurs with any web page not just about:newtab. For testing purpose it was quick/easy to hold ctrl+t to mass tab generation.

Comment 4

[Tomislav Jovanovic :zombie]

Thanks Andrew.

It seems to me that all those new tabs should be in unloaded (discarded) state, so we either wake them up ourselves (ironically, in the name of perceived performance) or one of the extensions is doing it. I'll investigate further.

Comment 5

[Andrew Swan [:aswan]]

Okay, the thing with about:newtab is still an issue, I think we should consider disabling webrequest handling of data: urls from about:newtab (or about:newtab should switch to using blobs instead of big data: urls or something)

But the real story is more interesting: amazon.com loads about 500KB of css using data: urls. Which is a lot but shouldn't be a problem by itself. However, the logic for handling data: urls in the parent process loops over every webRequest listener: https://searchfox.org/mozilla-central/rev/567b68b8ff4b6d607ba34a6f1926873d21a7b4d7/toolkit/components/extensions/webrequest/WebRequest.jsm#317-328
The way the code is currently organized, the url is passed in as a string and parsed on each call. Why is that a problem? Because decentraleyes adds a new onBeforeRequest listener for every tab that exists!! (https://git.synz.io/Synzvato/decentraleyes/-/blob/master/core/state-manager.js#L138-159). It looks like it would be relatively easy to parse the url one time in shouldRunListener() and pass the url object down to the matcher to avoid parsing N times when there are N listeners.

Concrete steps that can be taken here are:

• parse data: urls one time as described above to avoid poor scaling behavior with many listeners
• reach out to decentraleyes author and suggest reorganizing to use a single listener with dispatching code in the extension rather than creating a listener per tab
• follow up on the newtab issues (which were apparently a red herring for this particular bug, but still unneeded overhead that we can get rid of)

Comment 6

[Tomislav Jovanovic :zombie]

Oh wow.

Let's break all that into actionable tasks:

• Filed bug 1630994 to reach out to Decentraleyes authors
• Andrew filed bug 1630999 for optimizing the new tab page
• I can take this one to handle the data: URI, and also see if we can maybe filter by tabId before we do url matching, since that should be much less expensive.

Comment 7

[Tomislav Jovanovic :zombie]

Also also, it looks like we only check channel.isSystemLoad only after we do filter and permissions matching:
https://searchfox.org/mozilla-central/rev/aec63591/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#599

Meaning, even if extensions can't see any of those data: URI favicons from the new tab page, we take a lot of time checking permissions. :(

Comment 8

[Andrew Swan [:aswan]]

(In reply to Tomislav Jovanovic :zombie from comment #7)

Also also, it looks like we only check channel.isSystemLoad only after we do filter and permissions matching:
https://searchfox.org/mozilla-central/rev/aec63591/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#599

Meaning, even if extensions can't see any of those data: URI favicons from the new tab page, we take a lot of time checking permissions. :(

That's a different code path -- that's inside ChannelWrapper which is used for http(s) loads. data: urls take a different path, they're handled by the ContentPolicyManager. This code is old and has evolved incrementally over time making it kind of hard to follow :/

Comment 10

[Shane Caraveo (:mixedpuppy)]

Rob is already removing data uri support from webrequest.

Comment 11

[Bernard Igiri]

Marking as P3 as this depends on bug 1630994 and bug 1630999, per Tomislav Jovanovic's comment and it's not a startup/shutdown performance issue.

Comment 12

[Andrew Swan [:aswan]]

Since bug 1631933 this is no longer relevant.