User-Agent: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre If you restore a closed normal tab that in its life was once a App Tab, when it is restored it will turn into a App Tab even if it is a normal tab when it is closed. Reproducible: Always Steps to Reproduce: 1. Open A Few Tab 2. In one of the tab go any web site you want and turn it into a App Tab 3. The App Tab is the most left tab now. Turn it back to normal Tab 4. Close the normal Tab 5. Right Clink the Tab Bar and select Undo Close Tab with your mouse 6. You see that The Closed Normal Tab reopen as a App tab 7. And the first normal Tab cover half of the App Tab. (another error) Actual Results: Closed Normal Tab when restored became a App Tab. Expected Results: Closed Normal Tab when restored remains as Normal Tab

Sometime reproduce this is not possible. Try producing a few times or try the next tab.

I just reproduced. Haven't hunted down why yet, but we'll want to block on this.

Blockinnnngg! (sung, in falsetto.)

The previous code: "if (aTab.pinned) tabData.pinned = true;" Relied on tabData.pinned being undefined when !aTab.pinned, but when the code: "tabData = browser.__SS_data;" executes it no longer is.

Comment on attachment 463045 [details] [diff] [review] Patch Looks reasonable, but could we do "else\n delete tabData.pinned;" instead so that ', "pinned": false' isn't written on disk?

I also added delete tabData.userTypedValue; and delete tabData.userTypedClear; because it looks like the same bug could affect them.

1. Please write a test (it seems that this is a timing issue, so it might be difficult) 2. Unless you know that userTypedValue has the same problem, please don't make that change. A test will help your case though.

While writing a test I noticed that the error also occurs when "if (browser.__SS_data && browser.__SS_data._tabStillLoading)" evaluates to true. I have updated the patch to reflect this. I have also bundled with a test, however it always executes _collectTabData when the _tabStillLoading is true. So it unfortunately doesn't demonstrate that my old patch works. It does however demonstrate that both userTypedValue and pinned can be reset to an old value when __SS_data overwrites tabData. I have kept the "delete userTypedValue" code in the patch so it is possible to see how the old code resets userTypedValue to a previous value while the new code does not. However, I have no proof that this ever happens outside of my test; if that is a requirement I will gladly remove all "delete userTypedValue" code. I have tried to make a test that runs in such a way that _tabStillLoading is false but the bug still takes place, but I haven't had any success. Although I have used dump to manually confirm that the bug often is caused by that line of code. Right before "tabData = browser.__SS_data;": tabData.pinned = undefined and right after tabData.pinned = true.

Litmus test cases added: https://litmus.mozilla.org/show_test.cgi?id=12609 https://litmus.mozilla.org/show_test.cgi?id=12610

Comment on attachment 463391 [details] [diff] [review] Patch v3 + Test Has it really been 2 months? I'm really sorry. I think it looks good. It would be good to know how consistently the test is actually testing the right condition. So once there's a new patch I'll push it to try server and get a few runs. >diff --git a/browser/base/content/test/Makefile.in The test (and of course Makefile change) should go in browser/components/sessionstore/test/browser/. >diff --git a/browser/base/content/test/browser_bug579868.js >+function test() { >+ let tab1 = gBrowser.addTab("about:robots"); >+ let tab2 = gBrowser.addTab("about:credits"); >+ tab1.addEventListener("load", mainPart, true); >+ waitForExplicitFinish(); >+ >+ function mainPart() { >+ // Tell the session storer that the tab is pinned and that userTypedValue is "Hello World!" >+ let newTabState = '{"entries":[{"url":"about:robots","title":"Title","ID":0,"formdata":{},"scroll":"0,0"}],"index":1,"pinned":true,"userTypedValue":"Hello World!","attributes":{"image":"http://www.mozilla.org/favicon.ico"}}'; You can get rid of a bunch of these fields to make this more readable: title, ID, formdata, scroll, index, attributes. >+ let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore); >+ ss.setTabState(tab1, newTabState); >+ >+ // Undo pinning and userTypedValue >+ gBrowser.unpinTab(tab1); >+ tab1.linkedBrowser.userTypedValue = null; So in this case you would also want to confirm that browser.__SS_data._tabStillLoading is true, otherwise you're not actually testing the new code. Right? >+ // Close and restore tab >+ gBrowser.removeTab(tab1); I would actually add a check right here that looks at the state for the first closed tab (via ss.getClosedTabData()). That should actually be a good enough check since the problem is actually with saving data. the rest of the test is definitely good to have though. >+ tab1 = ss.undoCloseTab(window, 0); >+ >+ isnot(tab1.pinned, true, "Should not be pinned"); >+ let savedState = JSON.parse(ss.getTabState(tab1)); >+ isnot(savedState.userTypedValue, "Hello World!", "userTypedValue should not be Hello World!"); >+ gBrowser.removeTab(tab1); >+ gBrowser.removeTab(tab2); >+ finish(); >+ } >+} Clearing review, but feedback+

Comment on attachment 481959 [details] [diff] [review] Patch v3 + Test v2 >diff --git a/browser/components/sessionstore/test/browser/browser_579868.js >+ // Undo pinning and userTypedValue >+ gBrowser.unpinTab(tab1); >+ tab1.linkedBrowser.userTypedValue = null; >+ >+ is(tab1.linkedBrowser.__SS_data._tabStillLoading, true, >+ "_tabStillLoading should be true."); Is this consistently passing for you? >+ >+ // Close and restore tab >+ gBrowser.removeTab(tab1); >+ let savedState = JSON.parse(ss.getClosedTabData(window)); I'm willing to bet that these isnots are passing, but you're also not checking the right thing - you're comparing undefined to true and "Hellow World!". getClosedTabData will return a JSON.stringifed array of tabs. So you would actually need to get the right tab out of the array (should be at index 0, per your use of undoCloseTab).

(In reply to comment #12) > Comment on attachment 481959 [details] [diff] [review] > Patch v3 + Test v2 > > >diff --git a/browser/components/sessionstore/test/browser/browser_579868.js > >+ // Undo pinning and userTypedValue > >+ gBrowser.unpinTab(tab1); > >+ tab1.linkedBrowser.userTypedValue = null; > >+ > >+ is(tab1.linkedBrowser.__SS_data._tabStillLoading, true, > >+ "_tabStillLoading should be true."); > > Is this consistently passing for you? Yes. > >+ > >+ // Close and restore tab > >+ gBrowser.removeTab(tab1); > >+ let savedState = JSON.parse(ss.getClosedTabData(window)); > > I'm willing to bet that these isnots are passing, but you're also not checking > the right thing - you're comparing undefined to true and "Hellow World!". > getClosedTabData will return a JSON.stringifed array of tabs. So you would > actually need to get the right tab out of the array (should be at index 0, per > your use of undoCloseTab). Apologies. This time I verified that the new version passes with the patch and fails without it.

(In reply to comment #13) > Apologies. This time I verified that the new version passes with the patch and > fails without it. Always a good thing to do (even though I forget myself) Pushed to try server. Test results (in a sane format) will be at http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry&rev=4fc26903ec1e I'll give this a final look on Sunday/Monday.

Pushed http://hg.mozilla.org/mozilla-central/rev/da395a53dc39

I raised 604610 which has been marked as a duplicate of this, but I'm not sure it is. If I'm reading it correctly this bug only applies when the change to the tab (making it app tab or normal tab) occurs while the tab is still loading, is that right? My problem would happen with tabs that have been loaded for ages.

(In reply to comment #17) > I raised 604610 which has been marked as a duplicate of this, but I'm not sure > it is. If I'm reading it correctly this bug only applies when the change to > the tab (making it app tab or normal tab) occurs while the tab is still > loading, is that right? My problem would happen with tabs that have been > loaded for ages. It's a dupe. Thanks for filing it. Please let us know if it has been resolved in a recent build, thanks.