STR: 1. close a tab, with some other tabs remaining open in a single tab group 2. shift+ctr+t Expected: Closed tab is reopened in the same tab group Actual: A brand new tab group is created and switched to.

This patch fixes bugs 624265, 623792 and 624102 and includes test cases for all three of them. These bugs share the same source of error. Pushed to try today. Passed.

Do we know which patch has regressed this bug?

This one caused the regression: http://hg.mozilla.org/mozilla-central/rev/f453924d5fe1

Thanks! Adding to the dependency list.

bugspam (moving b9 to b10)

bugspam (removing b9)

Regression, and very startling experience -> blocking+.

It makes me sad that this bug is going to be in beta9.

I've added the relnote keyword, but is there any way we can take this bug in beta 9? When I encountered it, not knowing a bug had been filed, I had a very unsettling feeling that I'd lost my tabs.

I don't know if that is possible. The 3 bugs covered by this patch give some really bad user experience and no user will associate this with panorama as this seems like buggy firefox behavior :/

Comment on attachment 502409 [details] [diff] [review] patch v1 (fixes 624265, 623792, 624102) In addition to the things we discussed via IRC, I have these comments on the test: >+ let restoreTab = function (callback) { >+ let tab = undoCloseTab(0); >+ >+ tab.tabItem.addSubscriber(tab, 'reconnected', function () { >+ tab.tabItem.removeSubscriber(tab, 'reconnected'); >+ setTimeout(function () afterAllTabsLoaded(callback), 0); >+ }); >+ } Why the setTimeout? We try to avoid them if possible. >+ let next = function () { >+ while (gBrowser.tabs.length-1) >+ gBrowser.removeTab(gBrowser.tabs[1]); >+ >+ gBrowser.loadOneTab('about:blank', {inBackground: true}); >+ gBrowser.removeTab(gBrowser.tabs[0]); Why do you kill the original tab each time? I suppose it's ok... just seems odd. >+function enterAndLeavePrivateBrowsing(callback) { >+ function pbObserver(aSubject, aTopic, aData) { >+ if (aTopic != "private-browsing-transition-complete") >+ return; >+ >+ if (pb.privateBrowsingEnabled) >+ pb.privateBrowsingEnabled = false; >+ else { >+ Services.obs.removeObserver(pbObserver, "private-browsing-transition-complete"); >+ setTimeout(function () afterAllTabsLoaded(callback), 0); Another unneccessary setTimeout? >+function afterAllTabsLoaded(callback) { This routine is getting used in a number of places now...seems like we should add it to head.js. >+ let stillToLoad = 0; >+ >+ function runCallback() { >+ setTimeout(function () callback(), 0); >+ } Do we need this setTimeout? >+ if (browser.webProgress && browser.webProgress.isLoadingDocument) { As per bug 623330, I believe this line should be: if (browser.contentDocument.readyState != "complete") { >+ if (!stillToLoad) >+ runCallback(); ... and you'll note in bug 623330 that Dao has called me out on missing those last two lines! :) If you add this routine to head.js with this patch, I can use it in the 623330 patch.

(In reply to comment #14) > >+ setTimeout(function () afterAllTabsLoaded(callback), 0); > > Why the setTimeout? We try to avoid them if possible. (FWIW, if you actually need the asynchronicity there, "SimpleTest.executeSoon(foo)" is effectively the same as "setTimeout(foo,0)" and I think is preferred for these sorts of things)

(In reply to comment #15) > (FWIW, if you actually need the asynchronicity there, > "SimpleTest.executeSoon(foo)" is effectively the same as "setTimeout(foo,0)" > and I think is preferred for these sorts of things) Thanks for the hint - I'll use that in the one place that I figured out definitely needs it.

(In reply to comment #12) > I've added the relnote keyword, but is there any way we can take this bug in > beta 9? When I encountered it, not knowing a bug had been filed, I had a very > unsettling feeling that I'd lost my tabs. I think we'll relnote it and look to taking an approved patch as a ridealong if we are respinning for a bigger issue. Justification: There is no (real) data loss here, only (bad) user confusion when using panorama + undoing closed tabs. It is my understanding going into panorama shows the hidden tab group and enables the user to get back to the state they expect. It is a valid concern that users may not associate the behavior with panorama and thus won't get their tabs back, but they have to be a user of panorama to get into this state in the first place...I don't think it is unreasonable to expect them to stumble across the fix and understand what is going on down the road in normal panorama usage. Also, I would imagine closing the reopened tab (and thus having no tabs in the tab group) would put the user back in an expected state as well (unconfirmed).

Pushed to try a minute ago.

(In reply to comment #17) > but they have to be a user of > panorama to get into this state in the first place... Just for clarity -- I'm not sure if this point was stated in any comments above (it's not mentioned in the STR), but it is true from my testing. On my machine, with a fresh profile, this bug isn't triggered until *after* I've gone into & out of Panorama-mode at least once.

(In reply to comment #17) > Justification: [...] > expect. It is a valid concern that users may not associate the behavior with > panorama and thus won't get their tabs back, but they have to be a user of > panorama to get into this state in the first place...I don't think it is > unreasonable to expect them to stumble across the fix and understand what is > going on down the road in normal panorama usage. Also, I would imagine closing Other than intermittently triggering panorama by hitting its hotkey by mistake -- Cmd-E is remarkably close to both Cmd-W and Cmd-R -- I've never used panorama. (It's fine that other people like it, but it's not personally my cup of tea.) All the same, this bug has affected me and it was only through happenstance that I was able to come across this bug. There may not be "real data loss" here, but if I hadn't found this bug, I'm not sure I'd come to any other conclusion.

Passed try.

Comment on attachment 502954 [details] [diff] [review] patch v2c (minor test cleanup) Beautiful.

There really should be a second beta 9 build to include this fix, it breaks several important functions (undo close tab, private browsing, panorama).

Is there a workout for this issue for Beta9? maybe a way to disable panorama all together?

(In reply to comment #30) > Is there a workout for this issue for Beta9? maybe a way to disable panorama > all together? There's not really a workaround for beta 9, but if you're so inclined, the nightly builds include this fix: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

(and per comment 19, you'll be unaffected by this bug if, in a given browsing session, you never (intentionally or unintentionally) invoke panorama)

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre ID:20110114030359 Tim, do the automated tests cover all possible paths you have had in mind, or would a manual test be necessary?

(In reply to comment #33) > Tim, do the automated tests cover all possible paths you have had in mind, or > would a manual test be necessary? The tests cover all possible paths I could think of. The one thing that is not yet fixed is bug 624847.

Given that this is covered by its own bug, thanks for your feedback.