1276383 - Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC.

Comment 2

[Andrew McCreight [:mccr8]]

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?

Comment 3

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

Thanks! I believe so, but checking with jesup to be sure.

Comment 4

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

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.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC.

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

Comment 6

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

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.

Comment 7

[Andrew McCreight [:mccr8]]

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

Comment 8

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

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.

Comment 9

[Andrew McCreight [:mccr8]]

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?

Comment 10

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

Comment on attachment 8757636 [details] [diff] [review]
Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC.

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

lgtm.

Comment 11

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

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

Comment 12

[Andrew McCreight [:mccr8]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e2bc431fa9
Add nsIAsyncShutdown.xpcomWillShutdown and use it in WebRTC. r=Yoric,jib

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25e2bc431fa9