1510275 - Accessing the local storage causing too much work

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

In this profile: https://perfht.ml/2ArZ67J we spend piles of time doing Content:SecurityChange related things. It seems like this might be triggered by nsContentUtils::StorageDisabledByAntiTracking

Comment 1

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

Hey ehsan - are you familiar with this code at all? Do you know what the parent is up to - it seems super busy dealing with RemoteWebProgress messages and getting security state updates - see https://perfht.ml/2AvZkuK, for example.

Comment 2

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

It looks like Bugzilla is using localStorage to store comment state, and... if I'm reading this correctly, accesses to window.localStorage cause security change notifications to fire?

https://perfht.ml/2AqYFuk

Comment 3

[Jeff Muizelaar [:jrmuizel]]

It seems like I have a localStorage entry for every bug I have ever visited. I'm not able to load all of the local storage entries in dev tools because it takes too long.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

The entries are of the form: '230959:"{"visitedTime":1327966524169,"commentCount":32,"flags":{},"attachments":{"139101":[],"558444":[],"558763":[],"581637":[],"592114":[]}}"' and localStorage.length = 3683.

I wonder perhaps if these are from the bugzilla tweaks add-on which I used to use.

Comment 5

[(no longer active)]

This was made worse by bug 1504728.

Comment 6

[(no longer active)]

We need to find a strategy for nsGlobalWindowOuter::NotifyContentBlockingState() to avoid bombarding the parent process in situations like when there is a loop iterating over all of the entires in the localStorage...

Comment 7

[(no longer active)]

One interesting observation that I made here is that if I run this test case from a localhost server, it is about 15 times faster than if I type it in the web console on Bugzilla.  Similar performance difference exists if I type the code in the web console on perf-html.io.

let begin = performance.now();

for (let i = 0; i < 10000; i++) {
  localStorage.setItem("foo", localStorage.getItem("foo"));
  localStorage.setItem("bar", localStorage.getItem("bar"));
}

let end = performance.now();
alert(end - begin);

Comment 8

[(no longer active)]

Aha, the reason is all of the extra time it takes to serialize Bugzilla's certificate to a string, every single time.  :-(

I think this points to an architectural problem.  We've been cramming everything under the sun into the onSecurityChange notification and I think this bug is demonstrating the performance consequences of doing so in the extreme case.  Perhaps it is time to add a new nsIWebProgressListener method, onContentBlockingStateChange() or something, used exclusively for this purpose.  What do you think?

Comment 9

[Johann Hofmann [:johannh]]

Yes, that's a great idea. This is what we should have been doing from the start, in hindsight. How urgent is this? Seems like a great candidate for 66/67 cleanup work, but not really like a thing we could just squeeze in last minute in 65...

Comment 10

[(no longer active)]

On the contrary, I think we should address this problem for 65.  One way or another.  (Not suggesting comment 9 is necessarily the solution for 65 yet.)

Comment 11

[(no longer active)]

I worked on this a bit on Monday.  I tried to understand why this condition <https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/dom/base/nsGlobalWindowOuter.cpp#5073> is insufficient for preventing the repeated notifications from being dispatched here.

It turns out that there were two bugs at play here.

The first bug is this return statement: <https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/dom/base/ContentBlockingLog.h#170>.  This causes calls to nsIDocument::GetHasCookiesLoaded() <https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/dom/base/nsIDocument.h#997> to return false in the case where the content blocking log would include an entry without a STATE_COOKIES_LOADED item, since that return statement would cause us to return false from HasBlockedAnyOfType() prematurely without potentially examining the rest of the entries in the content blocking log.  This is what's special about Bugzilla, it has an entry in the content blocking log for https://www.google-analytics.com which causes this early return path to be taken.

But fixing that itself isn't sufficient to completely address this problem, since it also turns out that the logic here <https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/dom/base/nsGlobalWindowOuter.cpp#5057> is not quite right when it comes to turning the STATE_COOKIES_LOADED flag on or off.  For turning this state on, we need to look for |!aBlocked| not |aBlocked| because of the reason that is documented in the commented there (this is currently artificially achieved through setting aBlocked = true).  Also, we can't always set |unblocked| to false, rather we should set it based on what doc->GetHasCookiesLoaded() returns similar to other cases.  This is critical because otherwise we can't guarantee to only dispatch this notification the first time we see the flag on the top-level document.

With these two issues fixed, we only dispatch a single STATE_COOKIES_LOADED flag here.

Comment 12

[(no longer active)]

I did my best to write a test for this but simulating the exact conditions that simulate what happens to bugzilla inside a mochitest proved to be too difficult.  I kind of gave up after a couple of ours but please let me know if you'd like me to try harder...

Comment 13

[(no longer active)]

Attachment: Bug 1510275 - Make sure that we only ever dispatch a single STATE_COOKIES_LOADED notification per top-level document

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f9b987dc957
Make sure that we only ever dispatch a single STATE_COOKIES_LOADED notification per top-level document r=baku

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/1f9b987dc957