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

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

>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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 6

[Andreas Farre [:farre]]

Attachment: Bug 1315260 - Suspend idle callbacks.

Review commit: https://reviewboard.mozilla.org/r/91726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91726/

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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()?

Comment 8

[Andreas Farre [:farre]]

Comment on attachment 8809069 [details]
Bug 1315260 - Suspend idle callbacks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91726/diff/1-2/

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9211bfa02ed5
Ensure that we don't replace load popup. r=bkelly

Comment 11

[Andreas Farre [:farre]]

(In reply to Pulsebot from comment #10)
> Pushed by [e-mail]:
> 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.

Comment 12

[Andreas Farre [:farre]]

(In reply to Andreas Farre [:farre] from comment #11)
> (In reply to Pulsebot from comment #10)
> > Pushed by [e-mail]:
> > 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.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ddb12dd47ab
Revert rev 9211bfa02ed5 . Wrong commit message. r=me

Comment 15

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1315260-Test-that-rIC-doesn-t-run-for-documents-.patch

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Andreas, can you explain the condition this is testing for?

Comment 17

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1315260-Make-requesIdleCallbacks-suspend-along-w.patch

I dropped the previous review due to Bug 1313864 changes.

Comment 18

[Andreas Farre [:farre]]

(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.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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;

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 21

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1315260-Make-requesIdleCallbacks-suspend-along-w.patch

Removed rewrite debris.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 23

[Andreas Farre [:farre]]

(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.

Comment 24

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1315260-Make-requesIdleCallbacks-suspend-along-w.patch

Change order of resuming timeouts and idle callbacks.

Comment 25

[Andreas Farre [:farre]]

Attachment: 0003-Bug-1315260-Don-t-schedule-idle-callbacks-when-suspe.patch

Comment 26

[Andreas Farre [:farre]]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8894f4694536
https://hg.mozilla.org/mozilla-central/rev/55567871955e
https://hg.mozilla.org/mozilla-central/rev/5df1f8f9a5c7