1166293 - provide an easier way to do simple shutdown blockers

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Nathan Froyd [:froydnj]]

jesup was looking at the AsyncShutdown API yesterday and expressed dismay that there seemed to be a lot of work ("boilerplate") to use the API, when one would like to simply do:

  nsCOMPtr<nsIAsyncShutdownBlocker> blocker
  // I think this calls nsIAsyncShutdownClient.addBlocker under the
  // hood.  Does that call nsIAsyncShutdownBlocker.blockShutdown?
  shutdownService->GetBlocker("xpcom-shutdown", &blocker, "blockerName");

  // On some other thread...
  blocker->Remove();

I think what we have right now is pretty reasonable, but hiding a lot of the nsIAsyncShutdownBlocker stuff for the simple cases would be nice.  David, WDYT?

Comment 1

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

I agree that we should provide something simpler for the simple cases. The snippet that you provide has the drawback that it doesn't give any state information for debugging purposes. Perhaps we could do something simple with C++ 11 closures?

Comment 2

[Nathan Froyd [:froydnj]]

I'm not particularly worried about not providing state information, and I think that trying to do something with C++ closures might carry this beyond the simple API we want it to be.  What sort of state do we provide for something like Places?

Comment 3

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

Experience with JS clients indicates that state is pretty useful.
In Places, the state is:
- an enum indicating which of the shutdown operations we have completed so far;
- the state of all the clients of Places.

Also, with your code, we lose line & file, which makes it harder to look for specific crash signatures.

What about one of the following?

Option 1.
NS_GETSHUTDOWNBLOCKER("xpcom-shutdown", &blocker, "blockerName", [optional nsIAsyncShutdownStateReporter]);
// ...
blocker->Remove();

Option 2.
NS_GETSHUTDOWNBLOCKER("xpcom-shutdown", &blocker, "blockerName");
blocker->setStateReporter(...) // Optional
// ...
blocker->Remove();

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #3)
> Experience with JS clients indicates that state is pretty useful.
> In Places, the state is:
> - an enum indicating which of the shutdown operations we have completed so
> far;
> - the state of all the clients of Places.

Noted, but I would also contend that shutting down Places is significantly more complicated that the problems we are trying to solve here.

> Also, with your code, we lose line & file, which makes it harder to look for
> specific crash signatures.

Why is this?  Because the blocker is created inside the service (and that's where file/line come from) rather than a specific client location?

> What about one of the following?
> 
> Option 1.
> NS_GETSHUTDOWNBLOCKER("xpcom-shutdown", &blocker, "blockerName", [optional
> nsIAsyncShutdownStateReporter]);
> // ...
> blocker->Remove();
> 
> Option 2.
> NS_GETSHUTDOWNBLOCKER("xpcom-shutdown", &blocker, "blockerName");
> blocker->setStateReporter(...) // Optional
> // ...
> blocker->Remove();

I like option 1's API better, but I don't think we have to make it macro-based.  (Unless you were just using NS_GETSHUTDOWNBLOCKER to stand in for some getService-esque stuff.)

Comment 5

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

> Why is this?  Because the blocker is created inside the service (and that's where file/line come from) rather than a specific client location?

nsIAsyncShutdownClient::AddBlocker currently takes as argument __FILE__ and __LINE__. We need to find a way to pass them. One way is to leave them as arguments in a simplified method, then use a macro to pass them automatically.

Comment 6

[Nathan Froyd [:froydnj]]

Ah, hence the use of the macro in the proposed API.  That's fair.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Do shutdown blockers only work in the chrome process?

I wrote a patch for this and I've added blockers in both processes, but I'm only getting BlockShutdown called in the chrome process, not the content process.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Bug 1166293 - media::ShutdownBlocker helper for media shutdown code. WIP

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I've attached the patch showing the problem. Try a gUM call and breakpoint in BlockShutdown (or MediaManager::GetIfExists if your debugger as trouble breaking in the nested class).

Comment 10

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

The problem is that content processes never receive profile-before-change. I should be able to rig quickly a patch to let AsyncShutdown listen to content-child-shutdown, too.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Great!

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8674043 [details]
MozReview Request: Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8674043 [details]
MozReview Request: Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Randell, for now I've made MediaManager::Shutdown idempotent and call it from both "xpcom-will-shutdown" and the earlier ProfileBeforeChanged-added new BlockShutdown code in non-e10s.

Might this take care of the non-e10s leaks enough to land Bug 1198458?

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8674043 [details]
MozReview Request: Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

https://reviewboard.mozilla.org/r/22193/#review19837

I'm ok with shipping it as-is, but I think it can be improved a bit by specializing the Media::ShutdownBlocker.

::: dom/media/MediaManager.cpp:2667
(Diff revision 2)
> +    profileBeforeChange->RemoveBlocker(sSingleton->mShutdownBlocker);

Couldn't we roll the profileBeforeChange code into the MediaUtils helper, so this becomes something like:

sSingleton->mShutdownBlocker->Remove();

and the code above becomes:

nsresult rv = sSingleton->mShutdownBlocker->AddBlocker(NS_LITERAL_STRING(__FILE__), __LINE__, NS_LITERAL_STRING("Media shutdown"));

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/22193/#review19837

> Couldn't we roll the profileBeforeChange code into the MediaUtils helper, so this becomes something like:
> 
> sSingleton->mShutdownBlocker->Remove();
> 
> and the code above becomes:
> 
> nsresult rv = sSingleton->mShutdownBlocker->AddBlocker(NS_LITERAL_STRING(__FILE__), __LINE__, NS_LITERAL_STRING("Media shutdown"));

ProfileBeforeChange is one of 5 shutdown barriers, so baking it in entirely seemed limiting.

To store it got tricky since our code relies on removal working without BlockShutdown being called right now. There's no good identifier to pass into a constructor either.

I also used the JS semantics as a guide:

  AsyncShutdown.profileBeforeChange.removeBlocker(blocker);

It's the lack of exceptions that makes this 8 unreadable lines in c++ unfortunately.

Can we revisit this when we have a second user of it?

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8674043 [details]
MozReview Request: Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Fixed up try issues. Carrying forward r=jesup.

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8674043 [details]
MozReview Request: Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Rebased, and missed a try fix for Windows.

Comment 21

[Pulsebot]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b969d7c0af78