1171722 - Don't warn if DebuggerOnGCRunnable::Enqueue() fails during shutdown

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

We get this warning message every time we do a shutdown CC, which is obnoxious. Maybe DebuggerOnGCRunnable::Enqueue could detect if we're in shutdown and just return NS_OK immediately?

Comment 1

[Eric Rahm [:erahm]]

For reference, WARNING: 'NS_FAILED(DebuggerOnGCRunnable::Enqueue(aRuntime, aDesc))' shows up 2,433 times during testing of a debug linux64 build. During a local run of opening and closing the browser to a blank page I saw 10 instances.

Comment 2

[Andrew McCreight [:mccr8]]

Well, my quick hack to detect when we're in shutdown won't work.

Comment 3

[Andrew McCreight [:mccr8]]

Are you okay with it if I just remove the warning? It doesn't seem too useful.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I guess :-/

Swallowing an error and not reporting it, doesn't feel great to me, but I totally get that this is really annoying on shutdown.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Wait, can we just avoid posting the runnable if the reason is SHUTDOWN_CC?

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #5)
> Wait, can we just avoid posting the runnable if the reason is SHUTDOWN_CC?

Oh, right, I forgot that was one of the reasons. I'll see if that fixes it all.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #5)
> > Wait, can we just avoid posting the runnable if the reason is SHUTDOWN_CC?
> 
> Oh, right, I forgot that was one of the reasons. I'll see if that fixes it
> all.

So what needs to be done here? I can do the legwork if needs be, but it's not clear to me where to get the reason from.

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Eric Rahm [:erahm] from comment #7)
> So what needs to be done here? I can do the legwork if needs be, but it's
> not clear to me where to get the reason from.

Yeah, that's what stymied me, too. There's a string, but doing a string comparison seems kind of gross. Terrence, is there some way to figure out here what our GC trigger is? Should some additional info be added to this interface? We're trying to figure out if this is a shutdown GC, so we can just not dispatch this thing rather than cause an error.

Comment 9

[Terrence Cole [:terrence]]

Yes, we should just pass the reason through. I'm surprised we aren't already.

Comment 10

[Eric Rahm [:erahm]]

Attachment: Part 1: Add gcreason to GCSliceCallback.

This adds the gcreason to the gc slice callback. Terrence can you take a look at the js/public and js/src portions?

Comment 11

[Eric Rahm [:erahm]]

Attachment: Part 2: Add gcreason::XPCONNECT_SHUTDOWN

This adds a new reason for GC that happens during xpconnect shutdown. It will
be used in the next patch.

Comment 12

[Eric Rahm [:erahm]]

Attachment: Part 3: Specify gcreason::XPCONNECT_SHUTDOWN during xpconnect shutdown

Comment 13

[Eric Rahm [:erahm]]

Attachment: Part 4: Don't warn if DebuggerOnGCRunnable::Enqueue fails during shutdown

Comment 14

[Andrew McCreight [:mccr8]]

I'm going to wait until terrence okays the JS API changes before I review these, in case changes need to be made.

Comment 15

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd85caa999b0

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 8620567 [details] [diff] [review]
Part 1: Add gcreason to GCSliceCallback.

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

I was actually thinking we'd want to add the reason to the GCDescription.

This is more fitzgen's area, in any case.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8620567 [details] [diff] [review]
Part 1: Add gcreason to GCSliceCallback.

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

What Terrence said. r=me for the js parts with a reason member on GCDescription.

Comment 18

[Eric Rahm [:erahm]]

(In reply to Terrence Cole [:terrence] from comment #16)
> Comment on attachment 8620567 [details] [diff] [review]
> Part 1: Add gcreason to GCSliceCallback.
> 
> Review of attachment 8620567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was actually thinking we'd want to add the reason to the GCDescription.
> 

Make sense, as a bonus that removes the need to change the callback signature which will make this a smaller patch.

Comment 19

[Eric Rahm [:erahm]]

Attachment: Part 1: Add gcreason to GCSliceCallback

Comment 20

[Eric Rahm [:erahm]]

Comment on attachment 8620693 [details] [diff] [review]
Part 1: Add gcreason to GCSliceCallback

Updated per Terrence's suggestion. Carrying forward r+ from fitzgen.

Comment 21

[Eric Rahm [:erahm]]

Attachment: Part 4: Don't warn if DebuggerOnGCRunnable::Enqueue fails during shutdown

Comment 22

[Eric Rahm [:erahm]]

Alright, after updating the first patch it no longer needs mccr8's review. Fourth patch is updated to grab the reason from the GCDescription.

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 8620694 [details] [diff] [review]
Part 4: Don't warn if DebuggerOnGCRunnable::Enqueue fails during shutdown

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

Thanks for fixing this!

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +741,5 @@
>  
>    if (aProgress == JS::GC_CYCLE_END) {
> +    JS::gcreason::Reason reason = aDesc.reason_;
> +    NS_WARN_IF(NS_FAILED(DebuggerOnGCRunnable::Enqueue(aRuntime, aDesc)) &&
> +               !(reason == JS::gcreason::SHUTDOWN_CC ||

Maybe change this to a sequence of |reason != ... &&| to avoid the nesting?

Comment 24

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a834acad176c

Comment 25

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/459140d723e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/347e2838d028
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e186e706653
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d032cbecee

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/459140d723e8
https://hg.mozilla.org/mozilla-central/rev/347e2838d028
https://hg.mozilla.org/mozilla-central/rev/2e186e706653
https://hg.mozilla.org/mozilla-central/rev/b7d032cbecee