620290 - Switch to Tab does not unregister open pages when entering Private Browsing mode

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Christian Ascheberg]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0b9pre) Gecko/20101219 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0b9pre) Gecko/20101219 Firefox/4.0b9pre

After switching to private browsing mode, Firefox offers to switch to tabs that were only open in non-PB mode.
Furthermore when back in non-PB mode, Firefox will still offer switch-to-tab results for tabs that you closed then.

Reproducible: Always

Steps to Reproduce:
1. open some websites
2. switch to PB mode
3. type part of URL / title of tabs that where open in non-PB into location bar
---
4. switch back to non-PB mode
5. close a tab, again type part of URL of that tab into location bar
Actual Results:  
at 3.: switch-to-tab result for tab that was only open in non-PB mode will be displayed
at 5.: switch-to-tab result will be displayed for closed tab
Both results are not functional of course

Expected Results:  
None of those switch-to-tab results should be displayed.

Comment 1

[Christian Ascheberg]

Oh, forgot to say that it is a recent regression:

works:
win32 2010-12-15-03-mozilla-central f11f7ed625ba
broken:
win32 2010-12-16-03-mozilla-central a5413c3c1013

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f11f7ed625ba&tochange=a5413c3c1013

Comment 2

[Marco Bonardo [:mak]]

Does this happen if you type immediately after switching or also after some seconds? Since unregistering is async, it's even possible you search before all unregistering has been done. I'll try to reproduce.

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Does this happen if you type immediately after switching or also after some
> seconds? Since unregistering is async, it's even possible you search before all
> unregistering has been done. I'll try to reproduce.
That can't be the case since the query is also async, and all requests happen in the order in which they were called.

Comment 4

[Shawn Wilsher :sdwilsh]

Not going to block on this until we can get someone else to confirm it.

Comment 5

[Alice0775 White]

I can reproduce on 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101221 Firefox/4.0b9pre ID:20101221030401

(In reply to comment #2)
> Does this happen if you type immediately after switching or also after some
> seconds? 
I am not related in a timing.

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/e435c9812855
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101215 Firefox/4.0b9pre ID:20101215174043
Fails;
http://hg.mozilla.org/mozilla-central/rev/cd62a8afc52e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101215 Firefox/4.0b9pre ID:20101215204205
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e435c9812855&tochange=cd62a8afc52e

In localbuild, the following cset causes the problem,
f118db7595c6	Shawn Wilsher — Bug 582703 - Improve the concurrency of location bar searches. Initial patch from sdwilsh is r=mak, further changes from mak are r=sdwilsh. switch-to-tab changes are r=unfocused sr=rstrong a=blocking

Comment 7

[Marco Bonardo [:mak]]

taking for investigation

Comment 8

[(no longer active)]

We should be reinitializing the list of open tabs as part of nsISessionStore::SetBrowserState.

Comment 9

[Marco Bonardo [:mak]]

I think session store is not related. the only difference is that in previous code we were setting mInPrivateBrowsing through the enter/leave observer, here we always check pb.privateBrowsingEnabled.... This could mean we fail to unregister pages because previously we were notifying sessionstore before history, so pages were already unregistered, while now we don't try to unregister.

If so this can easily be fixed by implementing bug 558321. I'll check if this is the case.

Comment 10

[Marco Bonardo [:mak]]

Confirmed, I'm marking this as dependent on bug 558321 and re-assigning to Blair, that will be able to fix 2 blockers with 1 patch. Be happy.

Comment 11

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

(In reply to comment #8)
> We should be reinitializing the list of open tabs as part of
> nsISessionStore::SetBrowserState.

We do, though indirectly. onLocationChange will fire for tabs as they are loading. For the to-be-restored tabs (which won't trigger onLocationChange immediately) we have bug 599909 to force that event & make sure the tabs will match.

Marco should be right here. I think it's just the check for enabled that was causing issues. I think with my patch in bug 599909 we might even be testing that (relevant comment there is #15).

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Fixed with bug 558321, which landed here: https://hg.mozilla.org/mozilla-central/rev/6bbe54cb876e

Comment 14

[Christian Ascheberg]

bug 558321 was reopened, too

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Re-fixed with bug 558321 re-landing:
http://hg.mozilla.org/mozilla-central/rev/2f9ad4aa0795

Comment 17

[u279076]

Verified with Firefox 4.0b10build1.