Closed Bug 1313864 Opened 7 years ago Closed 7 years ago

Make requestIdleCallback to use TaskQueue or similar to reduce the number of runnables it dispatches to the main event loop

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: smaug, Assigned: farre)

References

Details

Attachments

(2 files, 7 obsolete files)

0001-Bug-1313864-Move-running-idle-callbacks-to-executor..patch
26.75 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
0002-Bug-1313864-Use-the-timeout-handle-to-cancel-rIC-tim.patch
1.84 KB, patch
bkelly
: review+
Details | Diff | Splinter Review 
The current rIC patch calls NS_IdleDispatchToCurrentThread for each rIC call in foreground window. That may flood the idle queue in the main event loop.
Using TaskQueue would have just one or two runnables in main event loop.

Also, if the window goes away, we should be able to kill idle tasks asap. Using TaskQueue would help with that too - just clear the queue. (Not sure if TaskQueue supports that yet.)

Better to get bug 1300659 landed first though.
I agree. We just need to remember to keep the observable order of executed idle callbacks the same, i.e if we have several connected windows I think that they need to share an idle event TaskQueue.
bkelly will, I believe, use one TaskQueue per setTimeout/setInterval per each TabGroup.
(though I think will want eventually DocGroup).

And, does anything in the spec even require keeping the order between windows?
I see only "Append callback to window's list of idle request callbacks, associated with handle. ". That is per window.
Priority: -- → P2
So, ThrottledEventQueue currently only knows how to do normal dispatch.  Its an nsIEventTarget and IdleDispatch() is on nsThread.

I think the only effect this would have would be to ensure fair splitting of the idle time between different TabGroups.  Since nsThread already manages the idle queue as a low priority queue, I don't think it will have any effect on jank.

Given that, I'm not sure its a high priority.
Note, making background windows not use DelayedDispatch() for every idle callback would be nice, though.  However, if that code used the window setTimeout() infrastructure it would automatically get the ThrottledEventQueue.
This has nothing to do with jank. This is about having better way to close down windows. No need to leave possibly tons of runnables to idle queue.
And, as you say, this also prevents flooding, so similar thing you did for the setTimeout/setInterval
Can you explain what you mean by "better way to close down windows"?

Do you mean just throwing away the ThrottledEventQueue and all the runnables in it?  That is also something that is not supported today.  It could be added, I suppose, but I'm not sure we could safely throw away the top level TabGroup TEQ.  You might need to make an Idle specific TEQ.

Perhaps an alternative way to do this is to just make IdleRequest do this itself.  The nsGlobalWindow already has an ordered list of IdleRequests.  We could make an IdleRequest::Run() method call IdleDispatch() for the next Request.  Then all you have to do is cancel the one queued IdleRequest when you Suspend().  Closing the window would cancel the queue request and drop the array of remaining requests.

Normally I would not advocate duplicating logic, but I think the idle case is suitable different from where ThrottledTaskQueue is used today.
If we do want to use ThrottledEventQueue, we could perhaps create an nsIIdleEventTarget interface and mix it in.  That way not every nsIEventTarget would have to implement IdleDispatch().
(In reply to Ben Kelly [:bkelly] from comment #6)
> Can you explain what you mean by "better way to close down windows"?
> 
> Do you mean just throwing away the ThrottledEventQueue and all the runnables
> in it?
That. And I'm surprised it is not supported.


> Perhaps an alternative way to do this is to just make IdleRequest do this
> itself.  The nsGlobalWindow already has an ordered list of IdleRequests.  We
> could make an IdleRequest::Run() method call IdleDispatch() for the next
> Request.  Then all you have to do is cancel the one queued IdleRequest when
> you Suspend().  Closing the window would cancel the queue request and drop
> the array of remaining requests.
That sounds good. Simple.
(In reply to Olli Pettay [:smaug] from comment #8)
> > Do you mean just throwing away the ThrottledEventQueue and all the runnables
> > in it?
> That. And I'm surprised it is not supported.

AFAIK we don't drop anything from the main thread once its dispatched.  We always drain.  So the most compatible way to get stuff using ThrottledEventQueue was to make it work the same way.  I think we would probably get a lot of leaks and bustage if we tried to throw away runnables we had advertised as successfully dispatched.
I think I've then totally misunderstood your goals for the queue. I thought it would be for cases where web page ends up dispatching something to the event loop (and something which can be cancelled).
But fine, keeping it consistent with normal event loop handling is safe.
Assignee: nobody → afarre
Andreas, as part of this can you remove the delayed dispatch thing for background windows?  I think using a timer as if a timeout was set with a value of 0ms (immediate) would have the same effect, but you automatically get all Suspend()/Resume()/TaskQueue logic implemented for free.
Flags: needinfo?(afarre)
(In reply to Ben Kelly [:bkelly] from comment #11)
> Andreas, as part of this can you remove the delayed dispatch thing for
> background windows?  I think using a timer as if a timeout was set with a
> value of 0ms (immediate) would have the same effect, but you automatically
> get all Suspend()/Resume()/TaskQueue logic implemented for free.

Oh, that's neat. Will do!
Flags: needinfo?(afarre)
Blocks: 1198381, 1315260
No longer depends on: 1198381
Blocks: 1314959
Comment on attachment 8809067 [details]
Bug 1313864 - Make IdleRequest dispatch itself.

https://reviewboard.mozilla.org/r/91722/#review92124

::: dom/base/IdleRequest.cpp:76
(Diff revision 1)
>  
>    return rv;
>  }
>  
> +void
> +IdleRequest::Dispatch()

MOZ_ASSERT(NS_IsMainThread())

::: dom/base/IdleRequest.cpp:86
(Diff revision 1)
> +}
> +
>  nsresult
>  IdleRequest::Run()
>  {
> -  if (mCallback) {
> +  if (mRunning && mCallback) {

Shouldn't this just MOZ_ASSERT(mRunning)?  How do you legitimately get mRunning=false here?

::: dom/base/nsGlobalWindow.cpp:612
(Diff revision 1)
>      }
>    }
>  
>    nsGlobalWindow* outer = GetOuterWindowInternal();
>    if (outer && outer->AsOuter()->IsBackground()) {
> +    bool needScheduling = mThrottledIdleRequestCallbacks.isEmpty();

I'd really like to get rid of mThrottledIdleRequestCallbacks.  I think having separate lists to manage is confusing and error prone.

Can we simplify this by doing:

1. Only keep one list.
2. Make IdleRequest::Dispatch() call NS_IdleDispatchToCurrentThread() when the window is in the foreground.
3. Make IdleRequest::Dispatch() insert a 0ms timer that then calls NS_IdleDispatchToCurrentThread() when it fires.  The timer logic will force the 0ms to the background throttle amount.
4. Don't worry about converting the timer to an immediately idle dispatch when a window goes from background to foreground.

So in this model we always have an IdleRequest that is either idle dispatched or in a timer delay which leads to an idle dispatch.

When the window goes from background to foreground the timer will be reset to its original 0ms and trigger the idle dispatch ASAP.

When the window goes from foreground to background the next dispatch will begin delaying by the background throttle amount.

I think that would keep the majority of the logic the same regardless of foreground or background.
Attachment #8809067 - Flags: review?(bkelly) → review-
Comment on attachment 8809067 [details]
Bug 1313864 - Make IdleRequest dispatch itself.

https://reviewboard.mozilla.org/r/91722/#review92124

> Shouldn't this just MOZ_ASSERT(mRunning)?  How do you legitimately get mRunning=false here?

Sorry, this is prophetic programming. I know in this patch that I will be followed by 1315260, where mRunning will be set to false while suspended. I'll move mRunning over to that one instead.

> I'd really like to get rid of mThrottledIdleRequestCallbacks.  I think having separate lists to manage is confusing and error prone.
> 
> Can we simplify this by doing:
> 
> 1. Only keep one list.
> 2. Make IdleRequest::Dispatch() call NS_IdleDispatchToCurrentThread() when the window is in the foreground.
> 3. Make IdleRequest::Dispatch() insert a 0ms timer that then calls NS_IdleDispatchToCurrentThread() when it fires.  The timer logic will force the 0ms to the background throttle amount.
> 4. Don't worry about converting the timer to an immediately idle dispatch when a window goes from background to foreground.
> 
> So in this model we always have an IdleRequest that is either idle dispatched or in a timer delay which leads to an idle dispatch.
> 
> When the window goes from background to foreground the timer will be reset to its original 0ms and trigger the idle dispatch ASAP.
> 
> When the window goes from foreground to background the next dispatch will begin delaying by the background throttle amount.
> 
> I think that would keep the majority of the logic the same regardless of foreground or background.

Right, I'll have a go at this. I had the same idea, but didn't follow all the way through.
Comment on attachment 8809067 [details]
Bug 1313864 - Make IdleRequest dispatch itself.

https://reviewboard.mozilla.org/r/91722/#review92124

> Right, I'll have a go at this. I had the same idea, but didn't follow all the way through.

So I don't see any throttling happen. With the changes you suggest, a page with a rIC calling itself in the background gives 100% CPU usage.
Comment on attachment 8809067 [details]
Bug 1313864 - Make IdleRequest dispatch itself.

https://reviewboard.mozilla.org/r/91722/#review92300

::: dom/base/IdleRequest.cpp:141
(Diff revision 1)
>    mCallback->Call(*deadline, error, "requestIdleCallback handler");
>    mCallback = nullptr;
>    Release();
>  
> +  if (next) {
> +    next->Dispatch();

At this point 'next' could've been canceled and wouldn't be in the list. Dispatching it wouldn't hurt, but we would stop because dispatch wouldn't continue.
(In reply to Andreas Farre [:farre] from comment #16)
> > Right, I'll have a go at this. I had the same idea, but didn't follow all the way through.
> 
> So I don't see any throttling happen. With the changes you suggest, a page
> with a rIC calling itself in the background gives 100% CPU usage.

Do you have a patch with the changes?  I don't see it in the mozreview link.  (Although I barely know how to use mozreview, so maybe its there and I just can't find it.)
Comment on attachment 8809067 [details]
Bug 1313864 - Make IdleRequest dispatch itself.

https://reviewboard.mozilla.org/r/91722/#review92798

r=me with comments addressed.  Thanks!

::: dom/base/IdleRequest.cpp:80
(Diff revision 2)
>  }
>  
> +void
> +IdleRequest::Dispatch()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

MOZ_DIAGNOSTIC_ASSERT(!mRunning) to ensure we don't double-dispatch.

::: dom/base/IdleRequest.cpp:132
(Diff revision 2)
>  
>    if (!aDidTimeout) {
>      CancelTimeout();
>    }
>  
>    remove();

I think there is a bug here.  You remove the `IdleRequest` from the list before calling the js callback.  After the js callback you dispatch the next `IdleRequest`.

If the `remove()` empties the list and the js callback calls `requestIdleCallback()` then you will trigger another dispatch there.  Our final dispatch in this method will then double-dispatch that newly added request.

I think we should move the `remove()` down to just before `DispatchNextIdleRequest()`.

::: dom/base/RunnableTimeoutHandler.h:23
(Diff revision 2)
> +namespace dom {
> +
> +class RunnableTimeoutHandler final : public nsITimeoutHandler
> +{
> +public:
> +  RunnableTimeoutHandler(already_AddRefed<nsIRunnable> aRunnable);

explicit

::: dom/base/RunnableTimeoutHandler.cpp:17
(Diff revision 2)
> +namespace dom {
> +
> +RunnableTimeoutHandler::RunnableTimeoutHandler(
> +  already_AddRefed<nsIRunnable> aRunnable)
> +  : mRunnable(Move(aRunnable))
> +{

MOZ_ASSERT(mRunnable)

::: dom/base/nsGlobalWindow.cpp:571
(Diff revision 2)
> +  if (mIdleRequestCallbacks.isEmpty()) {
>      return;
> +  }
>  
> -  RefPtr<IdleRequest> request(mThrottledIdleRequestCallbacks.popFirst());
> -  // ownership transferred from mThrottledIdleRequestCallbacks to
> +  nsGlobalWindow* outer = GetOuterWindowInternal();
> +  if (outer && outer->AsOuter()->IsBackground()) {

Please add a comment that we are using the timeout mechanism here in order to throttle callbacks in background windows.

::: dom/base/nsGlobalWindow.cpp:578
(Diff revision 2)
> -  mIdleRequestCallbacks.insertBack(request);
> -  NS_IdleDispatchToCurrentThread(request.forget());
> +    RefPtr<RunnableTimeoutHandler> timeout(new RunnableTimeoutHandler(
> +      NewRunnableMethod(this, &nsGlobalWindow::DispatchIdleRequest)));
> +    SetTimeoutOrInterval(timeout, 0, false, Timeout::Reason::eInternalTimeout,
> +                         &dummy);
> +  } else {
> +    RefPtr<IdleRequest> request(mIdleRequestCallbacks.getFirst());

nit:  Please use early-return style here:

```
if (outer && outer->AsOuter()->IsBackground()) {
  // ...
  return;
}

RefPtr<IdleRequest> request(mIdleRequestCallbacks.getFirst());
request->Dispatch();
```

::: dom/base/nsGlobalWindow.cpp:612
(Diff revision 2)
>      if (NS_WARN_IF(aError.Failed())) {
>        return 0;
>      }
>    }
>  
> -  nsGlobalWindow* outer = GetOuterWindowInternal();
> +  bool needsScheduling = mIdleRequestCallbacks.isEmpty();

Maybe add a comment that we only need to schedule if the list is currently empty since otherwise there is already a runnable scheduled.

::: dom/base/nsGlobalWindow.cpp:12479
(Diff revision 2)
>      case Timeout::Reason::eTimeoutOrInterval:
> -    default:
>        return ++mTimeoutIdCounter;
> +    case Timeout::Reason::eInternalTimeout:
> +    default:
> +      return 0;

This changes the default case.  What uses that?  Is this safe?
Attachment #8809067 - Flags: review?(bkelly) → review+
Comment on attachment 8809067 [details]
Bug 1313864 - Make IdleRequest dispatch itself.

https://reviewboard.mozilla.org/r/91722/#review92798

> This changes the default case.  What uses that?  Is this safe?

I wanted to cover all cases. I'll add an unreachable assert.
Attachment #8809067 - Attachment is obsolete: true
Comment on attachment 8816149 [details]
Bug 1313864 - Move running idle callbacks to executor.

https://reviewboard.mozilla.org/r/96926/#review97322

::: dom/base/IdleRequest.cpp:21
(Diff revision 1)
>  #include "nsPIDOMWindow.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> -IdleRequest::IdleRequest(JSContext* aCx, nsPIDOMWindowInner* aWindow,
> +IdleRequest::IdleRequest(nsPIDOMWindowInner *aWindow,

Wrong change. * goes with type, not with argument name

::: dom/base/IdleRequest.cpp:22
(Diff revision 1)
>  
>  namespace mozilla {
>  namespace dom {
>  
> -IdleRequest::IdleRequest(JSContext* aCx, nsPIDOMWindowInner* aWindow,
> -                         IdleRequestCallback& aCallback, uint32_t aHandle)
> +IdleRequest::IdleRequest(nsPIDOMWindowInner *aWindow,
> +                         IdleRequestCallback &aCallback, uint32_t aHandle)

Same here with &

::: dom/base/RunnableTimeoutHandler.h:20
(Diff revision 1)
> +class nsIRunnable;
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class RunnableTimeoutHandler final : public nsITimeoutHandler

this needs some comment. What is this class for?

::: dom/base/Timeout.h:49
(Diff revision 1)
>  
> -  enum class Reason { eTimeoutOrInterval, eIdleCallbackTimeout };
> +  enum class Reason
> +  {
> +    eTimeoutOrInterval,
> +    eIdleCallbackTimeout,
> +    eInternalTimeout,

I don't understand what is eInternalTimeout.
Especially I don't understand when eIdleCallbackTimeout is supposed to be used and when eInternalTimeout.
If eInternalTimeout has something to do with idle stuff, it should hint about it in its name.

::: dom/base/nsGlobalWindow.h:1900
(Diff revision 1)
>    // the method that was used to focus mFocusedNode
>    uint32_t mFocusMethod;
>  
>    uint32_t mSerial;
>  
> -  void DisableIdleCallbackRequests();
> +  // The current internal timeout handle

What on earth is "internal timeout handle" ;)
This needs a good comment.

::: dom/base/nsGlobalWindow.cpp:564
(Diff revision 1)
> +{
> +public:
> +  explicit IdleRequestExecutor(nsGlobalWindow* aWindow)
> +    : mDispatched(false)
> +    , mDeadline(TimeStamp::Now())
> +    , mWindow(aWindow)

I would prefer if there was an assertion somewhere here to check that aWindow is inner window.

::: dom/base/nsGlobalWindow.cpp:635
(Diff revision 1)
> +  if (!mWindow) {
> +    return;
> +  }
> +
> +  if (aDeadline > mDeadline) {
> +    mAllowedHandle = mWindow->IdleRequestHandle();

Am I missing something here. You just assign mAllowedHandle once (and not even initialize it in ctor!), but it is never used anywhere.

::: dom/base/nsGlobalWindow.cpp:677
(Diff revision 1)
> +  if (outer && outer->AsOuter()->IsBackground()) {
> +    int32_t dummy;
> +    RefPtr<RunnableTimeoutHandler> timeout(new RunnableTimeoutHandler(
> +      NewRunnableMethod(mIdleRequestExecutor, &IdleRequestExecutor::Dispatch)));
> +    // We really want to re-use the mechanisms in SetTimeoutOrInterval
> +    // to handle throttling, but due to Bug 1316871 we can't do that

Isn't Bug 1316871 just waiting to land?
Attachment #8816149 - Flags: review?(bugs) → review-
Fixed nit.
Attachment #8816149 - Attachment is obsolete: true
Attachment #8816812 - Flags: review?(bkelly)
Comment on attachment 8816812 [details] [diff] [review]
0001-Bug-1316871-Throttle-background-setTimeouts.-r-bkell.patch

Attached patch to wrong bug. Sorry about that.
Attachment #8816812 - Attachment is obsolete: true
Attachment #8816812 - Flags: review?(bkelly)
Comment on attachment 8816149 [details]
Bug 1313864 - Move running idle callbacks to executor.

https://reviewboard.mozilla.org/r/96926/#review97322

> this needs some comment. What is this class for?

It's a way to wrap nsIRunnables and call SetTimeoutOrInterval. Will add comment.

> I don't understand what is eInternalTimeout.
> Especially I don't understand when eIdleCallbackTimeout is supposed to be used and when eInternalTimeout.
> If eInternalTimeout has something to do with idle stuff, it should hint about it in its name.

What I just realised is that eIdleCallbackTimeout isn't really necessary anymore. What we want is a 'Reason' for the above nsIRunnable wrapper that catches the internal uses of SetTimeoutOrInterval (for throttling, background handling and suspension).

So I'll remove eIdleCallbackTimeout and keep eInternalTimeout, since that captures the more general case.

> What on earth is "internal timeout handle" ;)
> This needs a good comment.

Will add a commont, hoping that it will be good :)

> Isn't Bug 1316871 just waiting to land?

Yep. Will use 0 instead.
So if eIdleCallbackTimeout is removed and eInternalTimeout kept, but only used by idle stuff, I think the name should hint something about 'idle'.
eInternalTimeout is really vague.
I agree that eInternalTimeout is really vague, and it's correct that so far only idle stuff would run an nsIRunnable from a SetTimeoutOrInterval. And for all we know that is the only meaningful instance where we would call it, so let's go with a name that has a hint of 'idle'.

Just to explain what will happen is that with this patch is that the timeout and background/suspend-resume-handling for idle requests would be a timeout of the same "type", with the same domain of id numbers. This is perfectly ok, since neither the ids of rIC timeouts or the ids of background/etc escape and become observable from outside of nsGlobalWindow.

Since the enum is used to map to which counter is used to get a new id, the name of the enum will also be reflected in the name of the id number member.

How about:

eIdleRequestTimeoutHandler/mIdleRequestTimeoutHandlerID?
Comment on attachment 8816149 [details]
Bug 1313864 - Move running idle callbacks to executor.

https://reviewboard.mozilla.org/r/96926/#review97842

::: dom/base/nsGlobalWindow.cpp:670
(Diff revision 2)
> -  // mIdleRequestCallbacks
> -  mIdleRequestCallbacks.insertBack(request);
> -  NS_IdleDispatchToCurrentThread(request.forget());
> +  }
> +
> +  nsPIDOMWindowOuter* outer = GetOuterWindow();
> +  if (outer && outer->AsOuter()->IsBackground()) {
> +    int32_t dummy;
> +    RefPtr<RunnableTimeoutHandler> timeout(new RunnableTimeoutHandler(

This is leaking. Need to have CCable runnable somewhere here.

::: dom/base/nsGlobalWindow.cpp:753
(Diff revision 2)
> -    new IdleRequest(aCx, AsInner(), aCallback, handle);
> +    new IdleRequest(AsInner(), aCallback, handle);
>  
>    if (aOptions.mTimeout.WasPassed()) {
> -    aError = request->SetTimeout(aOptions.mTimeout.Value());
> -    if (NS_WARN_IF(aError.Failed())) {
> +    int32_t timeoutHandle;
> +    RefPtr<RunnableTimeoutHandler> timeout(new RunnableTimeoutHandler(
> +      NewRunnableMethod<IdleRequest*, DOMHighResTimeStamp, bool>(

This too looks leaking
Attachment #8816149 - Flags: review?(bugs) → review-
Comment on attachment 8816149 [details]
Bug 1313864 - Move running idle callbacks to executor.

https://reviewboard.mozilla.org/r/96926/#review97322

Will have to drop the review and submit it again due to rebase. Sorry for the hassle.
Attachment #8816149 - Attachment is obsolete: true

        Attachment #8827543 -
        Attachment is obsolete: true

        Attachment #8827543 -
        Flags: review?(bugs)

        Attachment #8828817 -
        Flags: review?(bkelly)

    
    

    
  
Comment on attachment 8828817 [details] [diff] [review]
0001-Bug-1313864-Move-running-idle-callbacks-to-executor..patch

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

r=me with comments addressed

::: dom/base/IdleRequest.cpp
@@ +19,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +IdleRequest::IdleRequest(IdleRequestCallback& aCallback, uint32_t aHandle)
> +    : mCallback(&aCallback), mHandle(aHandle), mTimeoutHandle(Nothing()) {}

nit: I believe typical style is to put the initializations on separate lines.  This makes it easier to read diffs if any single initializer changes in the future.

Also, its unusual style to pass IdleRequestCallback& for a reference counted object.  We typically pass IdleRequestCallback* here.

Please add a MOZ_DIAGNOSTIC_ASSERT(mCallback) to verify its properly initialized.

@@ +53,1 @@
>    mCallback->Call(*deadline, error, "requestIdleCallback handler");

Can Run() get called more than once?  If not, perhaps we should clear mCallback here and do a MOZ_DIAGNOSTIC_ASSERT(mCallback) above.

@@ +54,2 @@
>  
>    return error.StealNSResult();

This doesn't handle exceptions thrown on the ErrorResult properly.  You need to call error.SuppressException().  Or you could use IgnoredErrorResult, but that might look weird since you do use the error code.

::: dom/base/IdleRequest.h
@@ +29,2 @@
>  
> +  nsresult Run(nsPIDOMWindowInner* aWindow, DOMHighResTimeStamp aDeadline,

Just to help differentiate this from a normal nsIRunnable::Run(), can you maybe name this IdleRun()?

@@ +48,2 @@
>    RefPtr<IdleRequestCallback> mCallback;
>    uint32_t mHandle;

mHandle can be const.

::: dom/base/TimeoutHandler.cpp
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +TimeoutHandler::TimeoutHandler(JSContext* aCx)

Can you initialize mLineNo and mColumn to zero here?  I realize that you are going to overwrite them immediately, but if something goes wrong in GetCallingLocation we should have known values.  In particular, GetCallingLocation() is fallible and can return false.

::: dom/base/nsGlobalWindow.cpp
@@ +523,4 @@
>    }
>  }
>  
> +class IdleRequestExecutor final : public nsIRunnable

It would be really nice to move the guts of idle callback handling into a separate file similar to what we did with TimeoutManager.  nsGlobalWindow is huge and we should really avoid putting a lot of new code in here if we can.  Can you file a follow-up to do that?

@@ +532,5 @@
> +    : mDispatched(false)
> +    , mDeadline(TimeStamp::Now())
> +    , mWindow(aWindow)
> +  {
> +    MOZ_ASSERT(aWindow->IsInnerWindow());

Perhaps strengthen this to:

MOZ_DIAGNOSTIC_ASSERT(mWindow);
MOZ_DIAGNOSTIC_ASSERT(mWindow->IsInnerWindow());

@@ +539,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(IdleRequestExecutor, nsIRunnable)
> +
> +  NS_DECL_NSIRUNNABLE
> +  nsresult Cancel() override;

NS_DECL_NSICANCELABLERUNNABLE

@@ +540,5 @@
> +  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(IdleRequestExecutor, nsIRunnable)
> +
> +  NS_DECL_NSIRUNNABLE
> +  nsresult Cancel() override;
> +  void SetDeadline(TimeStamp aDeadline) override;

NS_DECL_NSIINCREMENTALRUNNABLE

@@ +544,5 @@
> +  void SetDeadline(TimeStamp aDeadline) override;
> +
> +  void Dispatch();
> +private:
> +  ~IdleRequestExecutor() {

You don't need to explicitly clear mWindow here.  The RefPtr destructor effectively does that.

nit: curly brace on following line

@@ +589,5 @@
> +
> +nsresult
> +IdleRequestExecutor::Cancel()
> +{
> +  mWindow = nullptr;

MOZ_ASSERT(NS_IsMainThread()) here please.

@@ +597,4 @@
>  void
> +IdleRequestExecutor::SetDeadline(TimeStamp aDeadline)
> +{
> +  if (!mWindow) {

MOZ_ASSERT(NS_IsMainThread()) here please.

@@ +606,5 @@
> +
> +void
> +IdleRequestExecutor::Dispatch()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

Can we MOZ_DIAGNOSTIC_ASSERT(mWindow) here?

@@ +608,5 @@
> +IdleRequestExecutor::Dispatch()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!mDispatched) {

Please name this MaybeDispatch() to indicate its intended to be called multiple times and might have no effect.

Also, please use short-circuit style logic like:

if (mDispatched) {
  return;
}

@@ +662,5 @@
> +    mIdleRequestExecutor = new IdleRequestExecutor(this);
> +  }
> +
> +  nsPIDOMWindowOuter* outer = GetOuterWindow();
> +  if (outer && outer->AsOuter()->IsBackground()) {

Please add a comment here explaining that we are doing a "0 ms" timeout if we are a background window in order to get background timer throttling.

@@ +709,5 @@
> +  AssertIsOnMainThread();
> +  RefPtr<IdleRequest> request = mIdleRequestCallbacks.getFirst();
> +
> +  if (!request) {
> +    return NS_OK;

Maybe add a comment here indicating there are no more idle requests and we stop scheduling the executor runnable.

@@ +826,5 @@
>    }
>  
> +  while (!mIdleRequestCallbacks.isEmpty()) {
> +    RefPtr<IdleRequest> request = mIdleRequestCallbacks.getFirst();
> +    RemoveIdleCallbackFromList(request, mIdleRequestCallbacks);

I think you need to check for TimeoutManager handles and cancel the timeouts here, right?  Otherwise you might get timeout callbacks for removed requests.  Maybe the safest thing would be to cancel timeouts in RemoveIdleCallbackFromList().

::: dom/base/nsGlobalWindow.h
@@ +1927,5 @@
>  
>    uint32_t mSerial;
>  
> +  // The value for the next idle request timeout handler handle
> +  uint32_t mIdleRequestTimeoutHandlerCounter;

Is this initialized or used anywhere?

        Attachment #8828817 -
        Flags: review?(bkelly) → review+

    
    

    
  


  
I got some errors from try, and needed to make TimeoutHandler cycle collected, since the subclasses of TimeoutHandler were cycle collected.

Other than that, all review feedback should be taken care of except from adding NS_DECL_NSICANCELABLERUNNABLE and NS_DECL_NSIINCREMENTALRUNNABLE. Both nsICancellableRunnable and nsIIncrementalRunnable are internal interfaces without genrated NS_DECL_-macros.

        Attachment #8828817 -
        Attachment is obsolete: true

        Attachment #8829880 -
        Flags: review?(bkelly)

        Attachment #8829880 -
        Flags: review?(bkelly) → review+

    
    

    
  
Finally got a try push all the way through, and it looks ok:

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

    
    

    
  
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d8fb1244c5
Move running idle callbacks to executor. r=bkelly

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/a0d8fb1244c5
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox54:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

    
    

    
  
Backed out for causing bug 1334904.

https://hg.mozilla.org/integration/mozilla-inbound/rev/71224049c0b52ab190564d3ea0eab089a159a4cf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
It wouldn't be a bad idea to land this on m-c and respin Nightly. The crash is 61% of all crashes on the latest Windows Nightly.

    
    

    
  
Nightly respins are running on mozilla-central.
https://hg.mozilla.org/mozilla-central/rev/71224049c0b5
Target Milestone: mozilla54 → ---

    
    

    
  


  
Found and fixed the problem discovered in bug 1334889. Wrong handle was used to cancel timeouts. I'd be grateful if you could just give this a glance again Ben.

        Attachment #8829880 -
        Attachment is obsolete: true

        Attachment #8831977 -
        Flags: review?(bkelly)

    
    

    
  
Comment on attachment 8831977 [details] [diff] [review]
0001-Bug-1313864-Move-running-idle-callbacks-to-executor..patch

Sorry, can you separate this into two patches?  The P1 would be what was r+'d before and the new patch would be the change to fix the crash.  Thanks!

        Attachment #8831977 -
        Flags: review?(bkelly)

    
    

    
  


  
This is the same as the one that was r+ before.

        Attachment #8831977 -
        Attachment is obsolete: true

        Attachment #8832065 -
        Flags: review?(bkelly)

    
    

    
  


  
And this is P2. Sorry for the hassle.

        Attachment #8832066 -
        Flags: review?(bkelly)

    
    

    
  
Comment on attachment 8832066 [details] [diff] [review]
0002-Bug-1313864-Use-the-timeout-handle-to-cancel-rIC-tim.patch

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

Thanks!

        Attachment #8832066 -
        Flags: review?(bkelly) → review+

    
    

    
  
Comment on attachment 8832065 [details] [diff] [review]
0001-Bug-1313864-Move-running-idle-callbacks-to-executor..patch

Carry over previous r+.

        Attachment #8832065 -
        Flags: review?(bkelly) → review+

    
    

    
  
Again try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=404b4c47a4aaf26802d56830f3f051da78a76718
Let's have another go at committing.

    
    

    
  
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/923cece3a29d
Move running idle callbacks to executor. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/49e1691e4a0e
Use the timeout handle to cancel rIC timeout. r=bkelly

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/923cece3a29d
https://hg.mozilla.org/mozilla-central/rev/49e1691e4a0e
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago

          status-firefox54:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1315232
Version: 50 Branch → Trunk
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.