Open
Bug 1245578
Opened 8 years ago
Updated 2 years ago
nsCookieService is not shutdown-safe
Categories
(Core :: Networking: Cookies, defect, P3)
Core
Networking: Cookies
Tracking
()
NEW
People
(Reporter: Yoric, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: crash, privacy, Whiteboard: [necko-backlog])
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
Details |
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.
Reporter | ||
Updated•8 years ago
|
Summary: nsCookieService is not AsyncShutdown-safe → nsCookieService is not shutdown-safe
Reporter | ||
Comment 1•8 years ago
|
||
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.
Reporter | ||
Comment 2•8 years ago
|
||
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/
Attachment #8716267 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → dteller
Comment 3•8 years ago
|
||
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.
Attachment #8716267 -
Flags: review?(jduell.mcbugs) → review?(bzbarsky)
Comment 4•8 years ago
|
||
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... ;)
Attachment #8716267 -
Flags: review?(bzbarsky) → review?(nfroyd)
Reporter | ||
Comment 5•8 years ago
|
||
Well, I guess we could move async shutdown to xpcom/.
Comment 6•8 years ago
|
||
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.
Attachment #8716267 -
Flags: review?(nfroyd)
Comment 7•8 years ago
|
||
(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/.
Reporter | ||
Comment 8•8 years ago
|
||
(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•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(dteller)
Updated•8 years ago
|
Whiteboard: [necko-backlog]
Comment 11•7 years ago
|
||
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Comment 12•7 years ago
|
||
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•