1245578 - nsCookieService is not shutdown-safe

Status: NEW

(Core :: Networking: Cookies, defect, P3)

Description

[David Teller [:Yoric] - still alive but not very active]

Right now, nsCookieService joyously shuts down on profile-before-change without waiting for any client. This prevents shutdown-time cleanup of cookies, hence causes privacy issues. I suspect that this also causes shutdown crashes.

We need to fix this.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

For clarification: instead of waiting for profile-before-change, nsCookieService should:
- offer a nsIAsyncShutdownClient for all its own clients;
- block shutdown of nsIAsyncShutdown::profileBeforeChange until all its clients are done.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1245578 - Make the cookie manager shutdown-safe;r?jduell

The Cookie Service was written in times immemorial, when shutdown was
a simple thing. These days, this causes the Cookie Service to
sometimes shutdown before some clients of the nsICookieManager can
finish their work. This interferes with privacy cleanups and is
suspected to cause shutdown hangs/crashes.

This patch replaces the shutdown during notification
profile-before-change with the following algorithm:

1. Add a shutdown blocker for profileBeforeChange;

2. Upon notification that profileBeforeChange is in progress, inform
clients that have setup shutdown blockers upon the cookie service;

3. Once all clients have lifted their shutdown blocker, lift the
profileBeforeChange shutdown blocker.

Review commit: https://reviewboard.mozilla.org/r/33765/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33765/

Comment 3

[Jason Duell]

Comment on attachment 8716267 [details]
MozReview Request: Bug 1245578 - Make the cookie manager shutdown-safe;r?jduell

I don't think I know our XPCOM service shutdown stuff well enough to review this.  I'm going to throw it at bz.  Boris, feel free to give to someone else if you can think of a good person.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8716267 [details]
MozReview Request: Bug 1245578 - Make the cookie manager shutdown-safe;r?jduell

Going to punt to Nathan, since he has review-blame for some of this stuff already.

On a personal note, it's a bit weird to have necko depending on IDLs that live in toolkit as opposed to living in XPCOM, but I guess we're just one big ball of wax now... ;)

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Well, I guess we could move async shutdown to xpcom/.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8716267 [details]
MozReview Request: Bug 1245578 - Make the cookie manager shutdown-safe;r?jduell

https://reviewboard.mozilla.org/r/33765/#review30923

I think this all makes sense, but there are some things I would like clarified.

::: netwerk/cookie/nsCookieService.cpp:740
(Diff revision 1)
> +  uint16_t mCounter;

This doesn't appear to be initialized anywhere?

Also, how are we shutting down all our services and starting back up?  That sounds awful!

::: netwerk/cookie/nsCookieService.cpp:741
(Diff revision 1)
> +  static uint16_t sCounter;

Likewise, I don't see this variable accessed anywhere...

::: netwerk/cookie/nsCookieService.cpp:790
(Diff revision 1)
> +    nsPrintfCString name("nsCookieService Shutdown: Blocking profile-before-change (%x)", this);

I think this printf string should be using %p instead of %x, since you're passing a pointer, not an integer.

::: netwerk/cookie/nsCookieService.cpp:831
(Diff revision 1)
> +  rv = bag->SetPropertyAsInterface(NS_LITERAL_STRING("Barrier"), barrier);

Is this supposed to be "Barrier" or "barrier".  If the former, the comment above needs to be changed.  (Also, having fields capitalized like that is weird.)  If it's the latter, as the comment above suggests, this code needs to be changed.

::: netwerk/cookie/nsCookieService.cpp:4865
(Diff revision 1)
> +  NS_IF_ADDREF(*aClient = client);

Just:

client.forget(aClient);

please.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #5)
> Well, I guess we could move async shutdown to xpcom/.

I think this makes sense, so long as we're not moving JS-y things into xpcom/.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> I think this makes sense, so long as we're not moving JS-y things into
> xpcom/.

Well, that might be a problem, since the implementation of async shutdown is in js. Well, maybe when we rewrite the world in Rust...

Comment 9

[Boris Zbarsky [:bzbarsky]]

The implementation and the interface have no reason to be in the same place.

In fact, it's common to have an interface in xpcom/ (e.g. nsISupports!) that is then implemented by various things including JS.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

:bz, good point.

Comment 11

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 12

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Not working on this anymore.