Closed Bug 1308061 Opened 8 years ago Closed 8 years ago

Implement sessionstore-closed-objects-changed event

Categories

(Firefox :: Session Restore, defect, P2)

Product:
Firefox
Component:
Session Restore
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [sessions]triaged)

Attachments

(1 file, 2 obsolete files)

Bug 1308061 - Implement sessionstore-closed-objects-changed event,
58 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
Implement browser.sessions.onChanged event, which is documented at https://developer.chrome.com/extensions/sessions#event-onChanged.
Mike, I have attached a patch for the changes to SessionStore.jsm to support the onChanged event. I have included tests for most of the instances in which the notification for sessionstore-closed-objects-changed will be issued. There are a few cases I haven’t been able to figure out, plus one case that just doesn’t seem to be working the way I expected. I would appreciate it if you could take a look and give me any advice you can offer on those items.

I also am not particularly happy with what I had to do with the restoreWindows function. The issue I came across is that the restoreWindows function itself sometimes does something which would require that a notification be sent, and it is also called from another method which also sometimes needs to notify. I didn’t want to cause two notifications when both of those situations would call for it, so I changed restoreWindows to return a boolean indicating whether a notification is required or not. As I say, I’m not really pleased with the resulting code, so if you can suggest something better that would be great.

The cases that still lack a test are:
- initializeWindow, at lines 1087, 1115, 1154 and 1165 of the diff. From what I can glean, initializeWindow will need to notify if we are currently restoring a session into the window, and a change is being made to this._closedWindows. I have looked over the code several times but remain confused by it and unsure of how to write a test. Also, if my initial statement above is true then maybe we don’t really need to notify during initializeWindow because there shouldn’t already be a valid set of closed windows in this._closedWindows. What do you think about this issue?

- maybeSaveClosedWindow, at line 1485 of the diff. In this case a window is already stored in _closedWindows, but should no longer be stored, and therefore it is removed, which causes the notification to be fired. It looks like the only scenario that would cause this is described in the comments at lines 1351-1362 and at 1385-1386 in the diff. If a tab in the window changes state from non-private to private between the first maybeSaveClosedWindow check and the second maybeSaveClosedWindow check, then it might already be stored but need to be removed. Or maybe I’m not really understanding the scenario - it’s pretty convoluted. In any case, I really have no idea how to test this case, so some guidance would be appreciated.

- saveClosedTabData, at line 1934 of the diff. There are two places in saveClosedTabData that could trigger a notification, so I wrote it so that only one notification would get fired even if both situations occur. The first one, which is at line 1929 in the diff is easy, it simply fires the notification when a tab is added to the list of closed tabs. It has a test. The second one is an issue. It fires the notification if a tab was added to the list of closed tabs, and then the list is larger than the _max_tabs_undo pref allows. That causes the list to be truncated, which also requires a notification. It doesn’t seem possible for the second situation to occur without the first one happening as well, so I don’t see how it’s possible to write a test for it. Either one (or both) cases will fire a single notification. I think it’s okay to just leave this uncovered, but I wanted to point it out.

- restoreLastSession, at line 2624 of the diff. When restoring a session, if there were closed windows in the last session they are merged with the current set of closed windows, which triggers a notification. I tried to find an existing test for restoreLastSession and none exist, so I’m not sure how to approach a test for this one.

Finally, I tried to write a test for the part of onIdleDaily which would notify if old closed tabs have been removed (line 2084 in the diff). For some reason my call to promiseBrowserState() which works just fine in the case of a closed window, never returns in the case of a closed tab, and I cannot figure out why. I have included the test in the test file, but commented out.
Attachment #8809099 - Attachment is obsolete: true
Attachment #8809099 - Flags: review?(mdeboer)
Andrew, while Part 2 does depend on Part 1, I don't think anything that might change in the implementation of Part 1 will have an effect on Part 2, so Part 2 can be reviewed now. Thanks!
Comment on attachment 8809099 [details]
Bug 1308061 - Part 1: Update SessionStore.jsm to notify when the list of closed objects changes,

https://reviewboard.mozilla.org/r/91754/#review93846

Why didn't you choose to keep a class-wide flag called `_storeChanged` and call `this._notifyOfClosedObjectsChange();` in all Sessionstore end-point API methods (observers and high-level API methods)?
In that case, the `notifyClosedObjectsChange` method would look like:
```js
_notifyOfClosedObjectsChange() {
  if (!this._storeChanged) {
    return;
  }
  this._storeChanged = false;
  setTimeout(() => {
    Services.obs.notifyObservers(null, NOTIFY_CLOSED_OBJECTS_CHANGED, null);
  }, 0);
},
```

It would make much of the book-keeping you're implementing here obsolete...

What do you think?
Comment on attachment 8811795 [details]
Bug 1308061 - Implement sessionstore-closed-objects-changed event,

https://reviewboard.mozilla.org/r/93750/#review93848
Attachment #8811795 - Flags: review?(mdeboer)
Comment on attachment 8809099 [details]
Bug 1308061 - Part 1: Update SessionStore.jsm to notify when the list of closed objects changes,

https://reviewboard.mozilla.org/r/91754/#review93846

Great idea, thanks! I have implemented it and pushed a new commit.
Comment on attachment 8811795 [details]
Bug 1308061 - Implement sessionstore-closed-objects-changed event,

https://reviewboard.mozilla.org/r/93750/#review94866

Looking much better, thanks!

::: browser/components/sessionstore/SessionStore.jsm:229
(Diff revision 2)
>      return SessionStoreInternal.getBrowserState();
>    },
>  
>    setBrowserState: function ss_setBrowserState(aState) {
>      SessionStoreInternal.setBrowserState(aState);
> +    SessionStoreInternal._notifyOfClosedObjectsChange();

I think it'd be better to put the calls to `_notifyOfClosedObjectsChange()` in the `SessionStoreInternal` equivalents. That would also take care of a few duplicate callsites (thinking of undo-by-id, for example).
(You denoted the method as private for a reason ;) )

::: browser/components/sessionstore/SessionStore.jsm:1950
(Diff revision 2)
> +    this._closedObjectsChanged = true;
>  
>      // Truncate the list of closed tabs, if needed.
>      if (closedTabs.length > this._max_tabs_undo) {
>        closedTabs.splice(this._max_tabs_undo, closedTabs.length);
> +      this._closedObjectsChanged = true;

Considering what you do above, you don't really need it here, right?

I think it's safe to say that this method _always_ changes closed objects data.

::: browser/components/sessionstore/SessionStore.jsm:3829
(Diff revision 2)
>    },
>  
>    /* ........ Auxiliary Functions .............. */
>  
>    /**
> +   * Remove a closed window from the list of closed

nit: what kind of character limit are you using here? Can you fix it to be 80ch - 120ch?

::: browser/components/sessionstore/SessionStore.jsm:3853
(Diff revision 2)
> +  _notifyOfClosedObjectsChange() {
> +    if (!this._closedObjectsChanged) {
> +      return;
> +    }
> +    this._closedObjectsChanged = false;
> +    setTimeout(() => {

How can it be that there's a global `setTimeout` available? I don't see it declared in here...

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js:7
(Diff revision 2)
> +
> +/**
> + * This test is for the sessionstore-closed-objects-changed notifications.
> + */
> +
> +Cu.import("resource:///modules/sessionstore/SessionStore.jsm");

I don't think you need to re-import SessionStore.jsm - see head.js.

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js:11
(Diff revision 2)
> +
> +Cu.import("resource:///modules/sessionstore/SessionStore.jsm");
> +
> +let notificationsCount = 0;
> +let topic = "sessionstore-closed-objects-changed";
> +let maxTabsUndoPrefName = "browser.sessionstore.max_tabs_undo";

Can you change constants to stand out a bit more, visually? We've got ALL_CAPS or kConstantName to choose from.

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js:24
(Diff revision 2)
> +}
> +
> +function closeWindow(win) {
> +  yield BrowserTestUtils.closeWindow(win);
> +  // Wait 20 ms to allow SessionStorage a chance to register the closed window.
> +  yield new Promise(resolve => setTimeout(resolve, 20));

hmmz, I'm not fond of using timeouts like this... can't we do something a bit more deterministic?

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js:29
(Diff revision 2)
> +  yield new Promise(resolve => setTimeout(resolve, 20));
> +}
> +
> +function* openTab(window, url) {
> +  let tab = window.gBrowser.addTab(url);
> +  yield promiseBrowserLoaded(tab.linkedBrowser, true, url);

Can you change this to `let tab = BrowserTestUtils.openNewForegroundTab(window.gBrowser, url);`?

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js:34
(Diff revision 2)
> +  yield promiseBrowserLoaded(tab.linkedBrowser, true, url);
> +  yield TabStateFlusher.flush(tab.linkedBrowser);
> +  return tab;
> +}
> +
> +function openAndCloseTab(window, url) {

Missing `*`

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js:112
(Diff revision 2)
> +
> +  info("Opening and closing another tab.");
> +  yield openAndCloseTab(win, "http://example.com/");
> +  assertNotificationCount(9);
> +
> +  /* TODO: I cannot seem to get this to work.

Basic tip I've got right now is to take a look at `browser_cleaner.js`. If you're having trouble here still, let's figure it out together on IRC.

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_windows.js:7
(Diff revision 2)
> +
> +/**
> + * This test is for the sessionstore-closed-objects-changed notifications.
> + */
> +
> +Cu.import("resource:///modules/sessionstore/SessionStore.jsm");

See my general comments on the test file above.

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_windows.js:53
(Diff revision 2)
> +  // run in the same session.
> +  let awaitPurge = TestUtils.topicObserved("browser:purge-session-history");
> +  Services.obs.notifyObservers(null, "browser:purge-session-history", 0);
> +  yield awaitPurge;
> +  // The purge needs time to settle before starting to wait for other notifications.
> +  yield new Promise(resolve => setTimeout(resolve, 20));

Why not simply `forgetClosedWindows();`?

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_windows.js:83
(Diff revision 2)
> +
> +  info(`Changing the ${maxWindowsUndoPrefName} pref.`);
> +  registerCleanupFunction(function () {
> +    Services.prefs.clearUserPref(maxWindowsUndoPrefName);
> +  });
> +  yield awaitNotification(() => {Services.prefs.setIntPref(maxWindowsUndoPrefName, 1);});

nit: you can omit the braces and trailing semi-colon:
`yield awaitNotification(() => Services.prefs.setIntPref(maxWindowsUndoPrefName, 1));`
(Also counts for occurrences below)
Attachment #8811795 - Flags: review?(mdeboer) → review-
Comment on attachment 8811795 [details]
Bug 1308061 - Implement sessionstore-closed-objects-changed event,

https://reviewboard.mozilla.org/r/93750/#review94866

> Considering what you do above, you don't really need it here, right?
> 
> I think it's safe to say that this method _always_ changes closed objects data.

Good point. :)

> hmmz, I'm not fond of using timeouts like this... can't we do something a bit more deterministic?

I'm not a big fan either, but this is actually something that we introduced in the last patch that landed, in the test browser_undoCloseById.js [1]. I believe it was your suggestion to address some intermittents that we were seeing. I'll poke around SessionStore.jsm to see if there is anything else that we could possibly wait for, but I fear there is not.

[1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_undoCloseById.js#26

> Basic tip I've got right now is to take a look at `browser_cleaner.js`. If you're having trouble here still, let's figure it out together on IRC.

`browser_cleaner.js` is where I looked when I originally wrote the code, so it is a good tip, but doesn't really explain why what I am trying to do isn't working. I did some digging into `promiseBrowserState` and I believe there is a bug in `waitForBrowserState` when one is working with multiple windows. I've added a bunch of debug code and have narrowed it down to the fact that `onSSTabRestored` [1] does not seem to be called for both tabs in both windows. I'm guessing that may be because the tab in the first window doesn't actually have a document in it - it's just the default empty tab you get when starting the browser. If we don't expect it to trigger `onSSTabRestored` then the internal value for `expectedTabsRestored` is being calculated incorrectly at [2], because it is `2` in this case, but that causes the promise to never be resolved. I'm not really sure which of the above two things are incorrect, or even if this is a bug. Do you mind taking a look and letting me know what you think? I'm also happy to chat with you about it on IRC. I can ping you tomorrow morning (my time).

[1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/head.js#108
[2] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/head.js#99

> Why not simply `forgetClosedWindows();`?

Yes, I could use `forgetClosedWindows` instead of this purge stuff above (especially as I don't care about tabs), but, having done a bot more experimenting, that's not what the wait above is needed for.

The problem for which I introduced this wait is that, without it, an "old" notification comes through during the test. I believe that the notification is being triggered by the previous test run. If I run this test in isolation I can remove the wait, but if I run it in a suite another notification gets registered by the listener. I could  change all this code to `yield awaitNotification(() => forgetClosedWindows());` and that seems to address the issue, but then the test cannot be run in isolation beacuse that `awaitNotification` never fires because there are no pre-existing closed windows. What I need to do is figure out how to not start observing notifications until the last notification from the previous test has fired, but I'm not sure how to do that. I believe that notification is triggered by the final window closing when the previous test finishes. Do you have any suggestions?
Comment on attachment 8811795 [details]
Bug 1308061 - Implement sessionstore-closed-objects-changed event,

https://reviewboard.mozilla.org/r/93750/#review94866

> I'm not a big fan either, but this is actually something that we introduced in the last patch that landed, in the test browser_undoCloseById.js [1]. I believe it was your suggestion to address some intermittents that we were seeing. I'll poke around SessionStore.jsm to see if there is anything else that we could possibly wait for, but I fear there is not.
> 
> [1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_undoCloseById.js#26

So when I looked at SessionStore.jsm I realized that the event that I just added is exactly the one that we can wait for here, so that fixes the issue!
Comment on attachment 8811795 [details]
Bug 1308061 - Implement sessionstore-closed-objects-changed event,

https://reviewboard.mozilla.org/r/93750/#review95220

I'm curious to see what you'll do with `promiseBrowserState()`, but I'm granting review regardless. This looks so much better now and I hope you agree!
Perhaps needless to say, but if you or others notice that a notfication is missing, I'll come looking for you to implement it ;-)
Well done!

::: browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js:109
(Diff revision 4)
> +  info("Opening and closing another tab.");
> +  yield openAndCloseTab(win, "http://example.com/");
> +  assertNotificationCount(9);
> +
> +  /* TODO: I cannot seem to get this to work.
> +     `yield promiseBrowserState(state)` never yields, and I'm not sure why.

If `promiseBrowserState()` doesn't yield, that means it's buggy. But you mentioned that before and shared your analysis.
I'd recommend fixing that method in head.js (because it's prolly expecting too many windows or tabs to open) or remove this specific test entirely. Obviously, I'd prefer the former :)
Attachment #8811795 - Flags: review?(mdeboer) → review+
Comment on attachment 8811795 [details]
Bug 1308061 - Implement sessionstore-closed-objects-changed event,

https://reviewboard.mozilla.org/r/93750/#review94866

> `browser_cleaner.js` is where I looked when I originally wrote the code, so it is a good tip, but doesn't really explain why what I am trying to do isn't working. I did some digging into `promiseBrowserState` and I believe there is a bug in `waitForBrowserState` when one is working with multiple windows. I've added a bunch of debug code and have narrowed it down to the fact that `onSSTabRestored` [1] does not seem to be called for both tabs in both windows. I'm guessing that may be because the tab in the first window doesn't actually have a document in it - it's just the default empty tab you get when starting the browser. If we don't expect it to trigger `onSSTabRestored` then the internal value for `expectedTabsRestored` is being calculated incorrectly at [2], because it is `2` in this case, but that causes the promise to never be resolved. I'm not really sure which of the above two things are incorrect, or even if this is a bug. Do you mind taking a look and letting me know what you think? I'm also happy to chat with you about it on IRC. I can ping you tomorrow morning (my time).
> 
> [1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/head.js#108
> [2] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/head.js#99

I've gone round and round trying to debug this, but I haven't gotten anywhere useful. I've gotten to the point, somehow, where simply calling `SessionStore.setBrowserState` causes the test to hang, after the state is restored,even though I am not wrapping it in a promise at all at that point. I'm not sure what's going on! If you have some time tomorrow to walk through some of the stuff I've tried I'd be happy to keep going, but at this point I'm not sure what to do other than remove the test that uses it.
Comment on attachment 8811795 [details]
Bug 1308061 - Implement sessionstore-closed-objects-changed event,

https://reviewboard.mozilla.org/r/93750/#review95220

> If `promiseBrowserState()` doesn't yield, that means it's buggy. But you mentioned that before and shared your analysis.
> I'd recommend fixing that method in head.js (because it's prolly expecting too many windows or tabs to open) or remove this specific test entirely. Obviously, I'd prefer the former :)

I created a reduced test case, and what I have found is that if I call `SessionStore.setBrowserState()` with a state that contains a single window, then everything works as expected, but if I call it with a state that contains two windows, then my test just hangs after the state is restored. There is nothing in my test that is waiting on anything, and I haven't been able to track down what is causing this hang. I can also see that there are existing tests that do just this, i.e., call `setBrowserState` with a state that contains two windows, and they pass, so I'm just not sure what's going on here. I've spent a huge amount of time trying to figure this out, so for now I'm just going to remove the test. :(
Comment on attachment 8811795 [details]
Bug 1308061 - Implement sessionstore-closed-objects-changed event,

https://reviewboard.mozilla.org/r/93750/#review94866

> Yes, I could use `forgetClosedWindows` instead of this purge stuff above (especially as I don't care about tabs), but, having done a bot more experimenting, that's not what the wait above is needed for.
> 
> The problem for which I introduced this wait is that, without it, an "old" notification comes through during the test. I believe that the notification is being triggered by the previous test run. If I run this test in isolation I can remove the wait, but if I run it in a suite another notification gets registered by the listener. I could  change all this code to `yield awaitNotification(() => forgetClosedWindows());` and that seems to address the issue, but then the test cannot be run in isolation beacuse that `awaitNotification` never fires because there are no pre-existing closed windows. What I need to do is figure out how to not start observing notifications until the last notification from the previous test has fired, but I'm not sure how to do that. I believe that notification is triggered by the final window closing when the previous test finishes. Do you have any suggestions?

When I submitted this to try it was failing because of an extra notification coming through in the test, which I think is precisely what I was trying to code against above. I made a change so that I wait for a notification on the purge, but there is still a risk that a notification from the previous test will come through and mess things up. I'll see how my latest version does on try, but if it's still an issue we'll have to try to think of something else.
Blocks: 1320306
I am updating this bug to be just for the patch for SessionStore so that can land independently.
Component: WebExtensions: General → Session Restore
Product: Toolkit → Firefox
Summary: Implement sessions.onChanged WebExtensions API → Implement sessionstore-closed-objects-changed event
Comment on attachment 8811793 [details]
Bug 1308061 - Part 2: Implement sessions.onChanged WebExtensions API,

Removing Part 2 from this bug.
Attachment #8811793 - Attachment is obsolete: true
Attachment #8811793 - Flags: review?(aswan)
This looks good on try. Requesting check in.
Keywords: checkin-needed
The patch needs a rebase.
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
Thanks for letting me know. I've rebased it, so it should be good to land now.
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b435f6479aa0
Implement sessionstore-closed-objects-changed event, r=mikedeboer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b435f6479aa0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: