Closed
Bug 1258354
Opened 8 years ago
Closed 8 years ago
Sanitize on shutdown adds 2 blockers, when only one is really needed
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 48
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
Yoric
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-release+
|
Details |
Sanitize on shutdown adds 1 blocker, but then sanitize adds another one, that is resolved earlier. This could somehow generate race conditions, and regardless we don't need to do that, we just need to annotate if the sanitize is on shutdown or not in the metadata.
Assignee | ||
Comment 1•8 years ago
|
||
The story is slightly different. We add a blocker to be notified of Places shutdown, then we wait for a task that adds another blocker, that is resolved before the external one. I'm not sure if async shutdown is expected to properly support adding a blocker from a running blocker, also considered: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/asyncshutdown/nsIAsyncShutdown.idl 82 * Calling `addBlocker` once nsIAsyncShutdownBarrier::wait() has been 83 * called on the owning barrier returns an error. So, maybe some refactoring will be useful regardless to avoid troubles.
Ah, the documentation is out of date. AsyncShutdown has been refactored to allow adding a blocker to be added while the barrier is running.
Assignee | ||
Comment 3•8 years ago
|
||
I see. The problem here is that the external blocker waits for the internal one and sometimes we end up aborting on the external blocker that has no metadata about what happened. So we don't know what blocked us. Maybe it just happen to come first in the blockers array. Ideally we could just pass-through the progress object... but in the end we don't really need the internal blocker, so I think I will also bypass it.
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41451/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41451/
Attachment #8732907 -
Flags: review?(dteller)
Assignee | ||
Comment 5•8 years ago
|
||
Sorry David, I know you're switching teams, but I still have some pending sanitize work that could benefit from your knowledge. Let me know if I should change reviewer.
I'll try and review it by Thursday, if that's ok with you.
Assignee | ||
Comment 7•8 years ago
|
||
no hurry.
Apologies for the delay, this was a crazy workweek. I'm on it now.
Attachment #8732907 -
Flags: review?(dteller)
Comment on attachment 8732907 [details] MozReview Request: Bug 1258354 - Sanitize on shutdown adds 2 blockers, when only one is really needed. r=yoric https://reviewboard.mozilla.org/r/41451/#review39437 ::: browser/base/content/sanitize.js:62 (Diff revision 1) > * cleared before the promise is finally rejected. > * > * If the consumer specifies the (optional) array parameter, only those > * items get cleared (irrespective of the preference settings) > */ > - sanitize: Task.async(function*(aItemsToClear = null) { > + sanitize: Task.async(function*(aItemsToClear = null, options = {}) { Could you document `options`? Looking at the code, it's not clear to me whether `isShutdown` is an input option, something that is going to be overwritten, or something else entirely.
Assignee | ||
Comment 10•8 years ago
|
||
actually isShudown is an "internal" option, used only internally by sanitize.js (in future Sanitize.jsm) to annotate whether we need a blocker or not. Thus I was not planning to document it. The options object will be reused by bug 1167238 to pass the "real" options, like prefDomain, range and so on and there I will have proper documentation.
Assignee | ||
Comment 11•8 years ago
|
||
I was also misremembering. isShutdown is part of the progress object we use to track async shutdown state, it indicates that progress is relative to a shutdown. There should be no need to document it at all. progress is the "internal" property I was speaking about in comment 10. External consumers are very unlikely to need to pass a progress object at all. and other options properties will be documented when they appear (bug 1167238). Here I have nothing of interest to document... If you wish I may add a comment before: let progress = { isShutdown: true };
Yes, that would be useful. In the current state, I don't understand this line when I read it, which is a bad sign.
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8732907 [details] MozReview Request: Bug 1258354 - Sanitize on shutdown adds 2 blockers, when only one is really needed. r=yoric Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41451/diff/1-2/
Attachment #8732907 -
Flags: review?(dteller)
Comment on attachment 8732907 [details] MozReview Request: Bug 1258354 - Sanitize on shutdown adds 2 blockers, when only one is really needed. r=yoric https://reviewboard.mozilla.org/r/41451/#review43009 I understand that this patch needs to be landed quickly. If so, you can wait for a followup to write the documentation. ::: browser/base/content/sanitize.js:63 (Diff revisions 1 - 2) > * > - * If the consumer specifies the (optional) array parameter, only those > - * items get cleared (irrespective of the preference settings) > + * @param [optional] itemsToClear > + * Array of items to be cleared. if specified only those > + * items get cleared, irrespectively of the preference settings. > + * @param [optional] options > + * Object whose properties are specific options for this clearing. That's not a documentation, it's a placeholder :) ::: browser/base/content/sanitize.js:842 (Diff revisions 1 - 2) > // Make sure that we are triggered during shutdown. > let shutdownClient = Cc["@mozilla.org/browser/nav-history-service;1"] > .getService(Ci.nsPIPlacesDatabase) > .shutdownClient > .jsclient; > + // We pass through an `options` object with a `progress` property to That's unfortunately not clear at all.
Attachment #8732907 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 15•8 years ago
|
||
I'll try to further improve the comment, I actually thought now it was clear. I can expand it. Re: the documentation, yes it's a placeholder cause the work to implement the important part of it didn't land yet. I expect no external consumer to use this argument until we make it actually useful.
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8732907 [details] MozReview Request: Bug 1258354 - Sanitize on shutdown adds 2 blockers, when only one is really needed. r=yoric Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41451/diff/2-3/
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7772a6288f8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8732907 [details] MozReview Request: Bug 1258354 - Sanitize on shutdown adds 2 blockers, when only one is really needed. r=yoric Approval Request Comment [Feature/regressing bug #]: Async shutdown [User impact if declined]: Bug 1258350 (shutdown crash) is hard to figure out. [Describe test coverage new/current, TreeHerder]: nightly [Risks and why]: limited code changes, low risk, safe to backout [String/UUID change made/needed]: none
Attachment #8732907 -
Flags: approval-mozilla-beta?
Attachment #8732907 -
Flags: approval-mozilla-aurora?
Comment 20•8 years ago
|
||
This will miss today's RC1 build. So it could make it into an RC2. After that there is not a lot of opportunity for backout. Is the possible gain from this worth risking an RC3 build?
Flags: needinfo?(mak77)
Assignee | ||
Comment 21•8 years ago
|
||
The gain is that it may help with crash bug 1258350, either by reducing the amount of crashes and/or help making a better patch for the crash in a future dot release. It depends how much you value that crash bug.
Flags: needinfo?(mak77)
Assignee | ||
Comment 22•8 years ago
|
||
alternatively, we could just uplift to Aurora, collect crash stats from 47 Beta and make a patch for a 46 dot release (again, if that crash impact is important enough).
Comment 23•8 years ago
|
||
Comment on attachment 8732907 [details] MozReview Request: Bug 1258354 - Sanitize on shutdown adds 2 blockers, when only one is really needed. r=yoric Please uplift to aurora, m-b, and m-r. Possible fix/diagnostic help for top crash on beta.
Attachment #8732907 -
Flags: approval-mozilla-release+
Attachment #8732907 -
Flags: approval-mozilla-beta?
Attachment #8732907 -
Flags: approval-mozilla-beta+
Attachment #8732907 -
Flags: approval-mozilla-aurora?
Attachment #8732907 -
Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-release/rev/ff7554aa29bf59c5f32dba55a76a61d9cad02cf8 https://hg.mozilla.org/releases/mozilla-beta/rev/d84b94361a277835e64d436933c263970845fa4f https://hg.mozilla.org/releases/mozilla-aurora/rev/5b11886851efd1ab4e012508f6b7151dc71972f6
status-firefox46:
--- → fixed
status-firefox47:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•