Closed Bug 1171722 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: mccr8, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

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?
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.
Assignee: nobody → continuation
Well, my quick hack to detect when we're in shutdown won't work.
Assignee: continuation → nobody
Are you okay with it if I just remove the warning? It doesn't seem too useful.
Flags: needinfo?(nfitzgerald)
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.
Flags: needinfo?(nfitzgerald)
Wait, can we just avoid posting the runnable if the reason is SHUTDOWN_CC?
(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.
(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.
Flags: needinfo?(continuation)
(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.
Flags: needinfo?(continuation) → needinfo?(terrence)
Yes, we should just pass the reason through. I'm surprised we aren't already.
Flags: needinfo?(terrence)
This adds the gcreason to the gc slice callback. Terrence can you take a look at the js/public and js/src portions?
Attachment #8620567 - Flags: review?(terrence)
Attachment #8620567 - Flags: review?(continuation)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
This adds a new reason for GC that happens during xpconnect shutdown. It will
be used in the next patch.
Attachment #8620570 - Flags: review?(terrence)
Attachment #8620571 - Flags: review?(continuation)
I'm going to wait until terrence okays the JS API changes before I review these, in case changes need to be made.
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.
Attachment #8620567 - Flags: review?(terrence) → review?(nfitzgerald)
Attachment #8620570 - Flags: review?(terrence) → review+
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.
Attachment #8620567 - Flags: review?(nfitzgerald) → review+
(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.
Attachment #8620567 - Attachment is obsolete: true
Attachment #8620567 - Flags: review?(continuation)
Comment on attachment 8620693 [details] [diff] [review]
Part 1: Add gcreason to GCSliceCallback

Updated per Terrence's suggestion. Carrying forward r+ from fitzgen.
Attachment #8620693 - Flags: review+
Attachment #8620575 - Attachment is obsolete: true
Attachment #8620575 - Flags: review?(continuation)
Alright, after updating the first patch it no longer needs mccr8's review. Fourth patch is updated to grab the reason from the GCDescription.
Attachment #8620571 - Flags: review?(continuation) → review+
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?
Attachment #8620694 - Flags: review?(continuation) → review+
You need to log in before you can comment on or make changes to this bug.