611330 - setTabValue() couldn't save data for to-be-restored tabs

Status: RESOLVED DUPLICATE of bug 607016

(Firefox :: Session Restore, defect)

Description

[YUKI "Piro" Hiroshi]

When setTabValue() is called, it stores the data to the property "aTab.__SS_extdata" of the tab. If the tab is not restored completely yet, (it will appear many times if you set browser.sessionstore.max_concurrent_tabs to 0) the data is stored to "aTab.linkedBrowser.__SS_data.extData" instead of "aTab.__SS_extdata".

http://hg.mozilla.org/mozilla-central/file/7cf6faecbed3/browser/components/sessionstore/src/nsSessionStore.js#l1317

However, SessionStoreService.prototype._collectTabData() (which is called from SessionStoreService.prototype.saveState()) clears aTab.linkedBrowser.__SS_data.extData if the tab has no aTab.__SS_extdata.

http://hg.mozilla.org/mozilla-central/file/7cf6faecbed3/browser/components/sessionstore/src/nsSessionStore.js#l1448
  1539     if (aTab.__SS_extdata)
  1540       tabData.extData = aTab.__SS_extdata;
  1541     else if (tabData.extData)
  1542       delete tabData.extData;

As a result, if SessionStoreService.prototype.saveState() is called before the to-be-restored tab is completely restored, data saved by setTabValue() will be lost.

This seems to be a regression caused by the bug 590268.

Comment 1

[YUKI "Piro" Hiroshi]

Steps to reproduce:

1. Open new browser window with a tab "about:config".
2. Set "browser.startup.page" to "3".
3. Set "browser.sessionstore.max_concurrent_tabs" to "0".
4. Set "devtools.errorconsole.enabled" to "true".
5. Open another tab and load "about:".
6. Select the first tab ("about:config").
7. Quit Minefield.
8. Restart Minefield. Then there are one restored tab "about:blank"
   and another to-be-restored tab with the title "About:".
9. Open Error Console by Ctrl-Shift-J.
10. Copy following script into the "Evaluate" box and run it.
-------------------------------------------------------------------
var ss = Components.classes['@mozilla.org/browser/sessionstore;1']
          .getService(Components.interfaces.nsISessionStore);
var wm = Components.classes['@mozilla.org/appshell/window-mediator;1']
          .getService(Components.interfaces.nsIWindowMediator);
var win = wm.getMostRecentWindow('navigator:browser');

win.gBrowser.tabContainer.addEventListener('TabClose', function(aEvent) {
  var tab = aEvent.originalTarget;
  var data = Date.now();
  ss.setTabValue(tab, 'test-data', data);
  alert('stored data: '+data);
}, false);

win.gBrowser.tabContainer.addEventListener('SSTabRestored', function(aEvent) {
  var tab = aEvent.originalTarget;
  var data = ss.getTabValue(tab, 'test-data');
  alert('restored data: '+data);
}, false);
-------------------------------------------------------------------
11. Close the "About:" tab by closebox of the tab. Then an alert is shown
    with the current time like "stored data: 1289492130203".
12. Restore the tab by Ctrl-Shift-T.

Actual Result:
An alert is shown with the message "stored data: ".

Expected Result:
An alert is shown with the message "stored data: 1289492130203".

Comment 2

[Ian Gilman [:iangilman]]

Adding Paul, as he wrote the original patch for bug 590268.

Comment 3

[Ian Gilman [:iangilman]]

I believe this is what's blocking me on bug 598795 and bug 594644 (both of which are blockers); when I apply those patches, I get this series of errors in the test for bug 590268:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 11: extData was correct - Got , expected 12898821010350.44926920051066277
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 10: extData was correct - Got , expected 12898821010350.17031393094726255
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 9: extData was correct - Got , expected 12898821010350.6403366812206732
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 8: extData was correct - Got , expected 12898821010350.4612792467367748
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 7: extData was correct - Got , expected 12898821010350.48433409830006124
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 6: extData was correct - Got , expected 12898821010350.1842668584955346
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 5: extData was correct - Got , expected 12898821010350.5407382056739869
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 4: extData was correct - Got , expected 12898821010350.43950214409426613
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 3: extData was correct - Got , expected 12898821010350.3894383361690794
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 2: extData was correct - Got , expected 12898821010350.7379392756827334
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | tab 1: extData was correct - Got , expected 12898821010350.47807025612296294
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_590268.js | checked the same number of uniq2 as we set - Got 0, expected 11

I believe this is because those patches (specifically bug 594644, but it depends on bug 598795) introduce new behavior in response to session restore, which is probably tickling this bug.  

Paul, any thoughts on how to fix this bug?  

If you want to run my patches, I'll have to get you updated versions.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I have an idea, will look at it soon. My guess is that if the tab didn't have extData before then we'll set it on the browser but when collecting tab data we take a shortcut and just use __SS_data since nothing should have been done to the tab, so we don't collect the new stuff on the browser.

This has come up a couple times now with different attributes, so this might just end up being duped to bug 607016.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I'm just going to fix this over in bug 607016.