mozilla / contain-facebook

Facebook Container isolates your Facebook activity from the rest of your web activity in order to prevent Facebook from tracking you outside of the Facebook website via third party cookies.
Mozilla Public License 2.0
974 stars 176 forks source link

Very slow performance on page with Facebook Container extension enabled #601

Closed dremin closed 2 years ago

dremin commented 4 years ago

When using the accordions on the page https://developer.mypurecloud.com/api/rest/v2/conversations/index.html, the web page freezes with Facebook Container enabled. With the extension disabled, the page is relatively smooth.

Actual behavior

Web page is very slow. Profile: https://perfht.ml/38HAxnk

Expected behavior

Expected web page to be smooth, which it is with the Facebook Container extension disabled.

Steps to reproduce

  1. Enable Facebook Container extension
  2. Visit https://developer.mypurecloud.com/api/rest/v2/conversations/index.html
  3. Open one of the accordion items (example in the profile is the GET /api/v2/conversations/messages/{conversationId}/messages/{messageId} accordion item)
  4. Notice web page freezes for a few seconds before the item opens.
  5. Repeat with Facebook Container disabled, and notice performance is smooth.
jakub-g commented 4 years ago

I just noticed a super slow performance on Github pull request page with the addon enabled.

  1. Enable FB container addon.
  2. Open some GH PR page which has >50 files
  3. Click "Viewed" on one of those files ⚠️ it takes about 2 seconds for the DOM update to happen
  4. Disable FB container addon and refresh the PR page.
  5. Click "Viewed" on one of those files 🆗 it's near-instant.
mrwensveen commented 3 years ago

I suspect this has to do with memory usage. I notice this behavior on pages that use 150+ MB (as shown in about:performance). DOM updates with Facebook Container enabled take much longer.

It's not so much the amount of DOM nodes, however. I tested with a page that uses a lot of data: URL's for images, and with the same page where those URL's are shorter (using URL.createObjectURL). The amount of DOM nodes is the same, but the memory consumption much lower. Facebook Container makes the inoptimized version almost unusable, while the low-memory version works fine with FBC enabled.

mrwensveen commented 3 years ago

While playing with the source in the add-on debugger I discovered that detectFacebookOnPage is the main culprit, with the patternDetection calls taking up a lot of time. The function is called every time a user clicks on the page (and then set up to periodically run again after a timeout).

What is the thought behind doing this on every click? Maybe there's a way to listen for changes to the DOM and only running the patternDetection function against the changed parts? Another option would be to optimize patternDetection itself, but I don't know if there's an alternative to document.querySelectAll.

mrwensveen commented 3 years ago

Hello again. I've created a patch that uses MutationObserver to detect changes in the DOM and runs the pattern detection on the changed nodes instead of the entire document (contentScriptInit). I've also removed the contentScriptInit call from the click action, because the mutation observer already does everything.

I think my code might need some polishing. Will you accept a PR?

maxxcrawford commented 3 years ago

While playing with the source in the add-on debugger I discovered that detectFacebookOnPage is the main culprit, with the patternDetection calls taking up a lot of time. The function is called every time a user clicks on the page (and then set up to periodically run again after a timeout).

What is the thought behind doing this on every click? Maybe there's a way to listen for changes to the DOM and only running the patternDetection function against the changed parts? Another option would be to optimize patternDetection itself, but I don't know if there's an alternative to document.querySelectAll.

The inital goal for the content script on non-FB pages was to badge Facebook share/login buttons across the web. Often times, these buttons would show up after a user-click (Example: Clicking the "login" button in a nav triggers a modal with a "Login with Facebook" button). The constant checking was to catch these.

Using MutationObserver sounds like it would absolutely be a performance improvement over contentScriptInit and user actions. Please open a PR and we can look. One caveat is that we most likely need our internal QA team to test a release with that accepted, so the timeline for acceptance may not be instanteous.

Thanks for digging in on this @mrwensveen!

maxxcrawford commented 3 years ago

Oh - One thing I should add. We have a draft PR to add a settings panel to this add-on. (#721) One of the optional settings we're adding is turning off the badge detection entirely. Because that setting is optional, I beleive fixing the memory consumption is still the best/right course of action.

barryvan commented 3 years ago

We've run into this issue as well with an application we're building -- with Facebook Container enabled, performance falls completely off a cliff. Disabling the addon fixes things. My investigations match @mrwensveen's observations -- it seems as though there's a querySelectorAll being run inside the pattern detection after every click anywhere on the page. As you can see in this screenshot, it makes things very slow...

image

Admittedly the context that this is happening in has a huge number of DOM nodes, but it makes the entire thing unusable.

Is there anything I can do to help getting this PR through and an update to the addon released?

With regard to #721, is it feasible for a site to somehow opt-out of this badge detection, on the basis that it won't be necessary? I don't believe this would undermine the utility of the addon itself, although I admittedly am not an expert.

KL13NT commented 3 years ago

This is still an issue. Heavy websites have very degraded performance when this extension is enabled. Why doesn't the extension inspect request links instead of looking at the DOM (which is costly)? It'd make more sense to block an outgoing request to Facebook tracking instead of inspecting DOM elements IMHO.

On this page specifically with the extension enabled, trying to change filters adds a huge overhead the average that drops the average FPS to less than 10. In contrast, with the extension disabled the average FPS remains above 30 FPS. This gets worse in interactive websites.

I'm currently debugging on Firefox 88.0.1 (64-bit) latest.

Proposed Solution

I recommend using background scripts to inspect requests instead of looking at the DOM, which would make up for a huge performance improvement. This could affect the user experience since it'd completely disable the "Facebook Container" badge on links and buttons, but is fixable. Instead of actively scanning the DOM for changes, you could use the mousemove event to detect hover over links and buttons and then apply existing UI logic such as adding the badge.

NPatch commented 2 years ago

Still an issue. I can't even open up a message on firefox anymore because it goes on an endless loading binge. Loading posts, loading the chats list and so on.

mrwensveen commented 2 years ago

It seems my PR has some merge conflicts now (it hadn't when I created it). I'll try to take a look at it. It may help to get this through.

mrwensveen commented 2 years ago

I fixed a few merge conflicts. Should be okay now. Please merge my PR, @maxxcrawford 🙏