Use AsyncShutdown for PageThumbsStorage.wipe()

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: Yoric)

Tracking

(Blocks 1 bug)

Trunk
Firefox 32
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox30 fixed, firefox31 fixed, firefox32 fixed)

Details

(Whiteboard: p=0 s=33.1 [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

As seen in bug 1005487, we currently possibly spawn the PageThumbsWorker on shutdown and try to wipe the directory. The worker can be destroyed before that succeeds though so we will have to make it use the AsyncShutdown API.

David says that this should be easy to do once bug 985655 has landed.
Flags: firefox-backlog+
To clarify, PageThumbsStorage.wipe() may be called on shutdown with "Clear History when Firefox closes" enabled.
Here's one way to do it. Untested.
Attachment #8421043 - Flags: feedback?(ttaubert)
(Reporter)

Updated

5 years ago
Attachment #8421043 - Attachment is patch: true
Comment on attachment 8421043 [details] [diff] [review]
Using AsyncShutdown for PageThumbsStorage.wipe()

Review of attachment 8421043 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +588,5 @@
> +    // reached profileBeforeChange, in which case it is too late
> +    // to clear the thumbnail wipe.
> +    AsyncShutdown.profileBeforeChange.addBlocker(
> +      "PageThumbs: removing all thumbnails",
> +      blocker);

Can we only add the blocker if nsIAppStartup.shuttingDown=true? Probably needs to be tested whether that is true at this step already when clearing history on shutdown. In that case we also wouldn't need to remove that blocker. Does AsyncShutdown fail if the promise is rejected?
Attachment #8421043 - Flags: feedback?(ttaubert) → feedback+
(In reply to Tim Taubert [:ttaubert] from comment #3)
> Can we only add the blocker if nsIAppStartup.shuttingDown=true?

We can, but that's still open to a race condition.
Also, it's probably not really useful. We just save a few function calls and two operations on a Map().

> Probably
> needs to be tested whether that is true at this step already when clearing
> history on shutdown. In that case we also wouldn't need to remove that
> blocker. Does AsyncShutdown fail if the promise is rejected?

No, AsyncShutdown always succeeds, but it will print a warning if the promise is rejected.
(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> (In reply to Tim Taubert [:ttaubert] from comment #3)
> > Can we only add the blocker if nsIAppStartup.shuttingDown=true?
> 
> We can, but that's still open to a race condition.
> Also, it's probably not really useful. We just save a few function calls and
> two operations on a Map().

Ok, yeah that shouldn't be a problem. Two more thoughts:

1) AsyncShutdown.removeBlocker() doesn't exist currently. How do we handle removing a promise that we already started spinning? That might complicate the code somewhat.

2) Would it be possible to implement/add shutdown blockers for PromiseWorkers in general? As long as there's some pending message/operation we could just wait until that's resolved?
(In reply to Tim Taubert [:ttaubert] from comment #5)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> > (In reply to Tim Taubert [:ttaubert] from comment #3)
> > > Can we only add the blocker if nsIAppStartup.shuttingDown=true?
> > 
> > We can, but that's still open to a race condition.
> > Also, it's probably not really useful. We just save a few function calls and
> > two operations on a Map().
> 
> Ok, yeah that shouldn't be a problem. Two more thoughts:
> 
> 1) AsyncShutdown.removeBlocker() doesn't exist currently. How do we handle
> removing a promise that we already started spinning? That might complicate
> the code somewhat.

AsyncShutdown.removeBlocker will land as part of bug 985655.
If we have already started spinning, removeBlocker is a noop (well, it returns `false` instead of `true`). However, that's exactly what we want in that case.

> 2) Would it be possible to implement/add shutdown blockers for
> PromiseWorkers in general? As long as there's some pending message/operation
> we could just wait until that's resolved?

Probably, yes. That's pretty much what I'm doing with OS.File. Generalizing this might need a little thinking, though.
(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> > 1) AsyncShutdown.removeBlocker() doesn't exist currently. How do we handle
> > removing a promise that we already started spinning? That might complicate
> > the code somewhat.
> 
> AsyncShutdown.removeBlocker will land as part of bug 985655.
> If we have already started spinning, removeBlocker is a noop (well, it
> returns `false` instead of `true`). However, that's exactly what we want in
> that case.

That sounds like a fine approach. The blocker we're currently adding will never be resolved though even if wipe() finished. Shouldn't we just add the promise returned by Task.spawn()?
(In reply to Tim Taubert [:ttaubert] from comment #7)
> That sounds like a fine approach. The blocker we're currently adding will
> never be resolved though even if wipe() finished. Shouldn't we just add the
> promise returned by Task.spawn()?

The call to `removeBlocker` behaves as if we resolved the blocker, so that shouldn't be an issue.
Ok, I guess I'm out of questions then :) We should document the behavior with a nice comment but other than that the approach sounds fine.
Assignee: nobody → dteller
Attachment #8421043 - Attachment is obsolete: true
Attachment #8423919 - Flags: review?(ttaubert)
Comment on attachment 8423919 [details] [diff] [review]
Using AsyncShutdown for PageThumbsStorage.wipe(), v2

Review of attachment 8423919 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me with the two nits fixed.

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +588,5 @@
> +    //
> +
> +    let promise = null;
> +      // A promise satisfied once the wipe operation
> +      // is complete.

Nit: the comment indentation is a little off.

@@ +598,5 @@
> +    AsyncShutdown.profileBeforeChange.addBlocker(
> +      "PageThumbs: removing all thumbnails",
> +      blocker);
> +
> +    promise = PageThubmsWorker.post("wipe", [this.path]);

Initializing |promise| with null is quite confusing. I think we should define and declare it at the same time.
Attachment #8423919 - Flags: review?(ttaubert) → review+
(Reporter)

Updated

5 years ago
Status: NEW → ASSIGNED
Tim, as I'll be afk this week, can I ask you to handle any uplift request, if it proves necessary?
Keywords: checkin-needed
Flags: needinfo?(ttaubert)
Sure!
Flags: needinfo?(ttaubert)
https://hg.mozilla.org/mozilla-central/rev/639b7f55347a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Comment on attachment 8424519 [details] [diff] [review]
Using AsyncShutdown for PageThumbsStorage.wipe(), v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Core bug in workers + bug 753768.
User impact if declined: Users that clear private data on shutdown can't quit Firefox if they haven't browsed sufficiently.
Testing completed (on m-c, etc.): 2 days on m-c. I'd let it simmer a few days then uplift.
Risk to taking this patch (and alternatives if risky): Can't think of any risk. Alternative would be to uplift bug 922298 instead, but it hasn't been reviewed yet.
String or IDL/UUID changes made by this patch:
Attachment #8424519 - Flags: approval-mozilla-beta?
Attachment #8424519 - Flags: approval-mozilla-aurora?
Triage drive-by: will come back for this to uplift early next week in order to ensure it's in Beta 8.
Comment on attachment 8424519 [details] [diff] [review]
Using AsyncShutdown for PageThumbsStorage.wipe(), v3

Approving for aurora to have it in Tomorrow nightly to get more coverage. 
Approving also in beta to have in beta 8.
Attachment #8424519 - Flags: approval-mozilla-beta?
Attachment #8424519 - Flags: approval-mozilla-beta+
Attachment #8424519 - Flags: approval-mozilla-aurora?
Attachment #8424519 - Flags: approval-mozilla-aurora+
Blocks: 1005487
No longer depends on: 1005487
Tried to reproduce shutdown hangs on Firefox 30 Beta 9 using scenarios from: bug 1005487, bug 1005958,
and bug=1006478. No hangs could be reproduced, so unless any users get this again after the fix, this issue seems fixed.
Whiteboard: [qa-]
Whiteboard: [qa-] → p=0 s=33.1 [qa-]
You need to log in before you can comment on or make changes to this bug.