911115 - [Session Restore] Tests for tab state cache

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Comment 1

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

Attachment: Simple tests for tab state cache

Comment 2

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

Comment on attachment 797975 [details] [diff] [review]
Simple tests for tab state cache

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

Here's a first batch of tests. We should certainly complete it (in a followup bug?) with tests for everything that can invalidate.

Comment 3

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

Well, well, well. If the tests are correct, there is indeed a caching bug somewhere:
https://tbpl.mozilla.org/?tree=Try&rev=08066a16c73f

Comment 4

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

Attachment: Simple tests for tab state cache, v2

Comment 5

[Steven MacLeod [:smacleod]]

Comment on attachment 798528 [details] [diff] [review]
Simple tests for tab state cache, v2

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

Just a few little things, tests look good overall.

::: browser/components/sessionstore/test/browser_tabStateCache.js
@@ +52,5 @@
> +  yield promiseBrowserLoaded(tab2.linkedBrowser);
> +  {
> +    let PREFIX = "Adding second tab: ";
> +    let delta = yield getTelemetryDelta(forceSaveState);
> +    is(delta.hits, 2, PREFIX + " we hit one tab");

Having |2| hits for "one tab", and |3| hits for "both tabs hit" is kind of strange. What's going on here that should be causing the extra hit? Maybe it should be documented?

@@ +106,5 @@
> +        yield forceSaveState();
> +      });
> +    });
> +    ok(delta.hits > 0, PREFIX + " has at least one hit " + delta.hits);
> +    ok(delta.misses > 0, PREFIX + " has at least one miss " + delta.misses);

Should these be switched to equality like the other tests as well?

::: browser/components/sessionstore/test/head.js
@@ +255,5 @@
> +  const PREF = "browser.sessionstore.interval";
> +  // Set interval to an arbitrary non-0 duration
> +  // to ensure that setting it to 0 will notify observers
> +  Services.prefs.setIntPref(PREF, 1000);
> +  Services.prefs.setIntPref(PREF, 0);

Why aren't we calling |Services.prefs.clearUserPref()| anymore after resolving the promise? (Like in the replaced |forceWriteState()|

Comment 6

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

Attachment: Simple tests for tab state cache, v3

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 801539 [details] [diff] [review]
Simple tests for tab state cache, v3

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +308,5 @@
>  
> +  // access to the tab state cache, for testing purposes
> +  get _tabStateCache() {
> +    return TabStateCache;
> +  },

How about adding a TabStateCache.jsm? That seems easier and cleaner to me than forwarding those modules here. I didn't see any dependencies that would prevent us from doing so, right? I wouldn't mind that exporting the Telemetry module as well, although it's internal.

We could also think about exporting internal objects in test mode only? Is there some pre-processor define for testing mode? We could then export a different list maybe.

::: browser/components/sessionstore/test/Makefile.in
@@ +31,5 @@
>  	browser_sessionStorage.js \
> +	browser_tabStateCache.js \
> +	$(NULL)
> +
> +NOTHING = \

Debug code?

::: browser/components/sessionstore/test/browser_tabStateCache.js
@@ +1,5 @@
> +"use strict";
> +
> +let cache = SessionStore._internal._tabStateCache;
> +let telemetry = SessionStore._internal._tabStateCacheTelemetry;
> +let otherTabs = 0;

Maybe numOtherTabs? I'd expect that variable to be an array of tabs otherwise.

@@ +3,5 @@
> +let cache = SessionStore._internal._tabStateCache;
> +let telemetry = SessionStore._internal._tabStateCacheTelemetry;
> +let otherTabs = 0;
> +
> +let URL_PREFIX = "http://example.org:80/"; // arbitrary URL

const?

@@ +34,5 @@
> +
> +add_task(function init() {
> +  // Start with an empty cache
> +  closeAllButPrimaryWindow();
> +  SessionStore._internal._tabStateCache.clear();

You should use the 'cache' reference here.

@@ +48,5 @@
> +  yield getTelemetryDelta(forceSaveState);
> +
> +  // Save/collect again a few times, ensure that we always hit
> +  info("Save/collect a few times with one tab");
> +  {

Why do we need that extra block here?

@@ +74,5 @@
> +  }
> +
> +  // Save/collect again a few times, ensure that we always hit
> +  info("Save/collect a few times with two tabs");
> +  {

Why the extra block?

@@ +91,5 @@
> +  gBrowser.removeTab(tab2);
> +  {
> +    let PREFIX = "Removing second tab: ";
> +    let delta = yield getTelemetryDelta(forceSaveState);
> +    ok(delta.hits, otherTabs + 1, PREFIX + " we hit for one tab");

This should be is().

@@ +108,5 @@
> +  yield promiseBrowserLoaded(browser1);
> +  yield forceSaveState();
> +
> +  info("Opening second browsing tab");
> +  let tab2 = gBrowser.addTab("about:credits");

about:credits is a remote page. We should use something different in tests.

@@ +115,5 @@
> +
> +  for (let url of [URL_PREFIX + "?browsing1",
> +                   URL_PREFIX + "?browsing2",
> +                   URL_PREFIX + "?browsing3",
> +                   URL_PREFIX + "?browsing4"]) {

Why not use a for loop from 1 to 4?

::: browser/components/sessionstore/test/head.js
@@ +255,5 @@
> +  const PREF = "browser.sessionstore.interval";
> +  // Set interval to an arbitrary non-0 duration
> +  // to ensure that setting it to 0 will notify observers
> +  Services.prefs.setIntPref(PREF, 1000);
> +  Services.prefs.setIntPref(PREF, 0);

Like Steven mentioned, these prefs should be cleared when the promise is resolved.

@@ +267,5 @@
>    }, true);
>  }
> +function promiseBrowserLoaded(aBrowser) {
> +  let deferred = Promise.defer();
> +  whenBrowserLoaded(aBrowser, () => deferred.resolve());

whenBrowserLoaded(aBrowser, deferred.resolve);

@@ +278,5 @@
> +  }, true);
> +}
> +function promiseBrowserUnloaded(aBrowser, aContainer) {
> +  let deferred = Promise.defer();
> +  whenBrowserUnloaded(aBrowser, aContainer, () => deferred.resolve());

whenBrowserUnloaded(aBrowser, aContainer, deferred.resolve);

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Running the test locally (only the one test) works. Running the complete test suite gives failures. Maybe you have a clue what's happening here?

TEST-UNEXPECTED-FAIL | /test/browser_tabStateCache.js | Adding second tab:  we hit one tab - Got 0, expected 2
TEST-UNEXPECTED-FAIL | /test/browser_tabStateCache.js | Adding second tab:  we missed one tab - Got 3, expected 1
TEST-UNEXPECTED-FAIL | /test/browser_tabStateCache.js | Removing second tab:  we hit for one tab - Got 0, expected 2
TEST-UNEXPECTED-FAIL | /test/browser_tabStateCache.js | Removing second tab:  has no miss - Got 2, expected 0
TEST-UNEXPECTED-FAIL | /test/browser_tabStateCache.js | Browsing to http://example.org:80/?browsing1 has at least one hit - Got 0, expected 2
TEST-UNEXPECTED-FAIL | /test/browser_tabStateCache.js | Browsing to http://example.org:80/?browsing1 has one miss - Got 3, expected 1

Comment 9

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

I have the exact same problem.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

I bisected it down to the following setup:

MOCHITEST_BROWSER_FILES = \
	head.js \
	browser_624727.js \
	browser_tabStateCache.js \
	$(NULL)

If you run those tests, browser_tabStateCache.js fails. Removing only browser_624727.js still makes our test fail but at least it's a pointer.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

browser_624727.js doesn't seem to clean up properly. We can easily fix that by doing:

+++ b/browser/components/sessionstore/test/browser_624727.js
@@ -3,6 +3,8 @@

 function test() {
-  waitForExplicitFinish();
+  TestRunner.run();
+}

+function runTests() {
   let assertNumberOfTabs = function (num, msg) {
     is(gBrowser.tabs.length, num, msg);
@@ -28,5 +30,5 @@ function test() {

   // run the test
-  waitForBrowserState(
+  yield waitForBrowserState(
     { windows: [{ tabs: [{ url: "about:blank" }] }] },
     function () {
@@ -34,5 +36,5 @@ function test() {
       assertNumberOfPinnedTabs(0, "there are no pinned tabs");
       is(gBrowser.tabs[0].linkedBrowser, linkedBrowser, "first tab's browser got re-used");
-      finish();
+      next();
     }
   );

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

The other failures are caused by Panorama calling .setTabValue() in the background. There is no good way to solve that but we can work around it by making our .setTabValue() method a little more clever with regards to updating cached state:

++ b/browser/components/sessionstore/src/SessionStore.jsm
@@ -1670,31 +1670,33 @@ let SessionStoreInternal = {

   setTabValue: function ssi_setTabValue(aTab, aKey, aStringValue) {
-    TabStateCache.delete(aTab);
     // If the tab hasn't been restored, then set the data there, otherwise we
     // could lose newly added data.
     let saveTo;
     if (aTab.__SS_extdata) {
       saveTo = aTab.__SS_extdata;
     }
     else if (aTab.linkedBrowser.__SS_data && aTab.linkedBrowser.__SS_data.extData) {
       saveTo = aTab.linkedBrowser.__SS_data.extData;
     }
     else {
       aTab.__SS_extdata = {};
       saveTo = aTab.__SS_extdata;
     }
     saveTo[aKey] = aStringValue;
+
+    // Update the tab state cache and schedule a save operation.
+    TabStateCache.update(aTab, "extData", saveTo);
     this.saveStateDelayed(aTab.ownerDocument.defaultView);
   },

We could do the same for .deleteTabValue() although that doesn't affect the tests introduced here. Also we would need to modify TabStateCache.update() or introduce a new method that supports removing keys from a cached tab state.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

The whole test suites is finishing successfully on my machine with those two changes.

Comment 14

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

Attachment: Moving TabStateCache to its own module

Comment 15

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

Attachment: Simple tests for tab state cache, v4

Applied your feedback, adapted to TabStateCache.jsm.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 803724 [details] [diff] [review]
Moving TabStateCache to its own module

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1955,3 @@
>      if (this._updateTextAndScrollDataForTab(aTab, tabData)) {
>        TabStateCache.set(aTab, tabData);
>      }

Instead of exposing 'TabData', couldn't we just let TabStateCache.set() wrap the tab data in TabData? There's no real point in having _collectTabData() return a TabData object if it didn't come from the cache, no? AFAIU TabData behaves quite transparently.

::: browser/components/sessionstore/src/TabStateCache.jsm
@@ +16,5 @@
> + * This cache implements a weak map from tabs (as XUL elements)
> + * to tab data (as instances of TabData).
> + *
> + * Note that we should never cache private data, as:
> + * - that data is used very seldom by SessionStore;

To be exact: it's only used when duplicating tabs.

@@ +71,5 @@
> +   * @param {string} aField The field to update.
> +   * @param {*} aValue The new value to place in the field.
> +   */
> +  updateField: function(aKey, aField, aValue) {
> +    return TabStateCacheInternal.udpateField(aKey, aField, aValue);

*updateField

@@ +239,5 @@
> +  _hits: 0,
> +  // Total number of misses during the session
> +  _misses: 0,
> +  // Total number of clears during the session
> +  _clears: 0,

These fields should maybe not have a leading underscore as they're accessed by the TabStateCache object providing the external API.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 803732 [details] [diff] [review]
Simple tests for tab state cache, v4

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

::: browser/components/sessionstore/test/Makefile.in
@@ +33,1 @@
>          browser_upgrade_backup.js \

Could you maybe fix the browser_upgrade_backup.js indentation while you're here? Doesn't look off here but in vim it does. Thanks :)

::: browser/components/sessionstore/test/browser_tabStateCache.js
@@ +1,1 @@
> +"use strict";

You're missing a license header here.

Comment 18

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

One thing I've noticed that may or may not be caused by the tab state cache: if I scroll the foreground tab and then immediately quit, my scroll position won't be saved when I resume. That's because we don't invalidate the cache for scrolling, and the final data collection at shutdown uses the cached version.

Comment 19

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

Yes, that might well be caused the tab state cache.

Comment 20

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

Attachment: Simple tests for tab state cache, v5

Applied feedback.

Comment 21

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

(In reply to Tim Taubert [:ttaubert] from comment #16)
> Comment on attachment 803724 [details] [diff] [review]
> Moving TabStateCache to its own module
> 
> Review of attachment 803724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +1955,3 @@
> >      if (this._updateTextAndScrollDataForTab(aTab, tabData)) {
> >        TabStateCache.set(aTab, tabData);
> >      }
> 
> Instead of exposing 'TabData', couldn't we just let TabStateCache.set() wrap
> the tab data in TabData? There's no real point in having _collectTabData()
> return a TabData object if it didn't come from the cache, no? AFAIU TabData
> behaves quite transparently.

Oh, well, let's get rid of TabData for the moment. We'll reintroduce it if necessary.

Comment 22

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

(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> Yes, that might well be caused the tab state cache.

OK. I filed bug 916884 about this then.

Comment 23

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=bcd18f913939

Comment 24

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

Attachment: Simple tests for tab state cache, v6

Rebased.

Comment 25

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

Attachment: Moving TabStateCache to its own module, v2

Rebased

Comment 26

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

Attachment: Moving TabStateCache to its own module, v3

Unbitrotten.

Comment 27

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

Attachment: Simple tests for the tab state cahce, v7

Unbitrotten.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1a03be1791d7
https://hg.mozilla.org/integration/fx-team/rev/73495103969d

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Well, it was worth a shot anyway. Backed out for mochitest-bc failures.
https://hg.mozilla.org/integration/fx-team/rev/85d944acf789

https://tbpl.mozilla.org/php/getParsedLog.php?id=28251883&tree=Fx-Team

Comment 30

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

Hey, I had not marked it checkin-needed :)

Comment 31

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

Attachment: Simple tests for the tab state cache, v8

Fixed the tests to take into account the pre-fetching of TabStateCache.

Comment 32

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=b0393af7602f

Comment 33

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

Attachment: Moving TabStateCache to its own module, v4

Added missing qref.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/bc1591a843f9
https://hg.mozilla.org/integration/fx-team/rev/170d90b73805

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bc1591a843f9
https://hg.mozilla.org/mozilla-central/rev/170d90b73805