Closed Bug 1339289 Opened 4 years ago Closed 4 years ago

Give names to more runnables


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox54 --- fixed


(Reporter: billm, Assigned: billm)




(3 files)

These patches give names to more runnables. This will make it easier to prioritize which runnables to label.
Attached patch runnable namingSplinter Review
Naming of runnables.
Attachment #8836938 - Flags: review?(ehsan)
Attached patch timer namingSplinter Review
This adds names to function and interface timers.

One odd thing is that we have a lot of Unknown timers firing. Nothing actually seems to happen, so they're totally useless. I need to investigate what is going on there.
Attachment #8836939 - Flags: review?(ehsan)
Comment on attachment 8836938 [details] [diff] [review]
runnable naming

Review of attachment 8836938 [details] [diff] [review]:

::: js/xpconnect/src/XPCJSContext.cpp
@@ +3603,5 @@
>              // to support nested event loops implemented using a pattern like
>              // "while (condition) thread.processNextEvent(true)", in case the
>              // condition is triggered here by a Promise "then" callback.
> +            NS_DispatchToMainThread(new Runnable("Empty microtask runnable"));

All else being equal let's not use spaces in the names.  This kind of thing can throw off scripts that aren't too careful about spaces...
Attachment #8836938 - Flags: review?(ehsan) → review+
Comment on attachment 8836939 [details] [diff] [review]
timer naming

Review of attachment 8836939 [details] [diff] [review]:

::: dom/base/nsContentSink.cpp
@@ +1631,5 @@
> +
> +nsContentSink::GetName(nsACString& aName)
> +{
> +  aName.AssignASCII("nsContentSink timer");

Nit about spaces.

::: dom/events/EventStateManager.cpp
@@ +238,5 @@
> +UITimerCallback::GetName(nsACString& aName)
> +{
> +  aName.AssignASCII("UITimerCallback timer");

Here too

::: dom/media/DecoderDoctorDiagnostics.cpp
@@ +647,5 @@
> +DecoderDoctorDocumentWatcher::GetName(nsACString& aName)
> +{
> +  aName.AssignASCII("DecoderDoctorDocumentWatcher timer");

And here

::: dom/storage/StorageCache.cpp
@@ +263,5 @@
> +  GetName(nsACString& aName) override
> +  {
> +    aName.AssignASCII("StorageCacheHolder timer");

And here

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp
@@ +604,5 @@
> +PendingAlertsCollector::GetName(nsACString& aName)
> +{
> +  aName.AssignASCII("PendingAlertsCollector timer");

Attachment #8836939 - Flags: review?(ehsan) → review+
(In reply to Bill McCloskey (:billm) from comment #2)
> One odd thing is that we have a lot of Unknown timers firing. Nothing
> actually seems to happen, so they're totally useless. I need to investigate
> what is going on there.

Can you please elaborate what you meant here?  Did you mean that we fire timers that don't do any work?  That's really bad...
My assertion patch has the unfortunate problem that many labeled runnables lose their names. This patch fixes that. It also has the nice side effect of adding "(labeled)" to the end of the name of every labeled runnable. So now our telemetry will tell us which runnables are already done.
Attachment #8837329 - Flags: review?(ehsan)
I debugged the timers that weren't running anything. This seems to be caused by timers that are canceled after the runnable for them is dispatched but before the runnable has a chance to run.

I tried to see what was causing all the cancellations and I mostly saw Cancel() being called from here:

I think we could fix this, so I filed bug 1339632. For now, I just changed the name to show that it's a canceled timer.
Attachment #8837329 - Flags: review?(ehsan) → review+
Pushed by
Give names to a lot of common runnables (r=ehsan)
Give names to a lot of common timers (r=ehsan)
Make sure labeled runnables have names (r=ehsan)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.