Closed Bug 1276383 Opened 4 years ago Closed 4 years ago

Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC

Categories

(Core :: WebRTC, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

My understanding is that WebRTC only really needs to do stuff at content-child-shutdown for leak checking. If that's really the case, then we can switch it over to xpcom-will-shutdown, and save the amount of work we do in non-debug builds in content process shutdown.
I'll do a try run to check, but jib, do you think it will be okay to not run any of this WebRTC shutdownPhase code in child processes, in non-leak-checking builds?
Flags: needinfo?(jib)
Thanks! I believe so, but checking with jesup to be sure.
Flags: needinfo?(jib) → needinfo?(rjesup)
It should be (I posted more complete analysis on another bug around the shutdown blockers a week or two ago).  We have to handle content crashes anyways.
Flags: needinfo?(rjesup)
This adds a new async shutdown barrier for xpcom-will-shutdown. This
runs at the very start of XPCom shutdown, which means it won't be run in content processes in non-leakchecking builds.

r?Yoric for the toolkit/components/asyncshutdown changes
r?jib for the dom/media/ changes

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41af1d0ec0ac&selectedJob=21611376
Attachment #8757636 - Flags: review?(jib)
Attachment #8757636 - Flags: review?(dteller)
Rank: 23
Priority: -- → P2
Comment on attachment 8757636 [details] [diff] [review]
Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC.

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

Are you sure that xpcom-will-shutdown is executed in content processes in release? If so, r=me.
Attachment #8757636 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #6)
> Are you sure that xpcom-will-shutdown is executed in content processes in
> release? If so, r=me.

I'm sure it will _not_ be executed in content processes in release. This is the same as xpcom-shutdown-thread (which is what I think is what is meant as per bug 1277052). What do you think?
Flags: needinfo?(dteller)
Blocks: 1277067
Comment on attachment 8757636 [details] [diff] [review]
Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC.

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

::: dom/media/MediaShutdownManager.cpp
@@ +55,5 @@
>    nsCOMPtr<nsIAsyncShutdownClient> barrier;
>    nsresult rv = svc->GetProfileBeforeChange(getter_AddRefs(barrier));
>    if (!barrier) {
>      // We are probably in a content process.
> +    rv = svc->GetXpcomWillShutdown(getter_AddRefs(barrier));

If this barrier is not executed in content processes, what's the point of this line?

Here and in two other places.

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ +1036,5 @@
>  }
>  
>  // All processes
>  this.AsyncShutdown.webWorkersShutdown = getPhase("web-workers-shutdown");
> +this.AsyncShutdown.xpcomWillShutdown = getPhase("xpcom-will-shutdown");

If it's only triggered in the parent process, please expose it only in the parent process.

::: toolkit/components/asyncshutdown/nsIAsyncShutdown.idl
@@ +210,5 @@
>  
>    /**
> +   * Barrier for notification xpcom-will-shutdown.
> +   */
> +  readonly attribute nsIAsyncShutdownClient xpcomWillShutdown;

If it's only triggered in the parent process, please move this to the "parent process" section.
Attachment #8757636 - Flags: review+ → feedback+
Flags: needinfo?(dteller)
xpcom-will-shutdown is run any time we run XPCOM shutdown. We run XPCOM shutdown in the parent process in both debug and opt builds. In the child process, we only run XPCOM shutdown in debug builds.

WebRTC only needs to use this for the child process, in debug builds, to perform cleanup for leak checking.

Does that make sense?
Flags: needinfo?(dteller)
Comment on attachment 8757636 [details] [diff] [review]
Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC.

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

lgtm.
Attachment #8757636 - Flags: review?(jib) → review+
(In reply to Andrew McCreight [:mccr8] from comment #9)
> xpcom-will-shutdown is run any time we run XPCOM shutdown. We run XPCOM
> shutdown in the parent process in both debug and opt builds. In the child
> process, we only run XPCOM shutdown in debug builds.

I don't want to expose a hook that never fires, as it's bound to cause problems sooner or later. So we need to publish it only in the parent process (in non-Debug builds) / in all processes (in Debug builds). Could you change that?

> 
> WebRTC only needs to use this for the child process, in debug builds, to
> perform cleanup for leak checking.
> 
> Does that make sense?

Your C++ code doesn't look like it's used only in Debug builds, so I'd prefer if you could #ifdef DEBUG it and document this debug-specific oddity.
Flags: needinfo?(dteller)
(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #11)
> Your C++ code doesn't look like it's used only in Debug builds, so I'd
> prefer if you could #ifdef DEBUG it and document this debug-specific oddity.

The problem with adding something like #ifdef NS_FREE_PERMANENT_DATA is that there are two MOZ_RELEASE_ASSERTs right afterwards, that would only be applicable in the parent process or child process debug builds. I'll just add a comment about what is going on.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e2bc431fa9
Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC. r=Yoric,jib
https://hg.mozilla.org/mozilla-central/rev/25e2bc431fa9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1278686
Attachment #8757526 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.