Closed Bug 1166293 Opened 6 years ago Closed 6 years ago

provide an easier way to do simple shutdown blockers

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: jib)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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?
Flags: needinfo?(dteller)
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?
Flags: needinfo?(dteller) → needinfo?(nfroyd)
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?
Flags: needinfo?(nfroyd) → needinfo?(dteller)
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();
Flags: needinfo?(dteller) → needinfo?(nfroyd)
(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.)
Flags: needinfo?(nfroyd)
> 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.
Ah, hence the use of the macro in the proposed API.  That's fair.
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.
Flags: needinfo?(dteller)
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).
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.
Flags: needinfo?(dteller)
Great!
Blocks: 1198458
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.
Attachment #8674043 - Attachment description: MozReview Request: Bug 1166293 - media::ShutdownBlocker helper for media shutdown code. WIP → MozReview Request: Bug 1166293 - Use AsyncShutdown API to shut down media thread in non-e10s.
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.
Attachment #8674043 - Flags: review?(rjesup)
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 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"));
Attachment #8674043 - Flags: review?(rjesup) → review+
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?
Assignee: nobody → jib
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.
Fixed up try issues. Carrying forward r=jesup.
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.
Rebased, and missed a try fix for Windows.
https://hg.mozilla.org/mozilla-central/rev/b969d7c0af78
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
See Also: → 1270308
You need to log in before you can comment on or make changes to this bug.