Open Bug 1245065 Opened 9 years ago Updated 2 years ago

Investigate sanitize.js shutdown hangs/crashes

Categories

(Core :: General, defect)

defect

Tracking

()

People

(Reporter: Yoric, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

Attachments

(2 files)

According to AWSDY ( http://yoric.github.io/are-we-shutting-down-yet-/?version=Firefox+47.0a1&version=Firefox+46.0a2&version=Firefox+45.0b1&version=Firefox+45.0a2&version=Firefox+44.0b99&version=Firefox+44.0b9&version=Firefox+44.0b1&version=Firefox+44.0&version=Firefox+43.0b3 ), we have many shutdown hangs/crashes in sanitization. Some of these hangs/crashes take place while clearing formdata, some while clearing cookies. Given how sanitization works, there is the possibility that we are attempting to clear stuff after windows and other stuff are uninitialized, which could explain hangs.
Assignee: nobody → dteller
So, it's pretty clear that nsICookieManager doesn't shutdown properly. That's one of the causes of the issue.
Attached file shutdown.txt (deleted) —
Best guess: it's related to these errors.
My bet is that bug 1245578 will fix a big subset of these crashes.
These days, the sanitizer often attempts to cleanup cookies during shutdown *after* the cookie service has been disconnected, hence when it is too late. This causes privacy issues and is suspected of causing shutdown hangs/crashes. Bug 1245578 introduces a shutdownClient for the nsCookieService. In this patch, we make use of this shutdownClient to properly handle sanitization of the cookies during shutdown. Review commit: https://reviewboard.mozilla.org/r/33769/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33769/
Attachment #8716269 - Flags: review?(mak77)
The attached patch (which is based on bug 1245578) should fix the sanitization of cookies during shutdown. We still need to fix "sessions". I'm not sure what that is yet.
Comment on attachment 8716269 [details] MozReview Request: Bug 1245065 - Making sanitize.js handle properly nsCookieService shutdown;r?mak https://reviewboard.mozilla.org/r/33769/#review30449 ::: browser/base/content/sanitize.js:704 (Diff revision 1) > - .jsclient; > + .jsclient; one uses Cc, the other one Components.classes, and dots should be aligned with [
Attachment #8716269 - Flags: review?(mak77) → review+
Comment on attachment 8716269 [details] MozReview Request: Bug 1245065 - Making sanitize.js handle properly nsCookieService shutdown;r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33769/diff/1-2/
Comment on attachment 8716269 [details] MozReview Request: Bug 1245065 - Making sanitize.js handle properly nsCookieService shutdown;r?mak https://reviewboard.mozilla.org/r/33769/#review30521 ::: browser/base/content/sanitize.js:716 (Diff revision 2) > + return deferredSanitization.promise; I'd prefer if you'd unified the return code by inverting the if. It would be less error-prone to future changes.
Attachment #8716269 - Flags: review+
Will we have a 44.0.2 release for this and 1245578 (and possibly 1243549)? Without these repaired, sanitize on exit is not reliable, which is a significant privacy issue. This is a key issue as defined by Principle #4 of The Mozilla Manifesto (https://www.mozilla.org/about/manifesto/). Accordingly, I recommend an increase in importance.
(In reply to Marco Bonardo [::mak] from comment #9) > I'd prefer if you'd unified the return code by inverting the if. It would be > less error-prone to future changes. I'm fixing this directly in bug 1243549
Blocks: 1245101

Not working on this anymore.

Assignee: dteller → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: