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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file)

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.
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.
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.
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.
no hurry.
Apologies for the delay, this was a crazy workweek. I'm on it now.
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.
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.
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.
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+
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.
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/
https://hg.mozilla.org/mozilla-central/rev/b7772a6288f8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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?
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)
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)
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 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+
You need to log in before you can comment on or make changes to this bug.