1388664 - _saveStateAsync should be executed in idle dispatch

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Bao Quan [:beekill]]

Execute _saveStateAsync in idle dispatch to prevent the function is run when the time is inconvenient.

Please refer to bug 1353586 comment 0 and bug 1353586 comment 10.

Comment 1

[Bao Quan [:beekill]]

Attachment: Bug 1388664: Execute _saveStateAsync in idle callback.

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

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8895286 [details]
Bug 1388664: Execute _saveStateAsync in idle callback.

https://reviewboard.mozilla.org/r/166478/#review172248

Almost there! Please let me know if my comments below are in any way unclear. Thanks!

::: browser/components/sessionstore/SessionSaver.jsm:191
(Diff revision 1)
>      let interval = this._isIdle ? this._intervalWhileIdle : this._intervalWhileActive;
>      delay = Math.max(this._lastSaveTime + interval - Date.now(), delay, 0);
>  
>      // Schedule a state save.
>      this._wasIdle = this._isIdle;
> -    this._timeoutID = setTimeout(() => this._saveStateAsync(), delay);
> +    this._timeoutID = setTimeout(() => this.hiddenDOMWindow.requestIdleCallback(this._saveStateAsyncIdle), delay);

Please use `Services.appShell.hiddenDOMWindow`.

::: browser/components/sessionstore/SessionSaver.jsm:328
(Diff revision 1)
>  
>    /**
> +   * Execute _saveStateAsync when we have enough idle time. Otherwise, another idle request
> +   * is made to schedule _saveStateAsync again.
> +   * */
> +  _saveStateAsyncIdle(deadline) {

nit: s/_saveStateAsyncIdle/_saveStateAsyncWhenIdle/

::: browser/components/sessionstore/SessionSaver.jsm:329
(Diff revision 1)
>    /**
> +   * Execute _saveStateAsync when we have enough idle time. Otherwise, another idle request
> +   * is made to schedule _saveStateAsync again.
> +   * */
> +  _saveStateAsyncIdle(deadline) {
> +    if (deadline.timeRemaining() < 2) {

Please document above this line why you chose the magic number '2' in a comment.

::: browser/components/sessionstore/SessionSaver.jsm:330
(Diff revision 1)
> +   * Execute _saveStateAsync when we have enough idle time. Otherwise, another idle request
> +   * is made to schedule _saveStateAsync again.
> +   * */
> +  _saveStateAsyncIdle(deadline) {
> +    if (deadline.timeRemaining() < 2) {
> +      this.hiddenDOMWindow.requestIdleCallback(SessionSaverInternal._saveStateAsyncIdle);

My bet is you never encountered this case yourself, because `this` is referring to something other than `SessionSaverInternal` here, thus resulting in a TypeError to be thrown!
I suggest moving this code into the `setTimeout` block above just to keep all the hidden DOM window and idle callback magic contained in one place. You can move the helpful JSDoc comment with it, of course!

Comment 3

[Bao Quan [:beekill]]

Comment on attachment 8895286 [details]
Bug 1388664: Execute _saveStateAsync in idle callback.

https://reviewboard.mozilla.org/r/166478/#review172248

> Please document above this line why you chose the magic number '2' in a comment.

After think about it, I decided to change the time remaining should be larger than 5ms. According to [1], it takes around 5.9ms (median) to collect all windows and tabs data. My hope is that this change will benefit at least 50% of our users.

[1]: https://mzl.la/2ufqyBd

> My bet is you never encountered this case yourself, because `this` is referring to something other than `SessionSaverInternal` here, thus resulting in a TypeError to be thrown!
> I suggest moving this code into the `setTimeout` block above just to keep all the hidden DOM window and idle callback magic contained in one place. You can move the helpful JSDoc comment with it, of course!

Thanks Mike! I should have noticed that case earlier.

Comment 4

[Bao Quan [:beekill]]

Comment on attachment 8895286 [details]
Bug 1388664: Execute _saveStateAsync in idle callback.

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

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8895286 [details]
Bug 1388664: Execute _saveStateAsync in idle callback.

https://reviewboard.mozilla.org/r/166478/#review173366

Looks great! Please trigger a try build first before landing, just to be sure!

Comment 6

[Bao Quan [:beekill]]

Comment on attachment 8895286 [details]
Bug 1388664: Execute _saveStateAsync in idle callback.

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

Comment 7

[Bao Quan [:beekill]]

(In reply to Bao Quan [:beekill] from comment #6)
> Comment on attachment 8895286 [details]
> Bug 1388664: Execute _saveStateAsync in idle callback.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/166478/diff/2-3/

I changed the patch so it can pass tests: browser_not_collect_while_idle.js and browser_backup_recovery.js. The reason those test failed was because there is still a pending session save in idle callback. To fix this, I added `cancelIdleCallback` to `SessionSaverInternal.cancel`. I'll request a review from Mike so you can take a look at my change.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8895286 [details]
Bug 1388664: Execute _saveStateAsync in idle callback.

https://reviewboard.mozilla.org/r/166478/#review176472

::: browser/components/sessionstore/SessionSaver.jsm:219
(Diff revision 3)
>     * Cancels all pending session saves.
>     */
>    cancel() {
>      clearTimeout(this._timeoutID);
>      this._timeoutID = null;
> +    Services.appShell.hiddenDOMWindow.cancelIdleCallback(this._idleCallbackID);

oh, this is something `Services.tm.idleDispatchToMainThread()` doesn't provide... So good that you weren't using it here ;)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0f61a91f5af
Execute _saveStateAsync in idle callback. r=mikedeboer

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b0f61a91f5af