Closed Bug 1315260 Opened 3 years ago Closed 3 years ago

requestIdleCallback() can fire for documents in bfcache, during sync xhr, etc.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: farre)

References

Details

Attachments

(3 files, 3 obsolete files)

Currently requestIdleCallback() does not check to see if the window is allowed to fire callbacks.  It can inadvertently execute js code for windows in the bfcache or during a sync xhr, etc.

The easiest way to do this is call IdleRequest::Cancel() on all IdleRequest objects in mIdleRequestCallbacks and mThrottledIdleRequestCallbacks when nsGlobalWindow::Suspend() stop processing.  The callbacks then need to be re-activated in nsGlobalWindow::Resume().

This should make requestIdleCallback respect bfcache and all sync/modal states automatically.

We should also add a WPT test that does:

1) Create a frame
2) Frame calls requestIdleCallback()
3) Navigate frame (so it goes in bfcache)
4) Outer window does requestIdleCallback()
5) When outer window callback fires confirm inner frame's callback has not fired
6) Restore frame out of bfcache
7) Verify inner frame's callback fires
We should make sure the spec is sane here, too.  Especially for the "navigated away from" case; for sync XHR I expect the spec works via not processing events during it.
>1) Create a frame
>2) Frame calls requestIdleCallback()
>3) Navigate frame (so it goes in bfcache)
>4) Outer window does requestIdleCallback()
>5) When outer window callback fires confirm inner frame's callback has not fired

Note that I expect such a test would pass for us right now, if step 2 uses a function from inside the frame as the callback, because CallbackObject will not execute callbacks from a navigated-away-from window.  But if step 2 uses a function from the parent, I expect it would fail.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1)
> We should make sure the spec is sane here, too.  Especially for the
> "navigated away from" case; for sync XHR I expect the spec works via not
> processing events during it.

I believe this is implemented via a combination of nsGlobalWindow::Suspend() and nsDocument::SuppressEventHandling().  For example, requestAnimationFrame() is stopped during sync/modal/bfcache by SuspressEventHandling().

Currently I don't see code anywhere to suppress or suspend idle callbacks.  I think nsGlobalWindow::Suspend() is the easiest place to do this since the data structures are on the window.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> >1) Create a frame
> >2) Frame calls requestIdleCallback()
> >3) Navigate frame (so it goes in bfcache)
> >4) Outer window does requestIdleCallback()
> >5) When outer window callback fires confirm inner frame's callback has not fired
> 
> Note that I expect such a test would pass for us right now, if step 2 uses a
> function from inside the frame as the callback, because CallbackObject will
> not execute callbacks from a navigated-away-from window.  But if step 2 uses
> a function from the parent, I expect it would fail.

Hmm, but I don't think we would re-queue the callback after coming out of the bfcache which would make the end of the test fail.  Right?
> but I don't think we would re-queue the callback after coming out of the
> bfcache which would make the end of the test fail

Correct.
Assignee: nobody → afarre
Priority: -- → P1
Depends on: 1316289
Depends on: 1313864
Comment on attachment 8809069 [details]
Bug 1315260 - Suspend idle callbacks.

https://reviewboard.mozilla.org/r/91726/#review92134

::: dom/base/nsGlobalWindow.cpp:575
(Diff revision 1)
> +
> +  mIdleRequestCallbacks.getFirst()->Suspend();
> +  int32_t dummy;
> +  RefPtr<RunnableTimeoutHandler> timeout(new RunnableTimeoutHandler(
> +    NewRunnableMethod(this, &nsGlobalWindow::ResumeIdleCallbacks)));
> +  SetTimeoutOrInterval(timeout, 0, false, Timeout::Reason::eInternalTimeout,

Why do this instead of just starting the idle dispatch pump again in ::Resume()?
Attachment #8809069 - Flags: review?(bkelly) → review-
Comment on attachment 8809069 [details]
Bug 1315260 - Suspend idle callbacks.

https://reviewboard.mozilla.org/r/91726/#review93554

Sorry, I don't think resume is handled right here.

You may want to write a test that uses sync xhr or showModalDialog() to verify that the idle callbacks don't fire while suspended, but do fire after the window is resumed.

::: dom/base/nsGlobalWindow.h:1870
(Diff revision 2)
>    uint32_t mSerial;
>  
>    void DisableIdleCallbackRequests();
>  
> +  void SuspendIdleCallbacks();
> +  void ResumeIdleCallbacks();

ResumeIdleCallbacks() is neither defined nor called?

::: dom/base/nsGlobalWindow.cpp:11915
(Diff revision 2)
>        ac->AddWindowListener(mEnabledSensors[i], this);
>    }
>    EnableGamepadUpdates();
>    EnableVRUpdates();
>  
> +  DispatchNextIdleRequest();

This seems inadequate given all the IdleRequests have mRunning=false at the moment.
Attachment #8809069 - Flags: review?(bkelly) → review-
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9211bfa02ed5
Ensure that we don't replace load popup. r=bkelly
(In reply to Pulsebot from comment #10)
> Pushed by afarre@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9211bfa02ed5
> Ensure that we don't replace load popup. r=bkelly

Messed up the bug id for this patch.
(In reply to Andreas Farre [:farre] from comment #11)
> (In reply to Pulsebot from comment #10)
> > Pushed by afarre@mozilla.com:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/9211bfa02ed5
> > Ensure that we don't replace load popup. r=bkelly
> 
> Messed up the bug id for this patch.

As in that it should've been Bug 1316871 as well.
Andreas, can you back out the one patch and reland it with the correct bug number?  I know its a nuisance, but otherwise some of our release engineering scripts won't handle the patches correctly.
Flags: needinfo?(afarre)
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ddb12dd47ab
Revert rev 9211bfa02ed5 . Wrong commit message. r=me
Flags: needinfo?(afarre)
Andreas, can you explain the condition this is testing for?
Flags: needinfo?(afarre)
I dropped the previous review due to Bug 1313864 changes.
Attachment #8809069 - Attachment is obsolete: true
Attachment #8835037 - Flags: review?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #16)
> Andreas, can you explain the condition this is testing for?

So, we want to test that:

1) When we navigate away from a page with idle callbacks, those callbacks get suspended. This is trickier than it looks since a rIC can fire very quickly, so we run recursive idle callbacks up until the very point that we get the load for the new page. That happens when we set 'idleCalled' to false after getting the page load from after navigateWindow. If the idle callbacks would continue running idleCalled would be set to true and the assert following would fail (this happens currently!).

2) When we return to that page the idle callbacks should start up again if the user agent has a bfcache (event.persisted = true, and I've checked that we actually get event.persisted == true on issuing history.back()), otherwise we should just get the load event. 

Does that sound reasonable? I'm happy to write something like the above in the test if that helps.
Flags: needinfo?(afarre)
Attachment #8835028 - Flags: review?(bkelly) → review?(bugs)
Attachment #8835037 - Flags: review?(bkelly) → review?(bugs)
Comment on attachment 8835028 [details] [diff] [review]
0001-Bug-1315260-Test-that-rIC-doesn-t-run-for-documents-.patch

withEventListener could use once: true event listeners, then there wasn't need to remove event listeners


>+  function goBack(win) {
>+    win.history.back();
>+    return withEventListener(win, 'pagehide');
>+  }
Don't you want to add the event listener before calling win.history.back()
Or is it guaranteed back() is async here?

So, 
var p = withEventListener(win, 'pagehide');
win.history.back();
return p;
Attachment #8835028 - Flags: review?(bugs) → review+
Comment on attachment 8835037 [details] [diff] [review]
0002-Bug-1315260-Make-requesIdleCallbacks-suspend-along-w.patch

>@@ -547,6 +547,9 @@ public:
>   void SetDeadline(TimeStamp aDeadline) override;
> 
>   void MaybeDispatch();
>+
>+  void Suspend();
>+  void Resume();
what are these unimplemented methods?

>@@ -12112,6 +12132,8 @@ nsGlobalWindow::Suspend()
> 
>   mozilla::dom::workers::SuspendWorkersForWindow(AsInner());
> 
>+  SuspendIdleRequests();
>+
>   mTimeoutManager->Suspend();
> 
>   // Suspend all of the AudioContexts for this window
>@@ -12164,6 +12186,8 @@ nsGlobalWindow::Resume()
>     RefPtr<Promise> d = mAudioContexts[i]->Resume(dummy);
>   }
> 
>+  ResumeIdleRequests();
>+
>   mTimeoutManager->Resume();
Nit, could we possible resume idle requests after normal timeouts

r- because of those unimplemented methods. Is the patch missing something or are those just leftovers from some previous version of the patch?
Attachment #8835037 - Flags: review?(bugs) → review-
Removed rewrite debris.
Attachment #8835037 - Attachment is obsolete: true
Attachment #8835537 - Flags: review?(bugs)
Comment on attachment 8835537 [details] [diff] [review]
0002-Bug-1315260-Make-requesIdleCallbacks-suspend-along-w.patch

I still think IdleRequests should be resumed after normal timeouts, just in case some timer ordering matters.
Attachment #8835537 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8835537 [details] [diff] [review]
> 0002-Bug-1315260-Make-requesIdleCallbacks-suspend-along-w.patch
> 
> I still think IdleRequests should be resumed after normal timeouts, just in
> case some timer ordering matters.

I'll make it so.
Change order of resuming timeouts and idle callbacks.
Attachment #8835537 - Attachment is obsolete: true
Attachment #8835904 - Flags: review?(bugs)
Attachment #8835904 - Flags: review?(bugs) → review+
Attachment #8836639 - Flags: review?(bugs) → review+
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8894f4694536
Test that rIC doesn't run for documents in bfcache. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/55567871955e
Make requesIdleCallbacks suspend along with nsGlobalWindow. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df1f8f9a5c7
Don't schedule idle callbacks when suspended. r=smaug
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.