Open Bug 1344715 Opened 7 years ago Updated 2 years ago

Don't collect sessionstore data during a restore operation

Categories

(Firefox :: Session Restore, enhancement)

Product:
Firefox
Component:
Session Restore
Type:
enhancement

Tracking

()

People

(Reporter: ehsan.akhgari, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p5])

Attachments

(2 files, 3 obsolete files)

Bug 1344715 - while restoring, reschedule any session writes that take place,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1344715 - adds test to check that we are not saving session while restoring,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
Unfortunately I don't have a profile link at hand right now, but I have seen us spend time capturing session store information (similar to the profile in bug 1339480, for example) during a session restore, which slows down the restore option, and it also seems silly to try to collect information about the same session that we are restoring.
I believe you and that makes this a perfect starter bug for a GSOC student. Thanks for using the right tracking bug :)
Sir , I would like to work on this bug.
(In reply to milindl from comment #3)
> Created attachment 8847190 [details]
> Bug 1344715 - do not save session data when restoring,
> 
> Review commit: https://reviewboard.mozilla.org/r/120192/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/120192/

Hi, I've tried to understand the code and work on this bug.
In summary, I've exposed SessionStorageInternal's _browserSetState, which is true whenever we are setting state - and those are the times when we don't also want to get state. 

I've tested with the unit tests and mochitests in the browser/components/sessionstore/test folder.

I'm unsure whether using _browserSetState was the correct choice (should I have made a new one for restore purposes entirely? or something else?) but I felt that the best way to make progress was to try and get reviews.

Thanks :)
Comment on attachment 8847190 [details]
Bug 1344715 - while restoring, stop collecting sessionstore data,

https://reviewboard.mozilla.org/r/120192/#review122464

I'm positively impressed that you understood the code to this level already and are able to push a complete patch using our tools (using Mercurial and ReviewBoard!)

I'd like for David to take a look at the SessionSaver.jsm logic as well. I think it looks good, but I'm not 100% sure about updating of the 'lastSaveTime'.

Lastly, please prepare a unit test for this. You can do this in a separate commit/ patch as that will be easier for me to review. You can find various examples of unit tests next to the code, but please feel free to reach out here, on IRC or email!

::: commit-message-38dab:1
(Diff revision 1)
> +Bug 1344715 - do not save session data when restoring, r=mikedeboer

Please provide a nice, descriptive message here.

::: browser/components/sessionstore/SessionStore.jsm:222
(Diff revision 1)
>    get lastClosedObjectType() {
>      return SessionStoreInternal.lastClosedObjectType;
>    },
>  
> +  get browserSetState() {
> +    return SessionStoreInternal.browserSetState;

I think you've arrived at half of the solution!

I think it'd be best if you add a new public getter to SessionStore called `busy` that
1. Checks `SessionStoreInternal._browserSetState` first and returns `true` when it's set to `true` specifically,
2. Iterates over `SessionStoreInternale._windows` and checks the `busy` property on each and returns `true` right away when it finds one that is busy.

::: browser/components/sessionstore/SessionStore.jsm:537
(Diff revision 1)
>     */
>    get promiseInitialized() {
>      return this._deferredInitialized.promise;
>    },
>  
> +  get browserSetState() {

No need to make this look 'public'; you can remove it.
Attachment #8847190 - Flags: review?(mdeboer)
Assignee: nobody → i.milind.luthra
Status: NEW → ASSIGNED
Attachment #8847190 - Flags: feedback?(dteller)
Mike,

Thanks for the review, I appreciate it :)

I'll get on to the task. I am also working on another bug which has required me to go through some unit tests, so I feel like I should be able to write it. 

I'm facing some issues with Hg (I'm more used to git) but I'm learning. I'll get back on this soon.

Thanks!
Comment on attachment 8847190 [details]
Bug 1344715 - while restoring, stop collecting sessionstore data,

https://reviewboard.mozilla.org/r/120192/#review122464

> Please provide a nice, descriptive message here.

I've made a multiline message describing my work better. Is that OK (since the one-liner should be close to the 80 character limit I felt)
Comment on attachment 8847190 [details]
Bug 1344715 - while restoring, stop collecting sessionstore data,

https://reviewboard.mozilla.org/r/120192/#review122940

Almost there!

::: browser/components/sessionstore/SessionStore.jsm:219
(Diff revision 2)
>      SessionStoreInternal.canRestoreLastSession = val;
>    },
>  
> +  get busy() {
> +    return SessionStoreInternal._browserSetState ||
> +           Object.values(SessionStoreInternal._windows).some(win => win.busy);

I wish we could use this, but we can optimize this instead:

```js
Object.getOwnPropertyNames(SessionStoreInternal._windows).sort().reverse().some(id => SessionStoreInternal._windows[id].busy);
```

Can you tell me why? (small exercise ;-) )
Attachment #8847190 - Flags: review?(mdeboer) → review-
Comment on attachment 8847190 [details]
Bug 1344715 - while restoring, stop collecting sessionstore data,

https://reviewboard.mozilla.org/r/120192/#review122942

::: commit-message-38dab:2
(Diff revision 2)
> +Bug 1344715 - while restoring, stop collecting sessionstore data, r?mikedeboer
> +Add a public getter busy to SessionStore which returns true if restoring

Can you add a newline between the first line and your extended commit message?
Mike: Thanks for the review :)

I've made the changes necessary, and I have also tried to answer the exercise on MozReview comments.

I've also started making a new changeset for the unit test (it is a mochitest), which I'll post tonight (night in my timezone at least). It's a pretty WIP patch, so I will require some help there.

Thanks!
(In reply to milindl from comment #13)
> Created attachment 8848149 [details]
> Bug 1344715 - add test to check that we are not saving session while
> restoring,
> 
> The test tries to restore a state, then tries to write. If write is
> successful,
> then there is failure.
> 
> Review commit: https://reviewboard.mozilla.org/r/121120/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/121120/

Regarding this patch, I'll say briefly what I have done (and more importantly, why). I'll also list the possible causes of error.

1. Firstly I've set the interval of storing to an arbitrarily large number so we don't get any saving on its own
2. Then, I begin restoring a state with a callback (A).
3. I also call saveState with a callback (B) which runs only if the state writing was successful.

In A, I check if B was called - if it was, then there is an issue, since we should not have saved after initiating restore (and before it ended).

Possible issues: it is possible that the waitForBrowserState completes and the callback is executed before we make it to the waitForSaveState function. This means that we may occasionally PASS a test which should have failed, we should never FAIL a test which should have passed. I'm not sure how to deal with this, since the saveState call needs to be AFTER restore starts.

On my PC, it works as expected.

Thanks :)
Comment on attachment 8847190 [details]
Bug 1344715 - while restoring, stop collecting sessionstore data,

https://reviewboard.mozilla.org/r/120192/#review122940

> I wish we could use this, but we can optimize this instead:
> 
> ```js
> Object.getOwnPropertyNames(SessionStoreInternal._windows).sort().reverse().some(id => SessionStoreInternal._windows[id].busy);
> ```
> 
> Can you tell me why? (small exercise ;-) )

Mike: I think it is an optimization because we are ordering by window numbers - we're first checking if windowN is busy, then windowN-1....window1, window0. I feel like this has to do with the order in which windows are 'unbusied'. This means we won't iterate through useless windows, we'll find the first window which is still busy.
Comment on attachment 8847190 [details]
Bug 1344715 - while restoring, stop collecting sessionstore data,

https://reviewboard.mozilla.org/r/120192/#review125836

LGTM, thanks! I'd still like David (:Yoric) to take a look at the SessionSaver.jsm bit, because I'm not 100% certain of its validity. I'll set the appropriate flag.
Attachment #8847190 - Flags: review?(mdeboer) → review+
Attachment #8847190 - Flags: feedback?(dteller)
Comment on attachment 8848149 [details]
Bug 1344715 - add test to check that we are not saving session while restoring,

https://reviewboard.mozilla.org/r/121120/#review125838

::: browser/components/sessionstore/test/browser.ini:247
(Diff revision 1)
>  skip-if = !e10s # GroupedSHistory is e10s-only
>  
>  [browser_closed_objects_changed_notifications_tabs.js]
>  [browser_closed_objects_changed_notifications_windows.js]
>  [browser_duplicate_history.js]
> +[browser_1344715.js]

Please provide a canonical name for your test file. You can see examples in the entries above this one.

::: browser/components/sessionstore/test/browser_1344715.js:6
(Diff revision 1)
> +"use strict";
> +
> +var oldState = JSON.parse(SessionStore.getBrowserState());
> +
> +function test() {
> +  waitForExplicitFinish();

If you move your test code to use add_task(), combined with `async` functions, you don't need to manually `waitForExplicitFinish()` and call `done()` at the end.

Please look at other tests in this folder to get inspiration how we structure tests like this. Newer tests are usually better to look at, since they use the latest test harness and Javascript goodness. 

You'll see, that currently we're still using `
```js
add_task(function* test_something() {
  yield promiseDoSomething();
});
```

but now you can write:
```js
add_task(async function test_something() {
  await promiseDoSomething();
});
```

::: browser/components/sessionstore/test/browser_1344715.js:23
(Diff revision 1)
> +      tabs: [
> +        { entries: [{ url: "http://example.org#0", triggeringPrincipal_base64 }] },
> +        { entries: [{ url: "http://example.com#1", triggeringPrincipal_base64 }] },
> +        { entries: [{ url: "http://example.com#2", triggeringPrincipal_base64 }] },
> +      ],
> +      selected: 3

Tab with index `3` doesn't exist?

::: browser/components/sessionstore/test/browser_1344715.js:38
(Diff revision 1)
> +    }
> +  ] };
> +  let writeStateCalled = false;
> +  waitForBrowserState(state, () => {
> +    // Save should complete without the state being written
> +    ok(!writeStateCalled, "Wrote state while restoring");

Well, since you changed the interval to 1,000,000ms(!), it's not a real wonder that the state is never saved during restore. What I'd expect is that you set the interval to something really small and that the code you added prevents the state save instead.

In other words, I don't think that you're testing what you need to be testing with this code.
Attachment #8848149 - Flags: review?(mdeboer)
Comment on attachment 8847190 [details]
Bug 1344715 - while restoring, stop collecting sessionstore data,

https://reviewboard.mozilla.org/r/120192/#review125866

::: commit-message-38dab:3
(Diff revision 3)
> +Bug 1344715 - while restoring, stop collecting sessionstore data, r?mikedeboer
> +
> +Add a public getter busy to SessionStore which returns true if restoring

Nit: To simplify parsing the sentence, write "`busy`" (not just "busy").

::: browser/components/sessionstore/SessionSaver.jsm:190
(Diff revision 3)
> +    if (SessionStore.busy) {
> +      // Don't save anything if we are restoring
> +      // The current time is the last save time - we are restoring
> +
> +      this.updateLastSaveTime();
> +      return Promise.resolve();

I'm not sure it's the right `Promise` to return. Shouldn't you rather return a promise that is resolved once the ongoing save is resolved?

::: browser/components/sessionstore/SessionStore.jsm:220
(Diff revision 3)
>    },
>  
> +  get busy() {
> +    return SessionStoreInternal._browserSetState ||
> +           Object.getOwnPropertyNames(SessionStoreInternal._windows)
> +           .sort().reverse().some(id => SessionStoreInternal._windows[id].busy);

This deserves some documentation: I don't remember what `_windows[id].busy` means in this context, so it's not clear to me what this does.

Also, why do you even `sort()` and `reverse()`?
Mike: 
Thanks for the review!
Firstly, sorry for using an older testing style, and not a newer one. I'll fix that.
 
> Well, since you changed the interval to 1,000,000ms(!), it's not a real
> wonder that the state is never saved during restore. What I'd expect is that
> you set the interval to something really small and that the code you added
> prevents the state save instead.

Regarding this, I initially thought about having a observer based test, which would briefly be structured like

1. Start observing for saves (using the topic "sessionstore-state-write-complete")
2. Trigger a restore to the arvitrary session, with a callback to remove the observer we added in 1
3. if the observer is called at anytime, fail the test, since that means that we wrote state while restoring

I thought this would be problematic because the notification can arrive between 1 and 2 (triggering a possible FAIL when the code is working).

So, instead, I am making sure that the session saving is never triggered on its own, we always need to force it (hence the large timeout). I begin by restoring a state (forcefully), followed by a call to saveState with a callback A (which will be called only if we write the state). If callback A is called before we're done restoring our state, we cause a fail. 

This test suffers the opposite problem; that if the restore finished before I get around to calling saveState, then it will PASS without regard to the actual code.

The issue I face is that I'm not sure how to 'insert' the saving code into the restoring code; so in both cases there's a sort of 'chance' factor, which should be avoided.

I'd appreciate any input on this. In any case, I'll submit an alternate patch over the weekend.

Thanks!
Comment on attachment 8847190 [details]
Bug 1344715 - while restoring, stop collecting sessionstore data,

f=me, but I'd like my questions answered before this lands.
Attachment #8847190 - Flags: feedback?(dteller) → feedback+
Mike: 

To get a better idea for what approaches I meant, I've added 2 files in the latest commit I made. It's marked WIP, because I think there might be some changes I will need to make. I have a few doubts regarding both, that is;

In the first one, what if a write is triggered between adding an observer and calling the restore state function? It'll log as a FAIL though it was a pass. Similarly, between when the restore state function finishes and we remove the observer. 
Also, since we do not force a save, there's no check to see if save is called at all when we are restoring. 

In the second one, if the async restore completes before we call save, it'll show up as a PASS when it's a fail.

All that said, the tests work as expected on my PC (even the save attempts on the first one are not zero)

Thanks!
David:

Thanks for the feedback! 

> Nit: To simplify parsing the sentence, write "`busy`" (not just "busy").

Will do shortly.

> I'm not sure it's the right `Promise` to return. Shouldn't you rather return
> a promise that is resolved once the ongoing save is resolved?

I'm unsure about this. I just followed the preceding code, which deals with the case when we're using private browsing. That sends a resolved Promise and updates the time counter, in essence that "spoofs" a success case. I'd appreciate any ideas.

> > +  get busy() {
> > +    return SessionStoreInternal._browserSetState ||
> > +           Object.getOwnPropertyNames(SessionStoreInternal._windows)
> > +           .sort().reverse().some(id => SessionStoreInternal._windows[id].busy);
> 
> This deserves some documentation: I don't remember what `_windows[id].busy`
> means in this context, so it's not clear to me what this does.

_windows[id].busy is set when we call restoreWindow or restoreTab, and no single window can be busy while saving. I will do this shortly too. 

 
> Also, why do you even `sort()` and `reverse()`?

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1344715#c15 - In short, Mike told me to use this over `Object.values()`, and gave me an exercise to tell him why. The comment contains the details.
(In reply to milindl from comment #24)
> > Also, why do you even `sort()` and `reverse()`?
> 
> Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1344715#c15 - In
> short, Mike told me to use this over `Object.values()`, and gave me an
> exercise to tell him why. The comment contains the details.

So if there is a good reason, please document it in the code :)
Comment on attachment 8848149 [details]
Bug 1344715 - add test to check that we are not saving session while restoring,

https://reviewboard.mozilla.org/r/121120/#review126758

::: commit-message-3945f:1
(Diff revision 3)
> +Bug 1344715 - add test to check that we are not saving session while restoring[WIP],r?mikedeboer

Please remove 'WIP' from the commit message ;)

::: commit-message-3945f:4
(Diff revision 3)
> +Bug 1344715 - add test to check that we are not saving session while restoring[WIP],r?mikedeboer
> +
> +The test tries to restore a state, then tries to write. If write is successful,
> +then there is failure.

I think this can use a bit of re-formulation; 'Assert that we did not write anything to disk.' (or something like that)

::: commit-message-3945f:15
(Diff revision 3)
> +* trigger a session restore
> +* after restore is completed, remove observer and see if it was ever called
> +
> +and the second one follows this:
> +* set a really large timeout so that session saving doesn't happen on its own
> +* trigger a session restore asynch

nit: s/asynch/async/

::: browser/components/sessionstore/test/browser_save_while_restoring.js:7
(Diff revision 3)
> +
> +/**
> + * Ensure that session is not saved (written) while restoring session.
> + */
> +
> +var oldState = JSON.parse(SessionStore.getBrowserState());

nit: please rename this variable to denote that it's globally available: `gOriginalState = ...`.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:10
(Diff revision 3)
> + */
> +
> +var oldState = JSON.parse(SessionStore.getBrowserState());
> +add_task(async function test_save_while_restoring() {
> +  // Set the interval to a small number so that
> +  // we trigger a lot of saves

nit: I think this comment can fit on a single line and it's missing a dot at the end.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:11
(Diff revision 3)
> +
> +var oldState = JSON.parse(SessionStore.getBrowserState());
> +add_task(async function test_save_while_restoring() {
> +  // Set the interval to a small number so that
> +  // we trigger a lot of saves
> +  gPrefService.setIntPref("browser.sessionstore.interval", 1);

Please use `await SpecialPowers.pushPrefEnv()`, because then you won't have to use the cleanup function.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:16
(Diff revision 3)
> +  gPrefService.setIntPref("browser.sessionstore.interval", 1);
> +  registerCleanupFunction(() => {
> +    gPrefService.clearUserPref("browser.sessionstore.interval");
> +  });
> +
> +    // Restore this arbitrary `state` to our browser

nit: missing dot.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:33
(Diff revision 3)
> +        { entries: [{ url: "http://example.com#3", triggeringPrincipal_base64 }] },
> +        { entries: [{ url: "http://example.com#4", triggeringPrincipal_base64 }] },
> +        { entries: [{ url: "http://example.com#5", triggeringPrincipal_base64 }] },
> +        { entries: [{ url: "http://example.com#6", triggeringPrincipal_base64 }] }
> +      ],
> +      selected: 0

nit: can you use a different index here, so that another tab is selected in the second window?

::: browser/components/sessionstore/test/browser_save_while_restoring.js:43
(Diff revision 3)
> +    stateSavedWhileRestoring = true;
> +  };
> +
> +  Services.obs.addObserver(wroteStateObserver,
> +			   "sessionstore-state-write-complete", false);
> +  // While observing for any saves, restore `state`

nit: missing dot.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:48
(Diff revision 3)
> +  // While observing for any saves, restore `state`
> +  await promiseBrowserState(state);
> +  Services.obs.removeObserver(wroteStateObserver,
> +			      "sessionstore-state-write-complete");
> +
> +  ok(!stateSavedWhileRestoring, "Expected that state would not be written while restoring.");

nit: please use `Assert.ok()` in newer tests.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:55
(Diff revision 3)
> +  // Cleanup
> +  let windowsEnum = Services.wm.getEnumerator("navigator:browser");
> +  while (windowsEnum.hasMoreElements()) {
> +    let currWin = windowsEnum.getNext();
> +    if (currWin !== window)
> +      await BrowserTestUtils.closeWindow(currWin);

nit: missing braces. The sessionstore component uses this style, so we have to adhere to it.

::: browser/components/sessionstore/test/browser_save_while_restoring_2.js:6
(Diff revision 3)
> +"use strict";
> +/**
> + * Ensure that session is not saved (written) while restoring session.
> +*/
> +var oldState = JSON.parse(SessionStore.getBrowserState());
> +add_task(async function test_save_while_restoring() {

You can add this test as a second `add_task` in the file above, so you don't have to create a while new file here. Please keep the comment that explains what the test does, though!

::: browser/components/sessionstore/test/browser_save_while_restoring_2.js:33
(Diff revision 3)
> +        { entries: [{ url: "http://example.com#5", triggeringPrincipal_base64 }] },
> +        { entries: [{ url: "http://example.com#6", triggeringPrincipal_base64 }] }
> +      ],
> +      selected: 0
> +    }
> +  ] };

When you moved this task, you can put this state object into a global variable, named something like `var gStateToLoad = { ... }`

::: browser/components/sessionstore/test/browser_save_while_restoring_2.js:37
(Diff revision 3)
> +    }
> +  ] };
> +  let stateSavedWhileRestoring = false;
> +
> +  // Start a restore, and immediately after restoring,
> +  // check if we've saved state

nit: I think this comment can fit on a single line and it's missing a dot.

::: browser/components/sessionstore/test/browser_save_while_restoring_2.js:39
(Diff revision 3)
> +  let stateSavedWhileRestoring = false;
> +
> +  // Start a restore, and immediately after restoring,
> +  // check if we've saved state
> +  let promiseRestore = promiseBrowserState(state).then(() => {
> +    ok(!stateSavedWhileRestoring,

Please put this assertion _after_ `await promiseRestore;`, because then you know it's easier to follow that it's always executed. Also, please use `Assert.ok` here. See the full documentation for it at https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Assert.jsm
Attachment #8848149 - Flags: review?(mdeboer) → review-
Comment on attachment 8848149 [details]
Bug 1344715 - add test to check that we are not saving session while restoring,

https://reviewboard.mozilla.org/r/121120/#review126764

::: browser/components/sessionstore/test/browser_save_while_restoring.js:57
(Diff revision 3)
> +  while (windowsEnum.hasMoreElements()) {
> +    let currWin = windowsEnum.getNext();
> +    if (currWin !== window)
> +      await BrowserTestUtils.closeWindow(currWin);
> +  }
> +  await promiseBrowserState(oldState);

Perhaps it'd be cool/ good to leave the observer only to remove it here and check:
```js
await promiseBrowserState(gOriginalState);
Assert.ok(stateSavedWhileRestoring, "It's more than OK to have had a state save by now.");
Services.obs.removeObserver(wroteStateObserver,
  "sessionstore-state-write-complete");
```
(In reply to milindl from comment #24)
> > I'm not sure it's the right `Promise` to return. Shouldn't you rather return
> > a promise that is resolved once the ongoing save is resolved?
> 
> I'm unsure about this. I just followed the preceding code, which deals with
> the case when we're using private browsing. That sends a resolved Promise
> and updates the time counter, in essence that "spoofs" a success case. I'd
> appreciate any ideas.

What you need to do here is make sure that _writeState temporarily stores the promise it returns, for example in a member variable called `_writingStatePromise` and return that when it's set. You need to make sure to unset it when the promise is resolved OR rejected.
NB: if you return Promise.resolve() after all, make sure update the lastSaveTime as well in that case.
Comment on attachment 8848149 [details]
Bug 1344715 - add test to check that we are not saving session while restoring,

https://reviewboard.mozilla.org/r/121120/#review126758

> Please use `await SpecialPowers.pushPrefEnv()`, because then you won't have to use the cleanup function.

I'm using `pushPrefEnv`, and `popPrefEnv`, but the test fails if I do not `flushPrefEnv` after pushing. Any ideas?
Comment on attachment 8848149 [details]
Bug 1344715 - add test to check that we are not saving session while restoring,

https://reviewboard.mozilla.org/r/121120/#review127182

I think you need to take a closer look at the pref setting mechanics in these tests.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:31
(Diff revision 4)
> +    ],
> +    selected: 1
> +  }
> +] };
> +
> +

nit: superfluous newline.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:40
(Diff revision 4)
> +    let currWin = windowsEnum.getNext();
> +    if (currWin !== window) {
> +      await BrowserTestUtils.closeWindow(currWin);
> +    }
> +  }
> +}

nit: missing newline between function body and comment block.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:54
(Diff revision 4)
> + */
> +add_task(async function () {
> +  await SpecialPowers.pushPrefEnv({
> +    set: [[ "browser.sessionstore.interval", 1 ]]
> +  });
> +  await SpecialPowers.flushPrefEnv();

I'm guessing you don't need it here. You might want to move it above the `pushPrefEnv` call.

::: browser/components/sessionstore/test/browser_save_while_restoring.js:92
(Diff revision 4)
> +add_task(async function() {
> +  // Any saves must be forced on our part.
> +  await SpecialPowers.pushPrefEnv({
> +    set: [["browser.sessionstore.interval", 100000]]
> +  });
> +  await SpecialPowers.flushPrefEnv();

I *think* a flush resets everything back to their respective defaults, so this is certainly not doing what you expect it to.
Attachment #8848149 - Flags: review?(mdeboer) → review-
Mike:

While I have fixed the nits, I'm stuck with SpecialPowers.

I'm consistently getting FAILures (when I should get PASSes) on my machine.

I'm using

>  await SpecialPowers.pushPrefEnv({
>    set: [["browser.sessionstore.interval", <1 or 100000 here>]]
>  });

both with and without popPrefEnv.

In both cases, I'm getting failures. The tests work with the earlier

>  gPrefService.setIntPref("browser.sessionstore.interval", <1 or 100000 here>);
>    registerCleanupFunction(() => {
>      gPrefService.clearUserPref("browser.sessionstore.interval");
>    });

I've been stuck here for quite some time (I've tried playing around with flush before pushing, I've tried registering pop as a cleanup function, but nothing has worked so far).

Thanks!
(In reply to milindl from comment #32)
> In both cases, I'm getting failures. The tests work with the earlier
> 
> >  gPrefService.setIntPref("browser.sessionstore.interval", <1 or 100000 here>);
> >    registerCleanupFunction(() => {
> >      gPrefService.clearUserPref("browser.sessionstore.interval");
> >    });

OK, then please post the patch using this mechanism. Thanks!

> 
> I've been stuck here for quite some time (I've tried playing around with
> flush before pushing, I've tried registering pop as a cleanup function, but
> nothing has worked so far).

Sorry to hear that; tests are usually the most intense bit for session store related work, so I'm glad you're putting in the effort!

Can you also push an updated patch for part 1? Thanks!
Attachment #8848149 - Attachment is obsolete: true
Two things, firstly, I'm working on another update for the first patch; wherein I use a promise which resolves once the ongoing restore is completed, and then triggers a new save. I think this would be ideal :)

This idea should become somewhat clearer since I'll post a patch today itself.

Also, I was unsure about Hg when I started this, and so was using 2 different heads to make this work. I rebased them today and marked the older attachment obsolete.

Thanks! :)
Comment on attachment 8852844 [details]
Bug 1344715 - while restoring, reschedule any session writes that take place,

https://reviewboard.mozilla.org/r/125016/#review128088

Oops! Looks like you forgot to squash you commits! Looks like I can't really review it properly like this :(
Attachment #8852844 - Flags: review?(mdeboer) → review-
Attachment #8847190 - Attachment is obsolete: true
Attachment #8852839 - Attachment is obsolete: true
Attachment #8852839 - Flags: review?(mdeboer)
Sorry, I have it fixed as of now. I wanted to prototype a new idea, so I hadn't folded my commits together. It should be fine now (the commits contained distinct ideas, the first one used _writingStatePromise, while the second one endeavours to return a promise which waits till the restore is complete and then causes another save).

I'd appreciate thoughts on this idea

Thanks!:)
Comment on attachment 8852844 [details]
Bug 1344715 - while restoring, reschedule any session writes that take place,

https://reviewboard.mozilla.org/r/125016/#review128528

::: browser/components/sessionstore/SessionSaver.jsm:197
(Diff revision 2)
> +          resolve(this._saveState(forceUpdateAllWindows));
> +          Services.obs.removeObserver(restoreCompleteObserver,
> +                                      "sessionstore-browser-state-restored");
> +        };
> +        Services.obs.addObserver(restoreCompleteObserver,
> +                                 "sessionstore-browser-state-restored", false);

I'm afraid that this notification is not broadcasted in all cases where `SessionStore.busy` can be `true` as well. Imagine 'Undo close last window' or 'Undo close last tab'...

I'd also like limit the interaction between the SessionSaver and SessionStore modules as much as possible.

How about something like a new method on SessionStore, like `Sessionstore.whenBusyEnded(() => resolve(this._saveState(forceUpdateAllWindows)))`

It will do something similar like you did here, but for a new observer service message - i.e. 'sessionstore-busy-end' - that'll be broadcasted in `SessionStore._sendRestoreCompletedNotifications` and `SessionStore._setWindowStateReady`:

```js
if (!this.busy) {
  Services.obs.notifyObservers(null, NOTIFY_BUSY_END, "");
}
```

Tying all this together is an exercise to the reader ;-)
Attachment #8852844 - Flags: review?(mdeboer) → review-
Comment on attachment 8853422 [details]
Bug 1344715 - adds test to check that we are not saving session while restoring,

https://reviewboard.mozilla.org/r/125522/#review128900

LGTM - we'll see what the Try servers says once part 1 is finished. Thanks!
Attachment #8853422 - Flags: review?(mdeboer) → review+
Comment on attachment 8852844 [details]
Bug 1344715 - while restoring, reschedule any session writes that take place,

https://reviewboard.mozilla.org/r/125016/#review129380

Almost there! I'd like to take another look at your final patch. Do your tests still pass as well with the changes you made here?

::: browser/components/sessionstore/SessionSaver.jsm:188
(Diff revision 3)
>      }
>  
> +    if (SessionStore.busy) {
> +      // Do not save anything immediately. Instead, schedule another save once
> +      // the ongoing restore finishes, and busy ends.
> +      return new Promise((resolve, reject) => {

You can make this `return new Promise(resolve => {`, because you're not using the `reject` callback.

::: browser/components/sessionstore/SessionSaver.jsm:189
(Diff revision 3)
>  
> +    if (SessionStore.busy) {
> +      // Do not save anything immediately. Instead, schedule another save once
> +      // the ongoing restore finishes, and busy ends.
> +      return new Promise((resolve, reject) => {
> +        SessionStore.whenBusyEnded(

I think you can keep this one line, so it becomes:

```js
return new Promise(resolve =>
  SessionStore.whenBusyEnded(() => resolve(this._saveState(forceUpdateAllWindows)));
```

::: browser/components/sessionstore/SessionStore.jsm:219
(Diff revision 3)
>    set canRestoreLastSession(val) {
>      SessionStoreInternal.canRestoreLastSession = val;
>    },
>  
> +  get busy() {
> +    // We need to check if we're setting the browser state

Ah, I forgot to mention this during previous reviews! Please make this getter delegate to `return SessionStoreInternal.busy;` and move the implementation to that object instead.
The `whenBusyEnded` method should be placed below this getter to have the two grouped logically.

::: browser/components/sessionstore/SessionStore.jsm:4365
(Diff revision 3)
>  
>      // This was the last window restored at startup, notify observers.
>      Services.obs.notifyObservers(null,
>        this._browserSetState ? NOTIFY_BROWSER_STATE_RESTORED : NOTIFY_WINDOWS_RESTORED,
>        "");
> -
> +    if (!SessionStore.busy) {

`is (!this.busy) {` should work too.

::: browser/components/sessionstore/SessionStore.jsm:4404
(Diff revision 3)
>      this._windowBusyStates.set(aWindow, newCount);
>  
>      if (newCount == 0) {
>        this._setWindowStateBusyValue(aWindow, false);
>        this._sendWindowStateEvent(aWindow, "Ready");
> +      if (!SessionStore.busy) {

`if (!this.busy) {` should work too
Attachment #8852844 - Flags: review?(mdeboer) → review-
Comment on attachment 8852844 [details]
Bug 1344715 - while restoring, reschedule any session writes that take place,

https://reviewboard.mozilla.org/r/125016/#review129380

Yes, they do pass.:)
Comment on attachment 8852844 [details]
Bug 1344715 - while restoring, reschedule any session writes that take place,

https://reviewboard.mozilla.org/r/125016/#review129440

Cool! r=me with nits addressed. You can push those last two mini changes to MozReview, so I can land the commits for you - after a successful try run.

::: browser/components/sessionstore/SessionStore.jsm:4359
(Diff revision 4)
>      // This was the last window restored at startup, notify observers.
>      Services.obs.notifyObservers(null,
>        this._browserSetState ? NOTIFY_BROWSER_STATE_RESTORED : NOTIFY_WINDOWS_RESTORED,
>        "");
> -
> +    if (!this.busy) {
> +      Services.obs.notifyObservers(null, NOTIFY_BUSY_END, false);

nit: can you make these "" instead of false?

::: browser/components/sessionstore/SessionStore.jsm:4398
(Diff revision 4)
>  
>      if (newCount == 0) {
>        this._setWindowStateBusyValue(aWindow, false);
>        this._sendWindowStateEvent(aWindow, "Ready");
> +      if (!this.busy) {
> +        Services.obs.notifyObservers(null, NOTIFY_BUSY_END, false);

nit: same here.
Attachment #8852844 - Flags: review?(mdeboer) → review+
(In reply to milindl from comment #56)
> Comment on attachment 8853422 [details]
> Bug 1344715 - adds test to check that we are not saving session while
> restoring,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/125522/diff/4-5/

Fixes linting error, sorry!
Make:

Okay, so some of the tests, specifically, the tests I have written, seem to fail on try servers (I think). I cannot replicate the failure locally (I am running `./mach mochitest browser/components/sessionstore/test/browser_*` as well as `./mach mochitest browser/components/sessionstore/test/browser_save_while_restoring.js` to test it out locally).

Any ideas on how to tackle this issue?

Thanks!
(In reply to milindl from comment #60)
> Comment on attachment 8853422 [details]
> Bug 1344715 - adds test to check that we are not saving session while
> restoring,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/125522/diff/5-6/
Mike:

This is a small change - I was checking `this.busy` before actually setting `_browserSetState` to false, so it always returned true - in essence the NOTIFY_BUSY_END was never sent out.

Regarding the tests, I am having some issues (they failed on try server):

1. I cannot reproduce the failures locally - I tried doing it in different ways/using chunks etc, but I cannot seem to reproduce failures
2. When I remove the if 'busy' clause code from SessionSaver (it should fail), then only 2/3 tests fail [The tests using sessionstore interval = 1].
3. If I remove the tests using sessionstore interval = 1, and keep only the test using sessionstore interval = 100000, then the tests pass and fail as expected. 

In summary, keeping/removing the first two tests affects the outcome of the last test - and I am stuck at this point, since I do not know why this could be happening. I tried some things like having 1 total chunk and running that, but that does not affect my results.

Thanks!
Flags: needinfo?(mdeboer)
Whiteboard: [fxperf]
Milind, did you uncover anything else to do with this bug? Are you still planning on attempting to resolve the try failures or have you essentially moved on to other things?
Flags: needinfo?(i.milind.luthra)
(In reply to Doug Thayer [:dthayer] from comment #62)
> Milind, did you uncover anything else to do with this bug? Are you still
> planning on attempting to resolve the try failures or have you essentially
> moved on to other things?

I haven't seen this bug since I attempted it last year. If this is still relevant, and if it doesn't need to be looked at immediately, I could take a stab at it in the summers.
Flags: needinfo?(i.milind.luthra)
Marking this as fxperf:p5 for now, because it's not clear what the impact is.
Flags: needinfo?(mdeboer)
Whiteboard: [fxperf] → [fxperf:p5]
(In reply to :Gijs (he/him) from comment #64)
> Marking this as fxperf:p5 for now, because it's not clear what the impact is.

Mike, is this fair? Is this easy to fix or not really?
Flags: needinfo?(mdeboer)
I think so, since it hasn't come up in profiles after this was reported.

The first patch would need to change a little, because of 1) bitrot and 2) I'd like to see a different method of state keeping after all.
I don't know for sure if that'd fix the test failures, but it probably would help.
Flags: needinfo?(mdeboer)
Milind, please feel free to re-assign yourself when you have time!
Assignee: i.milind.luthra → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: