Closed Bug 1378970 Opened 2 years ago Closed 2 years ago

label CollectorRunnable via its associated timer

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: froydnj, Assigned: bevis)

Details

Attachments

(2 files, 1 obsolete file)

All other associated timers in this file use the SystemGroup for a
GarbageCollection task, and this one should too.
Attachment #8884057 - Flags: review?(wmccloskey)
Comment on attachment 8884057 [details] [diff] [review]
label CollectorRunnable via its associated timer

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

This seems fine. However, we also need to deal with this case:
http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/dom/base/nsJSEnvironment.cpp#303

Currently, the way this works is pretty weird. The target will always be either null (which means we use the current thread) or the current thread (which comes in via NS_IdleDispatchToThread). We really want to use the SystemGroup here.

In my opinion we should just remove mTarget and the aTarget parameter to SetTimer. They're not helping anyone right now and they seem confusing to me. Then we can just use SetTarget(SystemGroup::...) like you have in the other callsite.
Attachment #8884057 - Flags: review?(wmccloskey) → review+
Rename as part 1.
Attachment #8884057 - Attachment is obsolete: true
Attachment #8887407 - Flags: review+
We can't just change the signature of SetTimer to remove aTarget parameter because it's an overridden method from nsIIdleRunnable and can be invoked indirectly from nsRefreshDriver::Tick() by calling NS_IdleDispatchToCurrentThread() [1] if CollectorRunner was scheduled by nsRefreshDriver::DispatchIdleRunnableAfterTick() at [2], so I defined SetTimerInternal() to SetTarget with SystemGroup and to set timout using mDelay directly to replace all the internal callsite to SetTimer() and as the internal implementation of ::SetTimer().

In addition, there is a minor issue to call SetTimer() twice unexpectedly when calling NS_IdleDispatchToCurrentThread() in original implementation which is also addressed in this patch.

[1] http://searchfox.org/mozilla-central/rev/88180977d79654af025158d8ebeb8c2aa11940eb/layout/base/nsRefreshDriver.cpp#2087
[2] http://searchfox.org/mozilla-central/rev/88180977d79654af025158d8ebeb8c2aa11940eb/dom/base/nsJSEnvironment.cpp#340
Assignee: nfroyd → btseng
Status: NEW → ASSIGNED
Attachment #8887413 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #5)
> and then avoid passing the target here:
> http://searchfox.org/mozilla-central/rev/
> 88180977d79654af025158d8ebeb8c2aa11940eb/xpcom/threads/nsThreadUtils.cpp#391
There is one concern here that avoids me to remove |aTarget| parameter from SetTarget():
What if |aThread| of NS_IdleDispatchToThread is not the same to GetCurrentThreadEventTarget() implicitly set in nsTimerImpl's ctor:
http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/xpcom/threads/nsTimerImpl.cpp#156
It could be unexpected to the user of IdleDispatchToThread() to run its timer callback on the calling thread instead of the |aThread| that user specified, although there is no such use case for now but it could be confusing from API perspective of NS_IdleDispatchToThread.

How do you think?
Flags: needinfo?(wmccloskey)
Comment on attachment 8887413 [details] [diff] [review]
(v1) Part 2: Label CollectorRunner::mTimer using SystemGroup.

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

You're right, I guess it might be better to keep this parameter for when we allow dispatching idle events to arbitrary threads.

::: dom/base/nsJSEnvironment.cpp
@@ -297,5 @@
> -    }
> -
> -    if (mTimer) {
> -      mTimer->SetTarget(mTarget);
> -      mTimer->InitWithNamedFuncCallback(TimedOut, this, aDelay,

I don't know how aDelay here differs from mDelay, but maybe it would be better to pass the delay to SetTimerInternal?
Attachment #8887413 - Flags: review?(wmccloskey) → review+
Flags: needinfo?(wmccloskey)
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8a372d113f
Part 1: Label CollectorRunner::mScheduleTimer using SystemGroup. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/bed655e34ed9
Part 2: Label CollectorRunner::mTimer using SystemGroup. r=billm
https://hg.mozilla.org/mozilla-central/rev/7d8a372d113f
https://hg.mozilla.org/mozilla-central/rev/bed655e34ed9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.