1305950 - Don't collect/write Session Restore when user is idle

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Spinoff from bug 1304389.

Users expect Session Restore to store things that take place while they are using Firefox. If a page does stuff in the background, they don't really care. Differentiating both would be good for battery and disk life.

Comment 1

[YF (Yang)]

Might better if "when it is not modified"?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

That's a different bug :)

This one is about pages doing changes that trigger Session Restore (e.g. modifying Session Cookies, Session Storage, hidden forms, causing refreshes, ...) while the user is not in front of the computer.

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #2)
> This one is about pages doing changes that trigger Session Restore (e.g.
> modifying Session Cookies, Session Storage, hidden forms, causing refreshes,
> ...) while the user is not in front of the computer.

Well, we can invent our own sessionstore-specific heuristics or we can simply use nsIIdleService?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

I don't see a compelling reason to use something other than nsIIdleService.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

I'll see if I can produce a quick patch.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1305950 - Don't collect/save the session when the user is idle;

When the user is not actively using the computer, webpages may still
perform changes that require rewriting Session Restore, e.g.  updating
Session Cookies or DOM Session Storage, or refreshing, etc. Before
this patch, a single active page can require us to
recollect/serialize/write the entire Session Restore file every 15
seconds even when the user is not in front of the computer.

We expect that, when the user is not in front of the computer, changes
are not critical and don't need to be saved as often. We now adopt the
following strategy:

- when the user has been away for (by default) 15 seconds, finish any
  pending collect/write, then increase the collect/write buffering
  delay to (by default) 1h

- when the user returns, reschedule any pending 1h collect/write as a
  (by default) 15 seconds collect/write, then proceed with (by
  default) 15 seconds collect/write delays.

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

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Note that I haven't come up with a good test yet. I'll try and do that.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

https://reviewboard.mozilla.org/r/81672/#review80266

Just a few drive by. FWIW, this is quite close to what I had in mind.

::: browser/components/sessionstore/SessionSaver.jsm:35
(Diff revision 1)
>    "resource:///modules/sessionstore/SessionFile.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
>    "resource://gre/modules/PrivateBrowsingUtils.jsm");
>  
> -// Minimal interval between two save operations (in milliseconds).
> -XPCOMUtils.defineLazyGetter(this, "gInterval", function () {
> +var idleService = Cc["@mozilla.org/widget/idleservice;1"].
> +  getService(Ci.nsIIdleService);

Just use Services.idle where needed?

::: browser/components/sessionstore/SessionSaver.jsm:326
(Diff revision 1)
>      }, console.error);
>    },
>  };
> +
> +
> +SessionSaverInternal._intervalActive = Preferences.get(PREF_INTERVAL_ACTIVE, 15000 /* 15 seconds */);

nit: you could extend XPCOMUtils.defineLazyPreferenceGetter with an optional method invoked when the pref changes.

::: browser/components/sessionstore/SessionSaver.jsm:346
(Diff revision 1)
> +
> +Preferences.observe(PREF_INTERVAL_IDLE, interval => {
> +  SessionSaverInternal._intervalIdle = interval;
> +}, false);
> +
> +idleService.addIdleObserver(SessionSaverInternal, SessionSaverInternal._intervalActive);

15s of idle is really short, I'd personally use a multiple of the active time, but not less than a minute.
I don't have a clear evidence the current code may cause problems, but when we get "active" we do some work and generate some garbage, that makes me think we should actually try to detect a "real" idle, rather than the user reading a paragraph of an article.
The benefit should still be tangible and we won't have to act so often. As I said I'd not go for anything shorter than 1 minute.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

https://reviewboard.mozilla.org/r/81672/#review80266

> Just use Services.idle where needed?

I was convinced that `Services.idle` existed, but I don't see it in `Services.jsm`. I suppose I could add it, of course.

> nit: you could extend XPCOMUtils.defineLazyPreferenceGetter with an optional method invoked when the pref changes.

I was not aware of this function, thanks. It's a bit counter-intuitive to use a `defineLazy` for something that I'm using immediately, but it looks like it would do the trick.

> 15s of idle is really short, I'd personally use a multiple of the active time, but not less than a minute.
> I don't have a clear evidence the current code may cause problems, but when we get "active" we do some work and generate some garbage, that makes me think we should actually try to detect a "real" idle, rather than the user reading a paragraph of an article.
> The benefit should still be tangible and we won't have to act so often. As I said I'd not go for anything shorter than 1 minute.

I'm fine with any duration. We can even make it another preference, to ease testing.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1305950 (Part 1)- Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change;

This is a pretty common use case of preferences.

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

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1305950 - Add Services.idle;

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

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

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

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8795715 [details]
Bug 1305950 - Add Services.idle;

https://reviewboard.mozilla.org/r/81684/#review80298

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

https://reviewboard.mozilla.org/r/81672/#review80300

::: browser/components/sessionstore/SessionSaver.jsm:344
(Diff revision 2)
> +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_intervalWhileIdle", PREF_INTERVAL_IDLE,
> +  3600000 /* 1 h */);
> +
> +
> +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_idleDelay", PREF_IDLE_DELAY,
> +  300000 /* 5 minutes */,

Note that due to bug 973591 we may get "spurious" active events between 2 to 5 minutes from when idle started. So the shorter the delay, the better is. I'd suggest to go for 3 minutes, that is a good compromise between not-too-short and may-never-fire.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

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

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795714 [details]
Bug 1305950 (Part 1)- Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change;

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

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795715 [details]
Bug 1305950 - Add Services.idle;

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

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

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

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795714 [details]
Bug 1305950 (Part 1)- Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change;

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

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795715 [details]
Bug 1305950 - Add Services.idle;

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

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8795714 [details]
Bug 1305950 (Part 1)- Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change;

https://reviewboard.mozilla.org/r/81682/#review80404

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

https://reviewboard.mozilla.org/r/81672/#review80608

Thanks David, for picking this up! I'm looking forward to seeing the test :) I think the Places component has had to figure out testing the idle observer already (I think it's mocked there), so you might find it useful drawing inspiration from there.

::: browser/components/sessionstore/SessionSaver.jsm:34
(Diff revision 2)
>  XPCOMUtils.defineLazyModuleGetter(this, "SessionFile",
>    "resource:///modules/sessionstore/SessionFile.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
>    "resource://gre/modules/PrivateBrowsingUtils.jsm");
>  
>  // Minimal interval between two save operations (in milliseconds).

nit: generally speaking, we put multiline comments in a documentation block, like:

```js
/* Wakuwaku
 *
 * Foobar.
 */
```

::: browser/components/sessionstore/SessionSaver.jsm:52
(Diff revision 2)
> -
> -  return Services.prefs.getIntPref(PREF);
> -});
> +//
> +// When the user returns to the computer, if a save is pending, we reschedule
> +// it to happen soon, with "browser.sessionstore.interval".
> +const PREF_INTERVAL_ACTIVE = "browser.sessionstore.interval";
> +const PREF_INTERVAL_IDLE = "browser.sessionstore.interval.idle";
> +const PREF_IDLE_DELAY = "browser.sessionstore.idleDelay";

You want this to be a hidden pref or something?
Regardless, I'd like you to move the lazy pref getters to the top, below these constants, tack 'em to the global scope and rename to `gIntervalWhileActive`, `gIntervalWhileIdle` and `gIdleDelay`.

::: browser/components/sessionstore/SessionSaver.jsm:72
(Diff revision 2)
>  
>  var stopWatchStart = stopWatch("start");
>  var stopWatchCancel = stopWatch("cancel");
>  var stopWatchFinish = stopWatch("finish");
>  
> +

What's happening here? \n\r <--> \n thing?

::: browser/components/sessionstore/SessionSaver.jsm:208
(Diff revision 2)
>    /**
> +   * Observe idle/active notifications.
> +   */
> +  observe: function(subject, topic, data) {
> +    switch (topic) {
> +      case "idle": {

nit: it's not necessary to brace case-blocks. Please omit them.

::: browser/components/sessionstore/SessionSaver.jsm:216
(Diff revision 2)
> +      }
> +      case "active": {
> +        this._isIdle = false;
> +        if (this._timeoutID && this._wasIdle) {
> +          // A state save has been scheduled while we were idle.
> +          // Replace it ay an active save.

nit: s/ay/by/

::: browser/components/sessionstore/SessionSaver.jsm:348
(Diff revision 2)
> +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_idleDelay", PREF_IDLE_DELAY,
> +  300000 /* 5 minutes */,
> +  () => {
> +    // Update the idle observer.
> +    Services.idle.removeIdleObserver(SessionSaverInternal, previous);
> +    Services.idle.addIdleObserver(SessionSaverInternal, latest);

I *think* you meant to add function arguments here?

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

https://reviewboard.mozilla.org/r/81672/#review80608

> You want this to be a hidden pref or something?
> Regardless, I'd like you to move the lazy pref getters to the top, below these constants, tack 'em to the global scope and rename to `gIntervalWhileActive`, `gIntervalWhileIdle` and `gIdleDelay`.

> You want this to be a hidden pref or something?

Fixed.

> Regardless, I'd like you to move the lazy pref getters to the top, below these constants, tack 'em to the global scope and rename to gIntervalWhileActive, gIntervalWhileIdle and gIdleDelay

Well, global variables (especially lazy global variables) complicate the ongoing work to migrate from `Cu.import` to ES6 modules, so I'd rather avoid that if you don't mind.

> nit: it's not necessary to brace case-blocks. Please omit them.

Fun fact: it's necessary when you declare local variables, so I've taken the habit of doing just that. But fair enough, we don't need it here, removing.

> I *think* you meant to add function arguments here?

Oops, forgot to push the latest version.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795714 [details]
Bug 1305950 (Part 1)- Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change;

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

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795715 [details]
Bug 1305950 - Add Services.idle;

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

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81672/diff/4-5/

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Addressed your comments. I have not added a test yet. Unfortunately, as of yesterday evening, I'm on a (series of) tight deadlines, so I probably won't be able to pursue this bug before early December. 

I believe (hope) that the only thing missing for this bug is a test. Unassigning if someone wants to steal it. If nobody has, I'll try and resume in December.

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8795696 [details]
Bug 1305950 - Don't collect/save the session when the user is idle;

https://reviewboard.mozilla.org/r/81672/#review80926

OK, I like it! Personal preference is still to put the lazy pref getters/ observers below their name constant declarations at the top of the file, but that's mostly a stylistic change.
Again, thanks for picking this up and I hope you'll be able to finish this some day!

::: browser/app/profile/firefox.js:782
(Diff revision 5)
>  pref("browser.selfsupport.url", "https://self-repair.mozilla.org/%LOCALE%/repair");
>  
>  pref("browser.sessionstore.resume_from_crash", true);
>  pref("browser.sessionstore.resume_session_once", false);
>  
> -// minimal interval between two save operations in milliseconds
> +// minimal interval between two save operations in milliseconds (while the user is active)

nit: capital M and missing dot at the end.

::: browser/app/profile/firefox.js:785
(Diff revision 5)
>  pref("browser.sessionstore.resume_session_once", false);
>  
> -// minimal interval between two save operations in milliseconds
> -pref("browser.sessionstore.interval", 15000);
> +// minimal interval between two save operations in milliseconds (while the user is active)
> +pref("browser.sessionstore.interval", 15000); // 15 seconds
> +
> +// minimal interval between two save operations in milliseconds (while the user is idle)

nit: capital M and missing dot at the end.

::: browser/components/sessionstore/SessionSaver.jsm:211
(Diff revision 5)
> +   */
> +  observe: function(subject, topic, data) {
> +    switch (topic) {
> +      case "idle":
> +        this._isIdle = true;
> +      break;

nit: please indent the `break;` inside the block.

::: browser/components/sessionstore/SessionSaver.jsm:326
(Diff revision 5)
>        this.updateLastSaveTime();
>        notify(null, "sessionstore-state-write-complete");
>      }, console.error);
>    },
>  };
> +

nit: superfluous newlines here and below.

Comment 29

[Will Wang [:WillWang]]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Well, we can invent our own sessionstore-specific heuristics or we can
> simply use nsIIdleService?

I was wondering, is the "computer-wide" note[1] still true? (we "poll the OS for idle info")

If so, is the difference of idle period between "computer-wide" and "application-wide" small enough for us?
That is, would it be possible that user is using other browser or application, but firefox is still consuming the disk at full rate?

I am trying to observe the idle behavior, with considering the irccloud-like factor as Marco mentioned in comment 14, hope this will help.


[1] Note: The idle service is for computer-wide idle detection, not just application idle detection. In other words, even if the user is working in other applications, the idle service will still consider the user to be active.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIIdleService

Comment 30

[Marco Bonardo [:mak]]

(In reply to Will Wang [:WillWang] from comment #29)
> [1] Note: The idle service is for computer-wide idle detection, not just
> application idle detection. In other words, even if the user is working in
> other applications, the idle service will still consider the user to be
> active.

That's true, though it will still be an improvement, and it will also help with browser power consumption.
It would be great to have also an app idle service, but we don't have it, afaik.
The other win will come when we start distinguishing the critical session data from the "optional" ones.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

I had started writing an app idle service for Performance Monitoring, I'll see if I can find what the code has become. But that sounds like a future improvement.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Ok, so it was slightly different from my memories. Still, the information is mostly available here: `mozilla::EventStateManager::LatestUserInputStart()`. We would need to somehow expose this - carefully - to listeners. As a followup bug, I believe.

Comment 33

[Mike de Boer [:mikedeboer]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #32)
> Ok, so it was slightly different from my memories. Still, the information is
> mostly available here: `mozilla::EventStateManager::LatestUserInputStart()`.
> We would need to somehow expose this - carefully - to listeners. As a
> followup bug, I believe.

TBH, I assumed that this was what the idle timer entailed. Not using the computer is, well, almost never - at least in the present day. For me, at least, it has two modes; being lid closed (standby) or used (lid open).
In today's app-switching culture, I think the metric you describe here has much more practical value! So I think there's definitely merit in exposing it to chrome scripts as a service, or similar.
Not only for session store, but for all idle-service consumers!

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Filed as bug 1307538.

Comment 35

[Marco Bonardo [:mak]]

once this is done, the switch-over to the new idle would be easy.

Comment 36

[Will Wang [:WillWang]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #27)
> I believe (hope) that the only thing missing for this bug is a test.
> Unassigning if someone wants to steal it. If nobody has, I'll try and resume
> in December.

I would like to steal this bug, if possible.
I am going to implement the test to speedup the patch landing.

Comment 37

[Marco Bonardo [:mak]]

feel free to steal it.

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Will, any update? Otherwise, I can try and work on it in the plane.

Comment 39

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #38)
> Will, any update? Otherwise, I can try and work on it in the plane.

(Will is on the way to Hawaii so I reply for him in case he can't check email.)

Will has a WIP patch that adds a mock idle server and a skeleton test case. The plan is to have you review the patch when we meet face to face :)

Comment 40

[Will Wang [:WillWang]]

Attachment: bug1305950.patch

Hi Mike, Yoric,

If possible, could one of you help to review this patch? Many thanks!

To avoid confusing you after my series of modification, this patch integrates all previous patches and now includes 3 parts:
1. (From Yoric) Don't collect/save the session when the user is idle
2. (From Yoric) Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change
3. Add a test for the behavior of state writing in idle/active mode

For mocking the idle service in test, the previous patch "Add Services.idle" is not used now.

====
Detail for patch part 1:
1-1: I fixed nits which was mentioned in previous review.
1-2: I changed the way of getting idle service, same for the mocking in test.
1-3: I fixed a typo which could cause issues once backing to active mode.
====

Please also correct me if the way of review(MozReview/Bugzilla) or patch integration could be better, thanks!!

Comment 41

[Mike de Boer [:mikedeboer]]

Comment on attachment 8821512 [details] [diff] [review]
bug1305950.patch

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

This is going in the right direction, thanks Will! Can you rework the test a bit first? Just so I can see if we need to add a bit more test coverage.

::: browser/components/sessionstore/SessionSaver.jsm
@@ +33,5 @@
>  
> +/*
> + * Minimal interval between two save operations (in milliseconds).
> + *
> + * To save battery/disk/cpu, we generally do not save changes immediately when

nit: why not say 'system resources'?

@@ +39,5 @@
> + * followed by other changes, in which case only the last write is necessary.
> + * This delay is defined by "browser.sessionstore.interval".
> + *
> + * Furthermore, when the user is not actively using the computer, webpages
> + * may still perform changes that require rewriting Session Restore, e.g.

nit: (re)writing to sessionstore.

@@ +350,5 @@
> +    }
> +});
> +
> +var idleService = Cc["@mozilla.org/widget/idleservice;1"].getService(Ci.nsIIdleService);
> +idleService.addIdleObserver(SessionSaverInternal, SessionSaverInternal._idleDelay);

I think `Services.idle` was quite neat to have, so please use that.

::: browser/components/sessionstore/test/browser_1305950.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

License headers are not necessary for test files. Please use a canonical name for the test file. You can mention the bug number in a comment inside the file, though.

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Cu.import("resource://testing-common/MockRegistrar.jsm", this);

Nice! I now see why you reverted the `Services.idle` change... but there must be a way that you register the mock idle service _before_ the lazy service getter gets evaluated, no?

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Cu.import("resource://testing-common/MockRegistrar.jsm", this);
> +
> +// The mock idle service

nit: missing dot.

@@ +4,5 @@
> +
> +Cu.import("resource://testing-common/MockRegistrar.jsm", this);
> +
> +// The mock idle service
> +let idleService = {

Please use `var`, instead of `let` in the topmost scope.

@@ +37,5 @@
> +
> +  removeIdleObserver: function(observer, time) {
> +    this._observers.delete(observer);
> +    this._activity.removeCalls.push(time);
> +  },

nit: no trailing comma, please.

@@ +40,5 @@
> +    this._activity.removeCalls.push(time);
> +  },
> +};
> +
> +function test() {

Please use `add_task(function* testIntervalChanges() {})`

@@ +42,5 @@
> +};
> +
> +function test() {
> +
> +  waitForExplicitFinish();

Not necessary when using `add_task()`

@@ +53,5 @@
> +  // in "real" idle service to avoid possible interference, especially for the
> +  // CI server environment.
> +  Services.prefs.setIntPref("browser.sessionstore.idleDelay", 86400000);
> +
> +  // Mock an idle service

nit: missing dot.

@@ +70,5 @@
> +  // Wait a `sessionstore-state-write-complete` event from any previous
> +  // scheduled state write. This is needed since the `_lastSaveTime` in
> +  // runDelayed() should be set at least once, or the `_isIdle` flag will not
> +  // become effective.
> +  promiseSaveState()

Once you rewrote this to use a generator function, you can reformat this from CPS to async-await style.

@@ +91,5 @@
> +
> +    return p1;
> +  })
> +  .catch(function(){ // We expect the promise got rejected.
> +    ok(true,"[Test 1A] No state write during idle.");

For this you may want to use `Assert.throws(yield p1);`

::: browser/components/sessionstore/test/head.js
@@ +246,5 @@
>      Services.prefs.getIntPref("browser.sessionstore.interval");
>    return waitForTopic("sessionstore-state-write-complete", timeout, aCallback);
>  }
>  function promiseSaveState() {
> +  return new Promise((resolve,reject) => {

nit: missing space after comma.

::: js/xpconnect/loader/XPCOMUtils.jsm
@@ +324,5 @@
> +            this.value = undefined;
> +            let latest = lazyGetter();
> +            aOnUpdate(data, previous, latest);
> +          } else {
> +

nit: superfluous newline.

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8821512 [details] [diff] [review]
bug1305950.patch

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

Generally, smaller patches are better and easier to trust, so if you could move back the XPCOMUtils.jsm changes to their own patch, that would be nicer.

Other than that, fine by me, with the small fixes mentioned by either Mike or myself.

::: browser/components/sessionstore/test/browser_1305950.js
@@ +46,5 @@
> +  waitForExplicitFinish();
> +
> +  // We speed up the interval between session saves to ensure that the test
> +  // runs quickly.
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 2000);

Could it be even faster?

@@ +90,5 @@
> +    SessionSaver.runDelayed(0);
> +
> +    return p1;
> +  })
> +  .catch(function(){ // We expect the promise got rejected.

Nit: Mention that `pl` hits the timeout. Also, as suggested by mike, you can rewrite the entire chain more nicely with an `Assert.throws(yield pl)` once you move to `add_task`.

::: browser/components/sessionstore/test/head.js
@@ +251,3 @@
>      waitForSaveState(isSuccessful => {
>        if (!isSuccessful) {
> +        reject("timeout");

Could you please reject with an exception instead? This lets the test suite provide better error reporting.

e.g.

reject(new Error("timeout"))

Comment 43

[Will Wang [:WillWang]]

Attachment: Bug 1305950 (Part 2)- Don't collect/save the session when the user is idle

Hi Mike, Yoric,

I deeply appreciate your review! Sorry for my late reply, here is my revised patch with following improvements:
1. Rewrite the test as `add_task()` way.
2. Fix all nits as you suggested.
3. Use a canonical name for the test file.
4. Move back the XPCOMUtils.jsm changes to their own patch.


Reply for some questions:
> Nice! I now see why you reverted the `Services.idle` change... but there
> must be a way that you register the mock idle service _before_ the lazy
> service getter gets evaluated, no?
I didn't find a way to achieve that, it would be great if someone knows it's feasible, then I can update for this part.

> use `Assert.throws(yield p1);`
This seems not working for our case and I didn't find a similar usage in m-c, 
so I suppose you meant the `Assert.rejects` which expect a rejected promise, then I utilize that in this revised patch, many thanks for this suggestion!
(Current MDN incorrectly reflect the implementation of `Assert.rejects` in m-c, I will help to update that.)

> > +  Services.prefs.setIntPref("browser.sessionstore.interval", 2000);
> Could it be even faster?
I tried, however reducing the interval lead to some weird issues,
now I tend to keep this interval as many other tests used. (The test time looks acceptable.)
Or should we try harder for this case?

In sum, now patches for this bug are:
Part 1: Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change
Part 2: Don't collect/save the session when the user is idle (attachment of this comment)


Mike,
Could you please help to review again? Thanks for your advice!

Comment 44

[Mike de Boer [:mikedeboer]]

Comment on attachment 8830868 [details] [diff] [review]
Bug 1305950 (Part 2)- Don't collect/save the session when the user is idle

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

I don't see blocking issues for this code - thanks Will! Can you address my comments below and push this patch to try?

::: browser/components/sessionstore/test/browser_not_collect_when_idle.js
@@ +68,5 @@
> +  // runDelayed() should be set at least once, or the `_isIdle` flag will not
> +  // become effective.
> +  yield promiseSaveState();
> +
> +  ok(true,"Got the state write event, start to test idle mode...");

Please use `info()` here.

@@ +78,5 @@
> +  // Cancel any possible state save, which is not related with this test to
> +  // avoid interference.
> +  SessionSaver.cancel();
> +
> +  var p1 = promiseSaveState();

nit: please use `let` inside closures.

@@ +92,5 @@
> +  // the timeout.
> +  yield Assert.rejects(promiseSaveState(), null, "[Test 1B] Again: No state write during idle.");
> +
> +  // Back to the active mode.
> +  ok(true,"Start to test active mode...");

Please use `info()` here.

@@ +96,5 @@
> +  ok(true,"Start to test active mode...");
> +  idleService._fireObservers("active");
> +
> +  yield promiseSaveState();
> +  ok(true,"[Test 2] Has state write during active.");

Please use `info()` here and I'd prefer if you asserted something useful here to ensure the state is OK.

Comment 45

[Will Wang [:WillWang]]

(In reply to Mike de Boer [:mikedeboer] from comment #44)
> @@ +96,5 @@
> > +  ok(true,"Start to test active mode...");
> > +  idleService._fireObservers("active");
> > +
> > +  yield promiseSaveState();
> > +  ok(true,"[Test 2] Has state write during active.");
> 
> Please use `info()` here and I'd prefer if you asserted something useful
> here to ensure the state is OK.

Thanks for your review! 

Please allow me to confirm one more thing about which kind of meaning of "state is OK" is you are referring to:

1. compare the value from resolve() once getting the "sessionstore-state-write-complete" state(event).
(using resolve('Got save state') in the `promiseSaveState()` and is() to assert.)

2. more deeper(and may overlap with some tests): check the correctness of certain important writing, like the usage of `waitForSaveState()` in existing tests. 
(using `getStateTabCount()`, or `getBrowserState()`...)

I'll use the better one then push to try asap, thanks!

Comment 46

[Will Wang [:WillWang]]

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

Comment 47

[Will Wang [:WillWang]]

Attachment: Bug 1305950 (Part 2)- Don't collect/save the session when the user is idle

Update patch for the `info()` in comment 44, and this patch is the one used in try-push (comment 46).

Comment 48

[Mike de Boer [:mikedeboer]]

Let's leave it at `info()` right now and not assert anything. I get now that if there is not state save when the idle observer broadcasts 'active', the test will time out, thus fail.

Comment 49

[Will Wang [:WillWang]]

Yes, thanks for your reply, let's see the try result :D

Comment 50

[Will Wang [:WillWang]]

From the treeherder, I saw that the added test in this patch has failed test[1];
I re-trigger several times for that, it seems intermittent. I am investigating at this.


[1] `bc4` test in `Windows 7 VM debug`, Error: Save state timeout

Comment 51

[Will Wang [:WillWang]]

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

Comment 52

[Will Wang [:WillWang]]

For the intermittent test failure in comment 50:

Now I have found an effective way of reproducing that intermittent failure within my local Windows VM environment, which can repeatedly provide me more information and verify my upcoming revised patch in a much shorter time.

At first, the reproduced rate was 2%(2/100), then I increased it to 80~90% by intentionally throttling the CPU. (dynamically throttle the CPU used by all Firefox processes, spawned by each ./mach test.)

I am investigating the root cause from those logs I got.


===========================================================================================
Partial log for reference:

2017-02-24 23:53:42 7 INFO Start to test active mode...
2017-02-24 23:53:42 ==== mock: _fireObservers, ===========================
2017-02-24 23:53:42 ==== trigger active obs, this._timeoutID =241 ,this._wasIdle = true
2017-02-24 23:53:42 ==== active obs: enter if, call clear, runDelayed
2017-02-24 23:53:42 ==== runDelayed: delay(passed into function) = 2000
2017-02-24 23:53:42 ==== runDelayed: + _lastSaveTime = 1487951617489
2017-02-24 23:53:42 ==== runDelayed: + interval = 2000
2017-02-24 23:53:42 ==== runDelayed: - Date.now(slightly bigger) = 1487951622069
2017-02-24 23:53:42 ==== runDelayed: delay(final to setTimeout) = 2000
2017-02-24 23:53:42 ==== runDelayed: _timeoutID = 243
2017-02-24 23:53:46 ==== _timeoutID = 243
2017-02-24 23:53:46 ==== cancel: enter
2017-02-24 23:53:46 ==== _writeState: enter, writing now ...
2017-02-24 23:53:46 ==== callback waitForSaveState, isSuccessful = false
2017-02-24 23:53:46 8 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_not_collect_when_idle.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:255 - Error: Save state timeout
2017-02-24 23:53:46 Stack trace:
2017-02-24 23:53:46     promiseSaveState/</<@chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:255:16
2017-02-24 23:53:46     waitForTopic/timeout<@chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:216:5
2017-02-24 23:53:46     setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:672:12
2017-02-24 23:53:46     waitForTopic@chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:214:17
2017-02-24 23:53:46     waitForSaveState@chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:247:10
2017-02-24 23:53:46     promiseSaveState/<@chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:252:5
2017-02-24 23:53:46     Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:390:5
2017-02-24 23:53:46     promiseSaveState@chrome://mochitests/content/browser/browser/components/sessionstore/test/head.js:251:10
2017-02-24 23:53:46     testIntervalChanges@chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_not_collect_when_idle.js:108:9
2017-02-24 23:53:46     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
2017-02-24 23:53:46     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
2017-02-24 23:53:46     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
2017-02-24 23:53:46     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
2017-02-24 23:53:46     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
2017-02-24 23:53:46     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
2017-02-24 23:53:46 9 INFO Leaving test bound testIntervalChanges
2017-02-24 23:53:46 ====  already written, calling notify sessionstore-state-write-complete ...
2017-02-24 23:53:46 ==== cancel: enter
2017-02-24 23:53:46 ==== runDelayed: delay(passed into function) = 0
2017-02-24 23:53:46 ==== runDelayed: + _lastSaveTime = 1487951625930
2017-02-24 23:53:46 ==== runDelayed: + interval = 15000
2017-02-24 23:53:46 ==== runDelayed: - Date.now(slightly bigger) = 1487951626022
2017-02-24 23:53:46 ==== runDelayed: delay(final to setTimeout) = 14908
2017-02-24 23:53:46 ==== runDelayed: _timeoutID = 244
2017-02-24 23:53:47 MEMORY STAT | vsize 663MB | vsizeMaxContiguous 1806MB | residentFast 196MB | heapAllocated 91MB
2017-02-24 23:53:47 10 INFO TEST-OK | browser/components/sessionstore/test/browser_not_collect_when_idle.js | took 10158ms
2017-02-24 23:53:47 ==== calling saveStateDelayed:  onTabAdd
2017-02-24 23:53:47 ==== SessionStore.jsm, saveStateDelayed: calling runDelayed from everywhere...
2017-02-24 23:53:47 ==== Bail out, there's a pending run
2017-02-24 23:53:47 ==== SessionStore.jsm, saveStateDelayed: calling runDelayed from everywhere...
2017-02-24 23:53:47 ==== Bail out, there's a pending run
2017-02-24 23:53:47 11 INFO checking window state
2017-02-24 23:53:47 ==== calling saveStateDelayed:  case SessionStore:update
2017-02-24 23:53:47 ==== SessionStore.jsm, saveStateDelayed: calling runDelayed from everywhere...
2017-02-24 23:53:47 ==== Bail out, there's a pending run
2017-02-24 23:53:47 ==== calling saveStateDelayed:  case SessionStore:update
2017-02-24 23:53:47 ==== SessionStore.jsm, saveStateDelayed: calling runDelayed from everywhere...
2017-02-24 23:53:47 ==== Bail out, there's a pending run
2017-02-24 23:53:56 Completed ShutdownLeaks collections in process 2920
2017-02-24 23:53:56 Completed ShutdownLeaks collections in process 1440
2017-02-24 23:53:59 ==== calling saveStateDelayed:  case SessionStore:update
2017-02-24 23:53:59 ==== SessionStore.jsm, saveStateDelayed: calling runDelayed from everywhere...
2017-02-24 23:53:59 ==== Bail out, there's a pending run
2017-02-24 23:54:06 Completed ShutdownLeaks collections in process 5852
2017-02-24 23:54:06 12 INFO TEST-START | Shutdown
2017-02-24 23:54:06 13 INFO Browser Chrome Test Summary
2017-02-24 23:54:06 14 INFO Passed:  2
2017-02-24 23:54:06 15 INFO Failed:  1
2017-02-24 23:54:06 16 INFO Todo:    0
2017-02-24 23:54:06 17 INFO Mode:    e10s
2017-02-24 23:54:06 18 INFO *** End BrowserChrome Test Results ***
2017-02-24 23:54:06 ==== _timeoutID = 244
2017-02-24 23:54:06 ==== cancel: enter
2017-02-24 23:54:06 ==== _writeState: enter, writing now ...
2017-02-24 23:54:07 ==== calling saveStateDelayed:  case SessionStore:update
2017-02-24 23:54:07 ==== SessionStore.jsm, saveStateDelayed: calling runDelayed from everywhere...
2017-02-24 23:54:07 ==== runDelayed: delay(passed into function) = 2000
2017-02-24 23:54:07 ==== runDelayed: + _lastSaveTime = 1487951646248
2017-02-24 23:54:07 ==== runDelayed: + interval = 15000
2017-02-24 23:54:07 ==== runDelayed: - Date.now(slightly bigger) = 1487951646651
2017-02-24 23:54:07 ==== runDelayed: delay(final to setTimeout) = 14597
2017-02-24 23:54:07 ==== runDelayed: _timeoutID = 247
2017-02-24 23:54:07 ====  already written, calling notify sessionstore-state-write-complete ...
2017-02-24 23:54:07 ==== calling saveStateDelayed:  case SessionStore:update
2017-02-24 23:54:07 ==== SessionStore.jsm, saveStateDelayed: calling runDelayed from everywhere...
2017-02-24 23:54:07 ==== Bail out, there's a pending run
2017-02-24 23:54:08 ==== cancel: enter
2017-02-24 23:54:08 ==== _writeState: enter, writing now ...
2017-02-24 23:54:08 ==== cancel: enter
2017-02-24 23:54:10 ====  already written, calling notify sessionstore-state-write-complete ...
2017-02-24 23:54:26 TEST-INFO | Main app process: exit 0
2017-02-24 23:54:26 runtests.py | Application ran for: 0:02:01.838000

Comment 53

[Will Wang [:WillWang]]

For the intermittent test failure:

After the detailed test, I found that in the CPU-limited machine, the promise rejection "Error: Save state timeout" will appear intermittently from several promiseSaveState(), not only from the last one as try result shown.

The root cause of error from first promiseSaveState() had been identified and can be entirely avoided; an improved patch will be sent to try soon.

Comment 54

[Will Wang [:WillWang]]

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

Comment 55

[Will Wang [:WillWang]]

The revised patch without making the previous intermittent test fail is attached in next comment for review.

The previous test can be pretty frequently failed on extremely CPU-limited circumstance; I can easily reproduce the failure by throttling CPU to ~300 MHz(Yes, back to the old days...) in my local mass testing. Thus certain test procedures need more time to finish, which will slightly exceed the timeout we set in `waitForSaveState()` / `promiseSaveState()`, intermittently.

There are two intermittent fail points in the previous test, here are the findings and my proposed solution for them:

1. Before the "real" test, we wait for one any write event after Firefox starts:

As the comment in code, this is needed since the `_lastSaveTime` in runDelayed() should be set at least once, or the `_isIdle` flag will not become effective. In fact, we don't care and should not expect that write event could be captured within the timeout. Thus, in this case, I set the timeout to 30sec which is about as long as the timeout for the regular whole test case.

2. The write event after backing the active mode:

From my some observation in local test and try server, I noticed that the procedure between "triggering active mode" and "capturing write event" needs more time to finish, especially on a CPU-limited AND non-e10s environment. We can also observe that the test on try server "Windows 7 VM debug" platform with non-e10s mode fails more frequently. In brief, the call flow of the procedure mentioned above is:
* Test(browser_not_collect_when_idle.js): fire active observer
* SessionSaver.jsm: observe() => runDelayed() => _saveStateAsync => _saveState => _writeState
* SessionFile.jsm: SessionFile.write()
* Then wait for the notification service to notify write event after the disk write.
So it appears that above procedure intermittently needs more time than 2sec,  which we set for speeding up the test. 
Since if Firefox can normally back to saving works after few more seconds, it should not be a problem here to wait a short while. As a result, I add 3 more seconds and all of the local tests/try results look great.

Comment 56

[Will Wang [:WillWang]]

Attachment: Bug 1305950 (Part 2) (v3)- Don't collect/save the session when the user is idle

Hi Mike,

Here is the patch as above comment mentioned:
- Fixed intermittent test failure.
- Rebase for latest change on m-c.

Could you please help to review again?
Thanks a lot!

Comment 57

[Will Wang [:WillWang]]

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

Comment 58

[Mike de Boer [:mikedeboer]]

Comment on attachment 8852571 [details] [diff] [review]
Bug 1305950 (Part 2) (v3)- Don't collect/save the session when the user is idle

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

I have a couple of questions about the implementation as well, so this is not quite ready... It looks like you didn't add a test that covers changing the idleDelay pref value during runtime, so I'm not sure if you want to support that.

::: browser/components/sessionstore/SessionSaver.jsm
@@ +9,5 @@
>  const Cu = Components.utils;
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  
> +Cu.import("resource://gre/modules/Preferences.jsm", this);

You're not using this module anymore.

@@ +201,5 @@
>      this._timeoutID = null;
>    },
>  
>    /**
> +   * Observe idle/active notifications.

nit: 'idle/ active'

@@ +336,5 @@
> +    // Cancel any pending runs and call runDelayed() with
> +    // zero to apply the newly configured interval.
> +    SessionSaverInternal.cancel();
> +    SessionSaverInternal.runDelayed(0);
> +});

Please align these with the function body, or:

```js
XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_intervalWhileActive", PREF_INTERVAL_ACTIVE,
  15000 /* 15 seconds */, () => {
  // Cancel any pending runs and call runDelayed() with
  // zero to apply the newly configured interval.
  SessionSaverInternal.cancel();
  SessionSaverInternal.runDelayed(0);
});
```

@@ +343,5 @@
> +  3600000 /* 1 h */);
> +
> +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_idleDelay", PREF_IDLE_DELAY,
> +  180000 /* 3 minutes */,
> +  (key, previous, latest) => {

And I'm sorry, but does passing a fifth argument work at all? AFAIK it does not.

@@ +344,5 @@
> +
> +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_idleDelay", PREF_IDLE_DELAY,
> +  180000 /* 3 minutes */,
> +  (key, previous, latest) => {
> +    var idleService = Cc["@mozilla.org/widget/idleservice;1"].

Since the `var idleService` below is hoisted, it should also be available in this scope. In that case you don't need the extra `getService` call here.

@@ +353,5 @@
> +    }
> +    if (latest != undefined) {
> +      idleService.addIdleObserver(SessionSaverInternal, latest);
> +    }
> +});

Same here wrt alignment.

::: browser/components/sessionstore/test/browser_not_collect_when_idle.js
@@ +40,5 @@
> +    this._activity.removeCalls.push(time);
> +  }
> +};
> +
> +add_task(function* testIntervalChanges() {

Please use `async function testIntervalChanges() {`.

@@ +73,5 @@
> +  // become effective.
> +  // We wait at most 30 sec which is about as long as the timeout for the
> +  // regular whole test case, and we don't expect the write event comes fast
> +  // enough for an immediate `waitForSaveState()` from now.
> +  let promiseFirstWrite = new Promise(function(resolve, reject) {

You can say `await new Promise() {...` here.

@@ +76,5 @@
> +  // enough for an immediate `waitForSaveState()` from now.
> +  let promiseFirstWrite = new Promise(function(resolve, reject) {
> +    waitForTopic("sessionstore-state-write-complete", 30 * 1000, function(isSuccessful) {
> +      if (!isSuccessful) {
> +        reject(new Error("Timeout: didn't get any `sessionstore-state-write-complete` event"));

Make sure to return here; now it'll reject AND resolve.

@@ +78,5 @@
> +    waitForTopic("sessionstore-state-write-complete", 30 * 1000, function(isSuccessful) {
> +      if (!isSuccessful) {
> +        reject(new Error("Timeout: didn't get any `sessionstore-state-write-complete` event"));
> +      }
> +      resolve('Success!');

nit: double quotes, but preferably just leave it empty.

@@ +100,5 @@
> +  // `browser.sessionstore.interval.idle` ms, since the idle flag was just set.
> +  SessionSaver.runDelayed(0);
> +
> +  // We expect `p1` hits the timeout.
> +  yield Assert.rejects(p1, null, "[Test 1A] No state write during idle.");

make sure to change all these to `await` instead of `yield`.

@@ +110,5 @@
> +  // Back to the active mode.
> +  info("Start to test active mode...");
> +  idleService._fireObservers("active");
> +
> +  let promiseActiveWrite = new Promise(function(resolve, reject) {

You can say `await new Promise() {...` here.

@@ +113,5 @@
> +
> +  let promiseActiveWrite = new Promise(function(resolve, reject) {
> +    waitForTopic("sessionstore-state-write-complete", PREF_SS_INTERVAL + 3000, function(isSuccessful) {
> +      if (!isSuccessful) {
> +        reject(new Error("Timeout: didn't get any `sessionstore-state-write-complete` event"));

Make sure to return here; now it'll reject AND resolve.

@@ +115,5 @@
> +    waitForTopic("sessionstore-state-write-complete", PREF_SS_INTERVAL + 3000, function(isSuccessful) {
> +      if (!isSuccessful) {
> +        reject(new Error("Timeout: didn't get any `sessionstore-state-write-complete` event"));
> +      }
> +      resolve('Success!');

nit: double quotes, but preferably just leave it empty.

Comment 59

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #58)
> Comment on attachment 8852571 [details] [diff] [review]
> Bug 1305950 (Part 2) (v3)- Don't collect/save the session when the user is
> idle
> 
> Review of attachment 8852571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a couple of questions about the implementation as well, so this is
> not quite ready... It looks like you didn't add a test that covers changing
> the idleDelay pref value during runtime, so I'm not sure if you want to
> support that.
>
> @@ +343,5 @@
> > +  3600000 /* 1 h */);
> > +
> > +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_idleDelay", PREF_IDLE_DELAY,
> > +  180000 /* 3 minutes */,
> > +  (key, previous, latest) => {
> 
> And I'm sorry, but does passing a fifth argument work at all? AFAIK it does
> not.

Argh, I missed the first patch again. It's hard to continue reviewing after such a long time. :(
OK, so the fifth arguments mistery is solved.

My test coverage question still stands - I'm not entirely sure what you use it for exactly.

Looking forward to your update!

Comment 60

[Will Wang [:WillWang]]

Attachment: Bug 1305950 (Part 2) (v4)- Don't collect/save the session when the user is idle

Hi Mike,

Many thanks for your detailed review! :)

I already updated my patch, which fixes issues you replied(please see below for each) and also includes:
- Fix all eslint problems.
- Increase timeout of 2nd intermittent fail point in comment 55 (The write event after backing the active mode) (from 3s to 10s) to achieve _zero_ intermittent fail, since performing more extensive tests on try server, debug platforms (such as 'Linux debug', 'Linux x64 debug') show that they need more time to complete my previously mentioned procedure. 
 
> ::: browser/components/sessionstore/SessionSaver.jsm
> @@ +9,5 @@
> >  const Cu = Components.utils;
> >  const Cc = Components.classes;
> >  const Ci = Components.interfaces;
> >  
> > +Cu.import("resource://gre/modules/Preferences.jsm", this);
> 
> You're not using this module anymore.
Fixed, thanks for catching this!

> @@ +201,5 @@
> >      this._timeoutID = null;
> >    },
> >  
> >    /**
> > +   * Observe idle/active notifications.
> 
> nit: 'idle/ active'
Fixed

> @@ +336,5 @@
> > +    // Cancel any pending runs and call runDelayed() with
> > +    // zero to apply the newly configured interval.
> > +    SessionSaverInternal.cancel();
> > +    SessionSaverInternal.runDelayed(0);
> > +});
> 
> Please align these with the function body, or:
Fixed

> @@ +343,5 @@
> > +  3600000 /* 1 h */);
> > +
> > +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_idleDelay", PREF_IDLE_DELAY,
> > +  180000 /* 3 minutes */,
> > +  (key, previous, latest) => {
> 
> And I'm sorry, but does passing a fifth argument work at all? AFAIK it does
> not.
Sorry for confusing you, I better make the review easier next time!

> @@ +344,5 @@
> > +
> > +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_idleDelay", PREF_IDLE_DELAY,
> > +  180000 /* 3 minutes */,
> > +  (key, previous, latest) => {
> > +    var idleService = Cc["@mozilla.org/widget/idleservice;1"].
> 
> Since the `var idleService` below is hoisted, it should also be available in
> this scope. In that case you don't need the extra `getService` call here.
Indeed! But I think I might need to keep this to make the test works, which will update the observer of `_idleDelay`; If no extra `getService` call, the updated idle service is not the mock one.

> @@ +353,5 @@
> > +    }
> > +    if (latest != undefined) {
> > +      idleService.addIdleObserver(SessionSaverInternal, latest);
> > +    }
> > +});
> 
> Same here wrt alignment.
Fixed

> ::: browser/components/sessionstore/test/browser_not_collect_when_idle.js
> @@ +40,5 @@
> > +    this._activity.removeCalls.push(time);
> > +  }
> > +};
> > +
> > +add_task(function* testIntervalChanges() {
> 
> Please use `async function testIntervalChanges() {`.
Fixed

> @@ +73,5 @@
> > +  // become effective.
> > +  // We wait at most 30 sec which is about as long as the timeout for the
> > +  // regular whole test case, and we don't expect the write event comes fast
> > +  // enough for an immediate `waitForSaveState()` from now.
> > +  let promiseFirstWrite = new Promise(function(resolve, reject) {
> 
> You can say `await new Promise() {...` here.
Fixed

> @@ +76,5 @@
> > +  // enough for an immediate `waitForSaveState()` from now.
> > +  let promiseFirstWrite = new Promise(function(resolve, reject) {
> > +    waitForTopic("sessionstore-state-write-complete", 30 * 1000, function(isSuccessful) {
> > +      if (!isSuccessful) {
> > +        reject(new Error("Timeout: didn't get any `sessionstore-state-write-complete` event"));
> 
> Make sure to return here; now it'll reject AND resolve.
Many thanks for catching this!! Basically I reused the code snippet in head.js and didn't notice this flaw, now I also update the head.js.

> @@ +78,5 @@
> > +    waitForTopic("sessionstore-state-write-complete", 30 * 1000, function(isSuccessful) {
> > +      if (!isSuccessful) {
> > +        reject(new Error("Timeout: didn't get any `sessionstore-state-write-complete` event"));
> > +      }
> > +      resolve('Success!');
> 
> nit: double quotes, but preferably just leave it empty.
Fixed

> @@ +100,5 @@
> > +  // `browser.sessionstore.interval.idle` ms, since the idle flag was just set.
> > +  SessionSaver.runDelayed(0);
> > +
> > +  // We expect `p1` hits the timeout.
> > +  yield Assert.rejects(p1, null, "[Test 1A] No state write during idle.");
> 
> make sure to change all these to `await` instead of `yield`.
Fixed

> @@ +110,5 @@
> > +  // Back to the active mode.
> > +  info("Start to test active mode...");
> > +  idleService._fireObservers("active");
> > +
> > +  let promiseActiveWrite = new Promise(function(resolve, reject) {
> 
> You can say `await new Promise() {...` here.
Fixed

> @@ +113,5 @@
> > +
> > +  let promiseActiveWrite = new Promise(function(resolve, reject) {
> > +    waitForTopic("sessionstore-state-write-complete", PREF_SS_INTERVAL + 3000, function(isSuccessful) {
> > +      if (!isSuccessful) {
> > +        reject(new Error("Timeout: didn't get any `sessionstore-state-write-complete` event"));
> 
> Make sure to return here; now it'll reject AND resolve.
Fixed, thanks :)

> @@ +115,5 @@
> > +    waitForTopic("sessionstore-state-write-complete", PREF_SS_INTERVAL + 3000, function(isSuccessful) {
> > +      if (!isSuccessful) {
> > +        reject(new Error("Timeout: didn't get any `sessionstore-state-write-complete` event"));
> > +      }
> > +      resolve('Success!');
> 
> nit: double quotes, but preferably just leave it empty.
Fixed


> add a test that covers changing the idleDelay pref value during runtime, so I'm not sure if you want to support that.
During the runnung of this test, it change the idleDelay pref value to update the observer, if this part fails then the whole test fails. This might be sort of coverage for idleDelay, however I'm not sure, how do you think?

Could you please help to review again?
Thanks a lot!

Comment 61

[Mike de Boer [:mikedeboer]]

Comment on attachment 8854131 [details] [diff] [review]
Bug 1305950 (Part 2) (v4)- Don't collect/save the session when the user is idle

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

Thanks for the udpates, Will! r=me with the remaining comments addressed.

::: browser/components/sessionstore/SessionSaver.jsm
@@ +125,5 @@
>  
>    /**
> +   * `true` if the user has been idle for at least
> +   * `SessionSaverInternal._intervalWhileIdle` ms. Idleness is computed
> +   * with `Services.idle`.

nit: Services.idle is no more, please change this to `nsIIdleService`

@@ +200,5 @@
>      this._timeoutID = null;
>    },
>  
>    /**
> +   * Observe 'idle/ active' notifications.

nit: no need for the quotes.

@@ +341,5 @@
> +  3600000 /* 1 h */);
> +
> +XPCOMUtils.defineLazyPreferenceGetter(SessionSaverInternal, "_idleDelay", PREF_IDLE_DELAY,
> +  180000 /* 3 minutes */, (key, previous, latest) => {
> +  var idleService = Cc["@mozilla.org/widget/idleservice;1"].getService(Ci.nsIIdleService);

Can you add a comment above that explains that you're re-fetching the service, because of the Mock service in your unit test?

::: browser/components/sessionstore/test/browser_not_collect_when_idle.js
@@ +38,5 @@
> +    this._activity.removeCalls.push(time);
> +  }
> +};
> +
> +add_task(testIntervalChanges);

Please move `async function testIntervalChanges() {` in here. `add_task` now supports async await style methods. Please read this thread in dev-platform: https://lists.mozilla.org/pipermail/dev-platform/2017-March/017806.html

@@ +41,5 @@
> +
> +add_task(testIntervalChanges);
> +
> +async function testIntervalChanges() {
> +

nit: superfluous newline.

Comment 62

[Will Wang [:WillWang]]

Attachment: Bug 1305950 (Part 2) (v5)- Don't collect/save the session when the user is idle

Thanks a lot, Mike!

Here is the revised patch that fixed all review issues,
and I carry r+ from the previous v4 patch.

Comment 63

[Will Wang [:WillWang]]

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

Comment 64

[Will Wang [:WillWang]]

Set checkin-needed.

- 1st patch(comment 10): Bug 1305950 (Part 1)- Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change;
-2nd patch(comment 62): Bug 1305950 (Part 2) (v5)- Don't collect/save the session when the user is idle

Thanks!

Comment 65

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues are marked as resolved in MozReview.

Comment 66

[Will Wang [:WillWang]]

Hi Ryan :)

I am not sure whether you mean the issue of patch "Bug 1305950 - Don't collect/save the session when the user is idle;r?mdeboer" on MozReview, if so, that's the old one and could you help to land the patch on comment 62? (As the comment 64 mentioned) Please correct me if I am wrong :)

Many thanks!

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Whoops, missed that there was a non-MozReview attachment there. What crappy UX that has :(

Comment 68

[Ryan VanderMeulen [:RyanVM]]

In the future, it would have been a lot easier if you would have made Yoric's Part 1 a new attachment instead of leaving it in MozReview.

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77f56c43941
Extend XPCOMUtils.defineLazyPreferenceGetter to be informed upon preference change. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8192b5ac7e
Don't collect/save the session when the user is idle. r=mdeboer

Comment 70

[Will Wang [:WillWang]]

Ryan, Thanks for your consideration, I will use MozReview and the way you mentioned to ease similar problems in other bugs :)

Comment 71

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b77f56c43941
https://hg.mozilla.org/mozilla-central/rev/4d8192b5ac7e

Comment 72

[(no longer active)]

We're trying to find the root cause of a 1.7ms in the 95th percentile input event responsiveness telemetry probe from the parent process in bug 1362094 and this patch is one of the three possible culprits that we currently suspect as the possible root causes.

Will, do you think this could be the possible cause of bug 1362094?  Would you be OK with a temporary backout of this bug to see if the regression is reclaimed on telemetry?

Comment 73

[Will Wang [:WillWang]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #72)
> Will, do you think this could be the possible cause of bug 1362094?  Would
> you be OK with a temporary backout of this bug to see if the regression is
> reclaimed on telemetry?

Hi Ehsan, thank you for your investigation works and informing me this.

I'm surely OK with the backout if we have to, unfortunately, I didn't observe the relationship between these two bugs so far, could you share a bit the reason you observed, if possible? Thanks!

Comment 74

[(no longer active)]

I was just looking for changes that could look possibly have touched code that would run the risk of affecting the input event responsiveness metric only in the parent process.  Since the some of the session restore code runs off of a timer it does run the risk of running at a time when user input events can run, and it also runs in the parent process, so these changes seemed like possible culprits.  But otherwise I'm just guessing at this point, I could be totally wrong.  :-)

(If you feel there is absolutely no way this could have been responsible please let me know!)

Otherwise can you please back this out at your convenience?  If there is no work depending on this, I would prefer if you can avoid relanding it for a couple of weeks, but if there is a reason to land it sooner I think we may still be able to observe some change in the data, but I'd have to check and get back to you.

How does that sound?  If it sounds good please feel free to go ahead with the backout when you can!  Thank you.

Comment 75

[Will Wang [:WillWang]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #74)
> [...] Since the some of the session restore code runs
> off of a timer it does run the risk of running at a time when user input
> events can run, and it also runs in the parent process, so these changes
> seemed like possible culprits.  But otherwise I'm just guessing at this
> point, I could be totally wrong.  :-)
> 
> (If you feel there is absolutely no way this could have been responsible
> please let me know!)

Thanks for your detailed reply, Ehsan! :)

Your consideration is right regarding the timer of session restore, however, the patch in this bug was doing the opposite:
Make the timer (almost) disabled when user is idle, which means this bug is unlikely your target.

To reduce the noise during your hard investigation, I'll suggest that we don't backout this patch at this moment, which probably increases the possibility of jank... what do you think?

Comment 76

[(no longer active)]

OK yes from your explanation it seems unlikely for this patch to have affected this regression now that I think more about this...  :-/