1533134 - Handling the clear-origin-attributes-data notification in appcache takes a while and does main thread I/O during shutdown

Status: RESOLVED INACTIVE

(Core :: Networking: Cache, defect, P2)

Description

[Florian Quèze [:florian]]

See this profile: https://perfht.ml/2UoXtR3

I think this is a regression caused by bug 1462662

Comment 1

[Kate Hudson :k88hudson]

Is this something to do with your patch in Bug 1462662?

Comment 2

[Andrea Marchesini [:baku]]

about:newtab uses containers to load the website thumbnails. On shutdown, we delete them corresponding containers data. This operation requires several main-thread operations, similar to what we do for site-data cleanup.
In the profile I see I/O done on main-thread by nsApplicationCacheService.
Mayhemer, do you know if we we can move that cleanup out of the main-thread?

Comment 3

[Johann Hofmann [:johannh]]

So, bug 1350544 again?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Johann Hofmann [:johannh] from comment #3)

So, bug 1350544 again?

I'm afraid so. That code is old. Is it that big problem if this is shutdown only? I can't find the bits you mention in the profile, could you point me at the right time frame?

Comment 5

[Andrea Marchesini [:baku]]

https://profiler.firefox.com/public/9881a297737958ce4ebb06fe9e38cadc32a0e98e/calltree/?globalTrackOrder=0-1-2-3&range=4.3035_4.3626&thread=0&timelineType=stack&v=3 shows that 89% is spent by:

https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/netwerk/cache/nsApplicationCacheService.cpp#205
and
https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/netwerk/cache/nsApplicationCacheService.cpp#153

Comment 6

[Dragana Damjanovic [:dragana]]

michal, can you take a look?

Comment 7

[Honza Bambas (:mayhemer)]

Another case of bug 1350544. If this is happening on shutdown (presumably after the window is already gone) I don't see any reason to move this to a background thread. We will have to block the shutdown on that IO anyway (as we do in cache2/http, and it may really block the main thread as well on the IO thread join).

Comment 8

[Florian Quèze [:florian]]

Could we avoid writing this data to disk, instead of cleaning it up during shutdown?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Florian Quèze [:florian] from comment #8)

Could we avoid writing this data to disk, instead of cleaning it up during shutdown?

I have no idea who triggers that. It's totally unclear from the referred profile too. If we can determine if this is because of preferences set to "clear private data at shutdown" (very likely) then this is a WONTFIX. That option let's all cached data be written to disk so that long standing sessions can benefit from caching (in general, not just appcache) but all has to be cleared when firefox is closed. There is then not much to do and I don't want to get to the discussion about why we even have such (st...d) option these days and why it behaves the way it does - it's more for product managers.

If there is a strong push on changing this shutdown behavior (instead of just removing the option in favor of permanent PB mode) I was proposing a long ago to have a separate process spawned-and-forgotten at shutdown that would rename (i.e. make inaccessible) all data we want to clear and then start clearing it using any s-specific methods/functions for best efficiency. This would speed up firefox main process closing and also reopening.

Comment 10

[Honza Bambas (:mayhemer)]

Ah, yes, it is the pref! Didn't expand the stack completely. sanitizeOnShutdown function is called.

So, as comment 9 says - WONTFIX or remove this code in favor of perm PB mode or spin-off clearing to a new separate process.

Comment 11

[Florian Quèze [:florian]]

(In reply to Honza Bambas (:mayhemer) from comment #10)

Ah, yes, it is the pref! Didn't expand the stack completely. sanitizeOnShutdown function is called.

I don't think it's the pref, no. Unless I misunderstood something, this code path is taken unconditionally during shutdown since bug 1462662.

Comment 12

[Honza Bambas (:mayhemer)]

Was not aware, thanks, it means to take a different approach then. I don't think we write to the appcache something at all, it's extremely rare on the web. Then let's disable appcache for that container and let it bypass handling of that notification for that exact container too: no IO at all.

Do you agree?

Comment 13

[Florian Quèze [:florian]]

(In reply to Honza Bambas (:mayhemer) from comment #12)

let's disable appcache for that container and let it bypass handling of that notification for that exact container too: no IO at all.

Sounds great!

Comment 14

[Honza Bambas (:mayhemer)]

I need one advice, how can I get in C++ the id of the new tab segregation context:
https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/browser/modules/Sanitizer.jsm#914

thanks.

Comment 15

[Florian Quèze [:florian]]

(In reply to Honza Bambas (:mayhemer) from comment #14)

how can I get in C++ the id of the new tab segregation context:
https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/browser/modules/Sanitizer.jsm#914

I don't know. Johann, is this something you could help with?

Comment 16

[Johann Hofmann [:johannh]]

I have to say I'm a bit disappointed that ContextualIdentityService.jsm doesn't have a single line of documentation.

I think that Service does not have a C++ interface, so the best you could do is to take the hardcoded userContextId (5) from here: https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/toolkit/components/contextualidentity/ContextualIdentityService.jsm#103-108

And maybe add a comment to that code that this ID is mirrored in wherever you use it in your C++ code so that folks don't accidentally change it under your feet.

It's the best I can think of. Or put it in a pref, maybe? And then use that pref in your C++ as well as in the .jsm?

Maybe baku has some thoughts.

Thanks!

Comment 17

[Honza Bambas (:mayhemer)]

Thanks, Johann. I will hard code and comment and maybe file a followup to clean this up, that's probably best to quickly fix this.

Comment 18

[Johann Hofmann [:johannh]]

sgtm!

Comment 20

[:Gijs (he/him)]

Honza, in bug 1580650 you remarked you hadn't had time to work on this - would you want someone else to take it?

Comment 21

[Honza Bambas (:mayhemer)]

Feel free to steal this bug from me at will, but please let me know before you do, so we don't conflict. If I start working on it, will drop a note here as well.

Should we turn this to a P1?

Comment 22

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #21)

Feel free to steal this bug from me at will, but please let me know before you do, so we don't conflict. If I start working on it, will drop a note here as well.

Hopefully next week. Trying to consolidate some other things so I don't pick up too much at the same time.

Should we turn this to a P1?

I think a P1 requires an assignee...

Comment 23

[Valentin Gosu [:valentin] (he/him)]

Appcache was removed.