1008148 - Use AsyncShutdown for PageThumbsStorage.wipe()

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

To clarify, PageThumbsStorage.wipe() may be called on shutdown with "Clear History when Firefox closes" enabled.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Using AsyncShutdown for PageThumbsStorage.wipe()

Here's one way to do it. Untested.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(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()?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Using AsyncShutdown for PageThumbsStorage.wipe(), v2

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Using AsyncShutdown for PageThumbsStorage.wipe(), v3

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=0a8ddf7b12e7

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Tim, as I'll be afk this week, can I ask you to handle any uplift request, if it proves necessary?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Sure!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/639b7f55347a

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/639b7f55347a

Comment 17

[David Teller [:Yoric] - still alive but not very active]

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:

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by: will come back for this to uplift early next week in order to ensure it's in Beta 8.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ca75e3345ba
https://hg.mozilla.org/releases/mozilla-beta/rev/8d180eb195d2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/8d180eb195d2

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.