867143 - [SessionStore] Cache state aggressively

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

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

During data collection, it looks like much of the time is spent recollecting already collected data. We should cache it aggressively.

Comment 1

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

Attachment: Caching state

First version of a TabCache.
I am also working on caching the results of JSON.stringify.

Comment 2

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

This looks excellent!

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 777112 [details] [diff] [review]
Caching state

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

Great patch. Needs just a little more work but I'm looking forward to get this in!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +123,5 @@
> +/**
> + * |true| if we are in debug mode, |false| otherwise.
> + * Debug mode is controlled by preference browser.sessionstore.debug
> + */
> +let DEBUG = false;

Can we name this gDebuggingEnabled or something just to stick to conventions? DEBUG looks like a constant to me.

@@ +622,5 @@
>        return;
>  
> +    try {
> +      switch (aTopic) {
> +        case "domwindowopened": // catch new windows

I'd rather have us file a bug to fix error reporting than start wrapping everything that may fail in a try-catch stmt. Our current error reporting is driving me nuts anyway so if you have a reproducible error this might be a good test case.

@@ +725,5 @@
>        case "TabPinned":
>        case "TabUnpinned":
>          this.saveStateDelayed(win);
>          break;
> +      case "MozStorageChanged":

This needs to check that (event.storageArea == sessionStorage) because we're not interested in localStorage etc. changes.

@@ +727,5 @@
>          this.saveStateDelayed(win);
>          break;
> +      case "MozStorageChanged":
> +        this.saveStateDelayed(win);
> +        break;

Good to have that change in even though it's hidden in a "cache state" patch. Might be worth to file an extra bug for it and write a test to ensure the state is marked as dirty when sessionStorage is changed.

@@ +1099,5 @@
>      this._lastSessionState = null;
>      let openWindows = {};
>      this._forEachBrowserWindow(function(aWindow) {
>        Array.forEach(aWindow.gBrowser.tabs, function(aTab) {
> +        TabCache.delete(this);

This should be TabCache.delete(aTab);

@@ +1324,5 @@
>        return;
>      }
>  
> +    // Get the latest data for this tab (generally, from the cache)
> +    let tabState = this._collectTabData(aWindow, aTab, aTab.linkedBrowser);

_collectTabData() should not take a browser argument. It takes a tab and we can just to tab.linkedBrowser to get its browser. The same goes for the window argument. tab.ownerDocument.defaultView is your friend.

@@ +1512,5 @@
>  
> +    let tabState = this._collectTabData(
> +      /*aWindow*/aTab.ownerDocument.defaultView,
> +      /*aTab*/aTab,
> +      /*aBrowser*/aTab.linkedBrowser);

(Just pass the tab as a single argument.)

@@ +1533,5 @@
> +    }
> +    if (!("entries" in tabState)) {
> +      throw new TypeError("State argument must contain field 'entries'");
> +    }
> +    if (!(aTab.ownerDocument)) {

Nit: no need for the double parenthesis here.

@@ +1540,5 @@
> +
> +    let window = aTab.ownerDocument.defaultView;
> +    if (!("__SSi" in window)) {
> +      throw new TypeError("Default view of ownerDocument must have a unique identifier");
> +    }

I appreciate the detailed error reporting here but we should make sure to set Components.returnCode to Cr.NS_ERROR_INVALID_ARG as this setTabState() is still part of an IDL interface.

@@ +1555,5 @@
> +    // Duplicate the tab state
> +    let tabState = this._cloneFullTabData(
> +      /*aWindow*/aTab.ownerDocument.defaultView,
> +      /*aTab*/aTab,
> +      /*aBrowser*/aTab.linkedBrowser);

(As the argument list demonstrates, all we need to pass is the tab.)

@@ +1719,5 @@
>      return data[aKey] || "";
>    },
>  
>    setTabValue: function ssi_setTabValue(aTab, aKey, aStringValue) {
> +    TabCache.delete(aTab);

What about deleteTabValue()? That even seems to be missing a saveStateDelayed() call just like deleteWindowValue().

@@ +1944,3 @@
>     */
> +  _collectTabData: function ssi_collectTabData(
> +      aWindow, aTab, aBrowser) {

(Should take a tab, only.)

@@ +1946,5 @@
> +      aWindow, aTab, aBrowser) {
> +    if (DEBUG) {
> +      if (!aWindow) {
> +        throw new TypeError();
> +      }

I think those are errors we should also throw in non-debug mode. How is sessionstore supposed to work without this? :)

@@ +1955,5 @@
> +        throw new TypeError();
> +      }
> +    }
> +    let tabData;
> +    if ((tabData = TabCache.get(aTab))) {

Shouldn't we just check TabCache.has()? In case you also wanted to check for !!tabData, this should probably be an invariant and we should check it before putting stuff into the cache.

@@ +1963,5 @@
> +    TabCache.set(aTab, tabData);
> +
> +    try {
> +      this._updateTextAndScrollDataForTab(aWindow, aBrowser, tabData, false);
> +    } catch (ex) {

Let's not wrap this in a try-catch stmt. This wasn't needed before and it certainly doesn't make the code more readable.

BTW, yeah I just found out the we actually do this in _updateTextAndScrollData() for some reason but we shouldn't be doing it. All the other critical methods are using it without.

@@ +1986,5 @@
> +   *
> +   * @returns {oject} An object with the data for this tab. This object
> +   * is recomputed at every call.
> +   */
> +  _cloneFullTabData: function ssi_cloneFullTabData(aWindow, aTab, aBrowser) {

(Should take a tab, only.)

@@ +1990,5 @@
> +  _cloneFullTabData: function ssi_cloneFullTabData(aWindow, aTab, aBrowser) {
> +    let tabData = this._collectBaseTabData(aTab, true);
> +    try {
> +      this._updateTextAndScrollDataForTab(aWindow, aBrowser, tabData, true);
> +    } catch (ex) {

Remove the try-catch stmt, please.

@@ +1999,5 @@
> +    }
> +    return tabData;
> +  },
> +
> +  _collectBaseTabData: function ssi_collectBaseTabData(aTab, aIncludePrivateData = false) {

Could we make the second argument an options object? That would make call sites much more readable like:

> this._collectBaseTabData(aTab, {includePrivateData: true});

@@ +2289,4 @@
>     *        always return privacy sensitive data (use with care)
>     */
>    _updateTextAndScrollDataForTab:
> +    function ssi_updateTextAndScrollDataForTab(aWindow, aBrowser, aTabData, aIncludePrivateData = false) {

This should take a tab, tabData and an options object.

@@ +2710,5 @@
>      if (!this._isWindowLoaded(aWindow))
>        return;
>  
> +    let tabbrowser = aWindow.gBrowser;
> +    let browsers = tabbrowser.browsers;

We won't need that with the changes below.

@@ +2720,2 @@
>      // update the internal state data for this window
> +    for (let i = 0; i < tabs.length; ++i) {

This should be:

> for (let tab of tabs) {

@@ +2720,5 @@
>      // update the internal state data for this window
> +    for (let i = 0; i < tabs.length; ++i) {
> +      let tab = tabs[i];
> +
> +      tabsData.push(this._collectTabData(aWindow, tab, browsers[i]));

We can use tab.linkedBrowser here.

@@ +2726,5 @@
> +      // Since we are only ever called for open
> +      // windows during a session, we can call into
> +      // _extractHostsForCookiesFromHostScheme directly using data
> +      // that is attached to each browser.
> +      let hostSchemeData = tab.linkedBrowser.__SS_hostSchemeData || [];

tab.linkedBrowser could be saved to a variable at the top of the iteration.

@@ +2733,5 @@
> +                                                   hostSchemeData[j].scheme,
> +                                                   hosts, true, tab.pinned);
> +      }
> +      // FIXME: We should be able to move this out of the loop
> +      winData.selected = tabbrowser.mTabBox.selectedIndex + 1;

I think so, too. What keeps you from doing so? :)

@@ +2745,5 @@
>        this._windows[aWindow.__SSi].__lastSessionWindowID =
>          aWindow.__SS_lastSessionWindowID;
>  
>      this._dirtyWindows[aWindow.__SSi] = false;
> +  } ,

Nit : no space here .

@@ +3034,5 @@
>     */
>    restoreHistoryPrecursor:
>      function ssi_restoreHistoryPrecursor(aWindow, aTabs, aTabData, aSelectTab,
>                                           aIx, aCount, aRestoreImmediately = false) {
> +      

Nit: strange white space change.

@@ +4873,2 @@
>    OnHistoryReload: function(aReloadURI, aReloadFlags) {
> +    TabCache.delete(this.tab);

Why do we need to invalidate here? After all, this listener is only registered for not-yet-restored tabs (esp. with deferred session restore enabled) and will just start loading the tab's restored URL. I don't think we should/ need to invalidate here.

@@ +4894,5 @@
>    return (index == (this.length - aDomain.length)) &&
>           (prevChar == "." || prevChar == "/");
> +};
> +
> +function TabData(obj = null) {

What do we need this extra wrapper for? Can't we just store the object in the WeakMap?

Comment 4

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

(In reply to Tim Taubert [:ttaubert] from comment #3)
> Comment on attachment 777112 [details] [diff] [review]
> Caching state
> @@ +622,5 @@
> >        return;
> >  
> > +    try {
> > +      switch (aTopic) {
> > +        case "domwindowopened": // catch new windows
> 
> I'd rather have us file a bug to fix error reporting than start wrapping
> everything that may fail in a try-catch stmt. Our current error reporting is
> driving me nuts anyway so if you have a reproducible error this might be a
> good test case.

Filed as bug 895340.
However, this looks rather non-trivial. I suspect this will require patching XPConnect, so I'm not holding my breath.
Unless you have a strong objection, I'd rather leave the try/catch in place for the time being.

> @@ +727,5 @@
> >          this.saveStateDelayed(win);
> >          break;
> > +      case "MozStorageChanged":
> > +        this.saveStateDelayed(win);
> > +        break;
> 
> Good to have that change in even though it's hidden in a "cache state"
> patch. Might be worth to file an extra bug for it and write a test to ensure
> the state is marked as dirty when sessionStorage is changed.

Filed as bug 895342.


> @@ +1719,5 @@
> >      return data[aKey] || "";
> >    },
> >  
> >    setTabValue: function ssi_setTabValue(aTab, aKey, aStringValue) {
> > +    TabCache.delete(aTab);
> 
> What about deleteTabValue()? That even seems to be missing a
> saveStateDelayed() call just like deleteWindowValue().

Ah, good point.

> @@ +1955,5 @@
> > +        throw new TypeError();
> > +      }
> > +    }
> > +    let tabData;
> > +    if ((tabData = TabCache.get(aTab))) {
> 
> Shouldn't we just check TabCache.has()? In case you also wanted to check for
> !!tabData, this should probably be an invariant and we should check it
> before putting stuff into the cache.

If we assume that we never put falsy in the TabCache (we kind of check that in TabCache.set with the |instanceof| check), just extracting the value with TabCache.get should give us the result. Or am I missing something?

> BTW, yeah I just found out the we actually do this in
> _updateTextAndScrollData() for some reason but we shouldn't be doing it. All
> the other critical methods are using it without.

Fine with me.

> @@ +4873,2 @@
> >    OnHistoryReload: function(aReloadURI, aReloadFlags) {
> > +    TabCache.delete(this.tab);
> 
> Why do we need to invalidate here? After all, this listener is only
> registered for not-yet-restored tabs (esp. with deferred session restore
> enabled) and will just start loading the tab's restored URL. I don't think
> we should/ need to invalidate here.

Actually, that object is something of a mystery to me, so my reflex was to be conservative. I'm more than willing to take your word on this.

> @@ +4894,5 @@
> >    return (index == (this.length - aDomain.length)) &&
> >           (prevChar == "." || prevChar == "/");
> > +};
> > +
> > +function TabData(obj = null) {
> 
> What do we need this extra wrapper for? Can't we just store the object in
> the WeakMap?

I introduced this for two reasons:
- it helps track type errors in TabCache;
- it introduces nicely bug 894969.

Comment 5

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

Attachment: Caching state, v2

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> > > +    try {
> > > +      switch (aTopic) {
> > > +        case "domwindowopened": // catch new windows
> > 
> > I'd rather have us file a bug to fix error reporting than start wrapping
> > everything that may fail in a try-catch stmt. Our current error reporting is
> > driving me nuts anyway so if you have a reproducible error this might be a
> > good test case.
> 
> Filed as bug 895340.
> However, this looks rather non-trivial. I suspect this will require patching
> XPConnect, so I'm not holding my breath.
> Unless you have a strong objection, I'd rather leave the try/catch in place
> for the time being.

I don't object strongly. I just doesn't feel right :) Feel free to let it in for now and let's hope we can get better error reporting soon.

> > > +    let tabData;
> > > +    if ((tabData = TabCache.get(aTab))) {
> > 
> > Shouldn't we just check TabCache.has()? In case you also wanted to check for
> > !!tabData, this should probably be an invariant and we should check it
> > before putting stuff into the cache.
> 
> If we assume that we never put falsy in the TabCache (we kind of check that
> in TabCache.set with the |instanceof| check), just extracting the value with
> TabCache.get should give us the result. Or am I missing something?

True, but I feel like it may be easier to read if we do:

if (TabCache.has(aTab)) {
  return TabCache.get(aTab);
}

let tabData = new TabData(this._collectBaseTabData(aTab, false));
TabCache.set(aTab, tabData);

I find this a little more expressive. Just a personal opinion.

> > > +function TabData(obj = null) {
> > 
> > What do we need this extra wrapper for? Can't we just store the object in
> > the WeakMap?
> 
> I introduced this for two reasons:
> - it helps track type errors in TabCache;
> - it introduces nicely bug 894969.

Yeah it made a lot more sense after I reviewed bug 894969 :)

Comment 7

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

Attachment: Caching state, v3

Sorry forgot to save before href.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 777737 [details] [diff] [review]
Caching state, v3

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +728,5 @@
>          break;
> +      case "MozStorageChanged":
> +        if (event.storageArea == sessionStorage) {
> +          this.saveStateDelayed(win);
> +        }

Sorry, I was lazy with my last comment. This should be the current sessionStorage instance of the event's target: (aEvent.storageArea == aEvent.originalTarget.linkedBrowser.contentWindow.sessionStorage). Also you should wrap this in a try-catch block because accessing .sessionStorage may throw as it's unavailable for about: pages and others.

... And this is why we need tests :)

@@ +1326,5 @@
>        return;
>      }
>  
> +    // Get the latest data for this tab (generally, from the cache)
> +    let tabState = this._collectTabData(aWindow, aTab);

(No need to pass the window, see below.)

@@ +1948,3 @@
>     */
> +  _collectTabData: function ssi_collectTabData(
> +      aWindow, aTab) {

We don't need the 'aWindow' argument. We can just get it from aTab.ownerDocument.defaultView.

@@ +1975,5 @@
> +   * @returns {oject} An object with the data for this tab. This object
> +   * is recomputed at every call.
> +   */
> +  _cloneFullTabData: function ssi_cloneFullTabData(aTab) {
> +    let privacy = { includePrivateData: true };

Shouldn't this variable be named 'options'?

@@ +4899,5 @@
> + * Note that we should never cache private data, as:
> + * - that data is used very seldom by SessionStore;
> + * - caching private data in addition to public data is memory consuming.
> + */
> +let TabCache = {

Can we maybe rename this to TabStateCache? Or TabDataCache? That's a little more telling, IMO.

@@ +4914,5 @@
> +  set: function(aTab, aValue) {
> +    let key = this._normalizeToBrowser(aTab);
> +    if (gDebuggingEnabled) {
> +      if (!(aValue instanceof TabData)) {
> +        throw new TypeError("Attempting to cache a non TabData");

I wonder if we shouldn't always throw when something strange is passed to cache.set(). This can lead to quite unpredictable results.

Comment 9

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

Attachment: Caching state, v4

Ah, foolish me :)
As discussed over IRC, I have moved MozStorageChanged handling on the content process.
Also adding a little bit of a testing.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 777795 [details] [diff] [review]
Caching state, v4

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

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +21,5 @@
>      this.DOM_EVENTS.forEach(e => addEventListener(e, this, true));
>    },
>  
>    handleEvent: function (event) {
> +    debug("handleEvent: " + event.type);

We shouldn't flood the console with those messages in non-debug mode.

@@ +39,5 @@
> +            // Not a sessionStorage event
> +            break;
> +          }
> +        } catch (ex) {
> +          // This page does not even sessionStorage

Nit: I think you a word.

@@ +43,5 @@
> +          // This page does not even sessionStorage
> +          // (this is typically the case of about: pages)
> +          break;
> +        }
> +        sendAsyncMessage("SessionStore:sessionStorage");

Can we maybe put this into the try{} block? Those two break statements look a little ugly.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +61,5 @@
> +  "SessionStore:pageshow",
> +
> +  // The content script has received a MozStorageChanged event dealing
> +  // with a change in the contents of the sessionStorage.
> +  "SessionStore:sessionStorage"

Can we call this "SessionStore:mozStorageChanged"? From the current name it's really hard to tell what this message is for.

::: browser/components/sessionstore/test/browser_339445.js
@@ +28,5 @@
>  
> +      let mm = tab2.linkedBrowser.messageManager;
> +
> +      mm.addMessageListener("SessionStore:sessionStorage", function() {
> +        mm.removeMessageListener("SessionStore:sessionStorage", arguments.callee, true);

I think the test shouldn't know too much about sessionstore internals. It should rather make sure that all state has properly been written and then just change the sessionStorage data like in line 40. It then should wait for sessionstore-state-write and make sure that the sessionStorage data has been invalidated and recollected.

I think we should also move this to a new test with a good name, like browser_sessionStorage.js.

Comment 11

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

Attachment: Tests

Comment 12

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

Attachment: Caching state, v5

Comment 13

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

Applied your feedback. Also, I have moved the test to its own patch.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 778040 [details] [diff] [review]
Tests

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

Thanks!

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 778043 [details] [diff] [review]
Caching state, v5

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +61,5 @@
> +  "SessionStore:pageshow",
> +
> +  // The content script has received a MozStorageChanged event dealing
> +  // with a change in the contents of the sessionStorage.
> +  "SessionStore:sessionStorage"

That should be MozStorageChanged. Did the test work? If yes, why? :)

Comment 16

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

Attachment: Tests, v2

Removing the race condition that made the test pass despite the typo. We are now waiting for saveStateDelayed and not for saveState, which makes more sense.

Comment 17

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

Attachment: Caching state, v6

Same one, minus the typo.

Comment 18

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

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

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 778387 [details] [diff] [review]
Tests, v2

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

Hmmm. Why are we introducing a new notification? All we care about is changing state and waiting for the next sessionstore-state-write notification, no? We shouldn't care if it's a saveStateDelayed() call or whatever that caused the save. All we want to know is that the data has been invalidated correctly and is written to disk next time we write.

Comment 20

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

Well, I grew tired of attempting to trace back which calls to saveState() were due to which cause. This both fragile and non-trivial, so I added a notification that let me determine exactly what I wanted, without having to guess or adjust timeouts arbitrarily: that some event was causing the state to be saved eventually.

I have not found any other robust solution.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

I'm not really fond of adding another notification just for testing purposes. This might go away in the future and suddenly we have a few add-ons that make use of it just because they can. Apart from that I don't understand why we need to differentiate between "delayed saves" and other saves. We should only care about invalidation.

I don't want to delay this bug any further though so maybe we can have this discussion about the MozStorageChanged invalidation test in a follow-up bug? So we can get this landed and add the test a little later.

Comment 22

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

Attachment: Tests, v3

Getting rid of the additional notification.

Comment 23

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

Attachment: Caching state, v7

Fixed:
- one missing tab state invalidation;
- typo in TabStateCache that ensured the cache was pretty useless.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 778437 [details] [diff] [review]
Tests, v3

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

Thanks!

::: browser/components/sessionstore/test/browser_sessionStorage.js
@@ +43,5 @@
> +
> +  let tab;
> +  Task.spawn(function() {
> +    try {
> +      tab = gBrowser.addTab("about:home");

I think there was a bug that said we shouldn't use about:home in tests anymore. http://mochi.test:8888/ or http://example.com/ should do the same.

Comment 25

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

Attachment: Tests, v4

Same test, without about:home

Comment 26

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

Attachment: Tests, v4

This time after "hg qref".

Comment 27

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

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

Also, removing "addon-compat" since we're not changing the API or its semantics.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4e054f3c00
https://hg.mozilla.org/integration/mozilla-inbound/rev/24c800ff4936

Comment 29

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

Attachment: Caching state, v8

Ouch, my typo fix kills several tests.
Work in progress fix.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out at Yoric's request.
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f5580b9477

Comment 31

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

As it turns out, the typo that I fixed got more than one test to succeed by accident.

Here are a few reasons:
1. docShell capabilities do not cause invalidation (causing browser_capabilities.js to fail);
2. form data change seems to not always cause invalidation (or maybe it's a testing artifact - causing browser_662743.js to fail);
3. persistTabAttribute does not cause invalidation (causes browser_attributes.js to fail - that one is easy to fix);
4. hide/show - pin/unpin do not cause invalidation (cause browser_607016.js and browser_635418.js to fail - that one is easy to fix).

I need to think how to best fix 1. and 2.

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #31)
> 1. docShell capabilities do not cause invalidation (causing
> browser_capabilities.js to fail);

That one's not that easy to fix but OTOH it seems like we ignored changes in non-active windows to those capabilities anyway. I don't know if adding a notification would make sense here? Maybe even something more specific as an nsIDocShellCapabilityChangeObserver?

> 2. form data change seems to not always cause invalidation (or maybe it's a
> testing artifact - causing browser_662743.js to fail);

That sounds worth looking into a little more. We definitely should invalidate here.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

It just occurred to me that we should also be invalidating when reusing existing tabs in restoreWindow():

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#2796

We could be returning stale data if something manages to request it after the setBrowserState() call but before the tab itself has finished loading.

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #32)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #31)
> > 1. docShell capabilities do not cause invalidation (causing
> > browser_capabilities.js to fail);

After giving it a little more thought we should just update the test. docShell capability flags need to be switched *before* a page load to take effect so it doesn't really make sense for us to remember them if they have been flipped but no page has been loaded.

I think we're all good with adding a comment that explains our thoughts and we can just wait for onTabLoad() to invalidate the TabStateCache.

Comment 35

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

Attachment: Tests, v5

Comment 36

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

Attachment: Caching state, v9

Comment 37

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

Attachment: Caching state, v9

Handling a few cases that were previously not covered:
- _collectTabData will not cache the data if the tab wasn't fully loaded;
- as you suggested restoreWindow() will delete cache entries when tabs are reused.

This should cover it.

Comment 38

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

Comment on attachment 779196 [details] [diff] [review]
Tests, v5

Adding the change that you have suggested.

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 779196 [details] [diff] [review]
Tests, v5

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

::: browser/components/sessionstore/test/browser_sessionStorage.js
@@ +30,5 @@
> +
> +function waitForStorageChange(aTab) {
> +  let deferred = Promise.defer();
> +  waitForContentMessage(aTab.linkedBrowser,
> +    "sessionstore:MozStorageChanged",

Shouldn't the message be 'SessionStore:MozStorageChanged'? I'm quite sure they're case-sensitive.

Ok, so I just tried the test locally and even if I give it a totally nonsense message name it still succeeds...

@@ +31,5 @@
> +function waitForStorageChange(aTab) {
> +  let deferred = Promise.defer();
> +  waitForContentMessage(aTab.linkedBrowser,
> +    "sessionstore:MozStorageChanged",
> +    200,

I think the timeout should be way higher. Our test slaves are slow.

@@ +32,5 @@
> +  let deferred = Promise.defer();
> +  waitForContentMessage(aTab.linkedBrowser,
> +    "sessionstore:MozStorageChanged",
> +    200,
> +    ((x) => deferred.resolve(x)));

Nit: You could just pass 'deferred.resolve'.

@@ +57,5 @@
> +      ss.getBrowserState();
> +
> +      info("Change sessionStorage, ensure that state is saved");
> +      win.sessionStorage["SESSION_STORAGE_KEY"] = "SESSION_STORAGE_VALUE";
> +      yield waitForStorageChange(tab);

This really needs to check the return value.

@@ +67,5 @@
> +
> +
> +      info("Change localStorage, ensure that state is not saved");
> +      win.localStorage["LOCAL_STORAGE_KEY"] = "LOCAL_STORAGE_VALUE";
> +      yield waitForStorageChange(tab);

The return value should be false which is why the timeout shouldn't really be high here. But with a timeout too low this seems like it's not actually testing what it's supposed to on slow machines.

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 779201 [details] [diff] [review]
Caching state, v9

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +701,5 @@
>        return;
>  
> +    try {
> +      var win = aEvent.currentTarget.ownerDocument.defaultView;
> +      switch (aEvent.type) {

Let's please not put a try-catch around everything. We'll unnecessarily lose blame information and it doesn't make the code more readable.

@@ +1438,5 @@
>        this.restoreNextTab();
>      }
>  
> +    // If possible, update cached data without having to invalidate it
> +    TabStateCache.update(aTab, "hidden", false);

Good catch.

@@ +1778,5 @@
>    },
>  
>    persistTabAttribute: function ssi_persistTabAttribute(aName) {
>      if (TabAttributes.persist(aName)) {
> +      TabStateCache.clear();

Hmm. That's quite radical but OTOH it probably happens rarely (when an add-on is loaded or a new one installed).

@@ +1972,5 @@
> +    }
> +    tabData = new TabData(this._collectBaseTabData(aTab));
> +    if (this._updateTextAndScrollDataForTab(aTab, tabData)) {
> +      let browser = aTab.linkedBrowser;
> +      if (browser && browser.currentURI) {

_updateTextAndScrollDataForTab() returns false if (!browser.currentURI) so that check here seems superfluous.

@@ +4989,5 @@
> +    let key = this._normalizeToBrowser(aKey);
> +    let data = this._data.get(key);
> +    if (!data) {
> +      return undefined;
> +    }

Why does TabStateCache.update() return something?

Comment 41

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

(In reply to Tim Taubert [:ttaubert] from comment #40)
> Comment on attachment 779201 [details] [diff] [review]
> Caching state, v9
> 
> Review of attachment 779201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +701,5 @@
> >        return;
> >  
> > +    try {
> > +      var win = aEvent.currentTarget.ownerDocument.defaultView;
> > +      switch (aEvent.type) {
> 
> Let's please not put a try-catch around everything. We'll unnecessarily lose
> blame information and it doesn't make the code more readable.

Ok, filed as bug 896928. Having to add these try-catch whenever I need to find a silent bug is pretty annoying.

> @@ +1778,5 @@
> >    },
> >  
> >    persistTabAttribute: function ssi_persistTabAttribute(aName) {
> >      if (TabAttributes.persist(aName)) {
> > +      TabStateCache.clear();
> 
> Hmm. That's quite radical but OTOH it probably happens rarely (when an
> add-on is loaded or a new one installed).

That was my idea. If it turns out to be too aggressive, we might be able to find less radical strategies in a followup bug.
> 
> @@ +4989,5 @@
> > +    let key = this._normalizeToBrowser(aKey);
> > +    let data = this._data.get(key);
> > +    if (!data) {
> > +      return undefined;
> > +    }
> 
> Why does TabStateCache.update() return something?

Ah, that's left from a strategy that didn't pan out as expected. Removing.

Comment 42

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

(In reply to Tim Taubert [:ttaubert] from comment #39)
> Comment on attachment 779196 [details] [diff] [review]
> Tests, v5
> 
> Review of attachment 779196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/test/browser_sessionStorage.js
> @@ +30,5 @@
> > +
> > +function waitForStorageChange(aTab) {
> > +  let deferred = Promise.defer();
> > +  waitForContentMessage(aTab.linkedBrowser,
> > +    "sessionstore:MozStorageChanged",
> 
> Shouldn't the message be 'SessionStore:MozStorageChanged'? I'm quite sure
> they're case-sensitive.
> 
> Ok, so I just tried the test locally and even if I give it a totally
> nonsense message name it still succeeds...

Good catch. I forgot to check the return values.

> @@ +67,5 @@
> > +
> > +
> > +      info("Change localStorage, ensure that state is not saved");
> > +      win.localStorage["LOCAL_STORAGE_KEY"] = "LOCAL_STORAGE_VALUE";
> > +      yield waitForStorageChange(tab);
> 
> The return value should be false which is why the timeout shouldn't really
> be high here. But with a timeout too low this seems like it's not actually
> testing what it's supposed to on slow machines.

Well, nothing else is going to cause storage change, so a high timeout shouldn't matter.

Comment 43

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

Attachment: Tests, v6

Comment 44

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

Attachment: Caching state, v10

Applied feedback.

Comment 45

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

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

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #42)
> > The return value should be false which is why the timeout shouldn't really
> > be high here. But with a timeout too low this seems like it's not actually
> > testing what it's supposed to on slow machines.
> 
> Well, nothing else is going to cause storage change, so a high timeout
> shouldn't matter.

The timeout does exactly matter because nothing is going to cause a storage change. That means we always have to wait for the timeout to fire and that makes the test slower. But I guess we'll just have to live with that. It's not like all the other tests out there are blazingly fast anyway.

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 779724 [details] [diff] [review]
Tests, v6

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

::: browser/components/sessionstore/test/browser_sessionStorage.js
@@ +69,5 @@
> +
> +      info("Change localStorage, ensure that state is not saved");
> +      win.localStorage["LOCAL_STORAGE_KEY"] = "LOCAL_STORAGE_VALUE";
> +      storageChanged = yield waitForStorageChange(tab);
> +      ok(!storageChanged, "Changing localStorage caused the right message to be sent");

Wrong error message. Should say that no message is expected to be sent.

Comment 48

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

(In reply to Tim Taubert [:ttaubert] from comment #46)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #42)
> > > The return value should be false which is why the timeout shouldn't really
> > > be high here. But with a timeout too low this seems like it's not actually
> > > testing what it's supposed to on slow machines.
> > 
> > Well, nothing else is going to cause storage change, so a high timeout
> > shouldn't matter.
> 
> The timeout does exactly matter because nothing is going to cause a storage
> change. That means we always have to wait for the timeout to fire and that
> makes the test slower. But I guess we'll just have to live with that. It's
> not like all the other tests out there are blazingly fast anyway.

Ah, understood. I thought you worried about false positives.

Comment 49

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

Attachment: Tests, v7

Same test, tweaked the message.

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/101c149b3287
https://hg.mozilla.org/integration/fx-team/rev/4eb7a0f560da

Comment 51

[Ed Morley [:emorley]]

Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25660147&tree=Fx-Team
{
04:08:52  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_drag_drop.js | Test timed out
04:47:21  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_bug_831693_searchbox_panel_navigation.js | Test timed out
04:47:21  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_bug_831693_searchbox_panel_navigation.js | Found a tab after previous test timed out: http://mochi.test:8888/browser/browser/devtools/inspector/test/browser_inspector_bug_831693_search_suggestions.html
04:47:22  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_bug_835722_infobar_reappears.js | uncaught exception - ReferenceError: info is not defined at chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_bug_831693_searchbox_panel_navigation.js:141
}

remote:   https://hg.mozilla.org/integration/fx-team/rev/3352acd5e550
remote:   https://hg.mozilla.org/integration/fx-team/rev/8a3b8fe77642

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

That log above has nothing to do with this bug but with a push before. The backout here is still justified for OSX failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_drag_drop.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625257.js | uncaught exception - NS_ERROR_ILLEGAL_VALUE: 'Illegal value' when calling method: [nsISessionStore::undoCloseTab] at chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625257.js:69
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_625257.js | Test timed out

Comment 53

[Tim Taubert [:ttaubert] (inactive)]

(Bah, please disregard that browser_newtab_drag_drop.js failure in my previous comment.)

Comment 54

[Ed Morley [:emorley]]

Bah sorry must have mis-read the TBPL page - the push before this one had a few green browser-chrome runs, obviously didn't scroll down far enough to see the other orange ones.

This can reland as-is :-)

Comment 55

[Ed Morley [:emorley]]

(In reply to Tim Taubert [:ttaubert] from comment #53)
> (Bah, please disregard that browser_newtab_drag_drop.js failure in my
> previous comment.)

Oh you meant just the one line, not disregard all of comment 52.
In which case, yeah those failures need fixing first.

Sorry for the noise! (I can tell it's going to be one of those days!)

Comment 56

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

Attachment: Caching state, v11

Ah, well, I had a green Try, it just didn't include MacOS X.
Let's be more aggressive, then:

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

As discussed over IRC, I'm adding a web progress listener on the content script. It is my understanding that it will be unregistered when we kill the tab.

Comment 57

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #56)
> It is my understanding that it will be unregistered when we kill the tab.

I needed to double check that. Indeed, they're added as weak refs:
http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#1222

Comment 58

[Dão Gottwald [:dao]]

Comment on attachment 780446 [details] [diff] [review]
Caching state, v11

>       case "change":
>         sendAsyncMessage("SessionStore:input");
>         break;
>+      case "MozStorageChanged":
>+        {
>+          let isSessionStorage = true;
>+          // We are only interested in sessionStorage events
>+          try {
>+            if (event.storageArea != content.sessionStorage) {
>+              isSessionStorage = false;
>+            }
>+          } catch (ex) {
>+            // This page does not even have sessionStorage
>+            // (this is typically the case of about: pages)
>+            isSessionStorage = false;
>+          }
>+          if (isSessionStorage) {
>+            sendAsyncMessage("SessionStore:MozStorageChanged");
>+          }
>+          break;
>+        }

style nit... I think it's preferable not to indent this whole block further. You could just add the brackets like this:

      case "MozStorageChanged": {
        ...
        break;
      }

or:

      case "MozStorageChanged": {
        ...
        break; }

Comment 59

[Dão Gottwald [:dao]]

Comment on attachment 780446 [details] [diff] [review]
Caching state, v11

>+    } catch (ex) {
>+      debug("Uncaught error during observe");
>+      debug(ex);
>+      debug(ex.stack);
>     }

Is this a workaround for bug 895340?

Comment 60

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #59)
> >+    } catch (ex) {
> >+      debug("Uncaught error during observe");
> >+      debug(ex);
> >+      debug(ex.stack);
> >     }
> 
> Is this a workaround for bug 895340?

Yeah. Now that this has landed even before this bug, can we remove this, David?

Comment 61

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 780446 [details] [diff] [review]
Caching state, v11

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

Nit: I agree with Dão about not diverging too much from our usual style of indenting switch statements.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +60,5 @@
>  
> +let ProgressListener = {
> +  init: function() {
> +    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIWebProgress);

Nit: please indent this a little more so that the dots are on a vertical line.

@@ +63,5 @@
> +    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIWebProgress);
> +    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATE_REQUEST);
> +  },
> +  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {

I think we should be using onLocationChange() here:
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIWebProgressListener#onLocationChange%28%29

That way we only invalidate if we're *really* going to load that page.

@@ +71,5 @@
> +      sendAsyncMessage("SessionStore:loadStart");
> +    }
> +  },
> +  QueryInterface: XPCOMUtils.generateQI(["nsIWebProgressListener",
> +                                         "nsISupportsWeakReference"])

Can we pass strings here? I thought this needs to be .generateQI([Ci.nsIWebProgressListener, Ci.nsISupportsWeakReference])?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +64,5 @@
> +  // with a change in the contents of the sessionStorage.
> +  "SessionStore:MozStorageChanged",
> +
> +  // The content script tells us that a new page is being loaded in a
> +  // browser.

Nit: "just started loading in a browser".

Comment 62

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

Attachment: Caching state, v12

Applied feedback.

Comment 63

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

Attachment: Tests, v8

Updated browser_625257.js to wait for SessionStore:loadStart instead of using its own nsIWebProgressListener. I took the opportunity to make it a Task instead of a nest of spaghetti.

Comment 64

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

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

Comment 65

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 781069 [details] [diff] [review]
Tests, v8

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

Thanks for cleaning this up! Just to make sure... does this test break if we don't invalidate? Maybe also on other platforms than OSX now?

::: browser/components/sessionstore/test/browser_625257.js
@@ +40,5 @@
> +
> +function waitForTabClosed() {
> +  let deferred = Promise.defer();
> +  let observer = function() {
> +    gBrowser.tabContainer.removeEventListener("TabClose", observer);

The listener isn't going to be removed without the capturing=true argument here.

@@ +41,5 @@
> +function waitForTabClosed() {
> +  let deferred = Promise.defer();
> +  let observer = function() {
> +    gBrowser.tabContainer.removeEventListener("TabClose", observer);
> +    executeSoon(deferred.resolve);

We don't actually need executeSoon() here because we're using the new Promise.jsm, right?

@@ +53,5 @@
>  
> +  Task.spawn(function() {
> +    try {
> +      // Open a new tab
> +      tab = gBrowser.addTab("about:blank");

let tab = ...

@@ +65,5 @@
>  
> +      // Start a load and interrupt it by closing the tab
> +      tab.linkedBrowser.loadURI(URI_TO_LOAD);
> +      let loaded = yield waitForLoadStarted(tab);
> +      is(loaded, true, "Load started");

nit: ok(loaded, "Load started");

Comment 66

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 781068 [details] [diff] [review]
Caching state, v12

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

Thank you, this looks great.

Comment 67

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

(In reply to Tim Taubert [:ttaubert] from comment #65)
> Comment on attachment 781069 [details] [diff] [review]
> Tests, v8
> 
> Review of attachment 781069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for cleaning this up! Just to make sure... does this test break if we
> don't invalidate? Maybe also on other platforms than OSX now?

I just checked. On MacOS X, it looks like just waiting for SessionStore:loadStart is sufficient to make the test pass, even if I don't invalidate... The reason is not clear to me.

>
> ::: browser/components/sessionstore/test/browser_625257.js
> @@ +40,5 @@
> > +
> > +function waitForTabClosed() {
> > +  let deferred = Promise.defer();
> > +  let observer = function() {
> > +    gBrowser.tabContainer.removeEventListener("TabClose", observer);
> 
> The listener isn't going to be removed without the capturing=true argument
> here.
> 
> @@ +41,5 @@
> > +function waitForTabClosed() {
> > +  let deferred = Promise.defer();
> > +  let observer = function() {
> > +    gBrowser.tabContainer.removeEventListener("TabClose", observer);
> > +    executeSoon(deferred.resolve);
> 
> We don't actually need executeSoon() here because we're using the new
> Promise.jsm, right?

True, but given the number of times we have changed implementation of promises, I didn't want to rely upon this.

Comment 68

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #67)
> True, but given the number of times we have changed implementation of
> promises, I didn't want to rely upon this.

That seems silly - I think we've settled on a behavior now, so we shouldn't be adding code to deal with potential changes "just in case". If we do change, adding an executeSoon to this test would not be complicated.

Comment 69

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

Attachment: Tests, v9

Applied feedback.

Comment 70

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

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

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6f8cc9245141
https://hg.mozilla.org/integration/fx-team/rev/6a62ce6a092b

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f8cc9245141
https://hg.mozilla.org/mozilla-central/rev/6a62ce6a092b