_saveStateAsync should be executed in idle dispatch

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Session Restore
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: beekill, Assigned: beekill)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
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.
(Assignee)

Updated

7 months ago
Blocks: 1353586
Comment hidden (mozreview-request)
Assignee: nobody → nnn_bikiu0707
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 2

7 months ago
mozreview-review
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!
Attachment #8895286 - Flags: review?(mdeboer) → review-
(Assignee)

Comment 3

6 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 5

6 months ago
mozreview-review
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!
Attachment #8895286 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

6 months ago
(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.
(Assignee)

Updated

6 months ago
Attachment #8895286 - Flags: review+ → review?(mdeboer)

Comment 8

6 months ago
mozreview-review
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 ;)
Attachment #8895286 - Flags: review?(mdeboer) → review+

Comment 9

6 months ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0f61a91f5af
Execute _saveStateAsync in idle callback. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/b0f61a91f5af
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.