Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100409 Minefield/3.7a5pre Currently, tab matches are not working in Private Browsing mode. STR: 1. Enter PB mode 2. Open a new tab, www.yahoo.ca 3. Open a new tab, start typing yahoo.ca ER: Should see a match for yahoo.ca AR: Do not see any results

We'll have to add some testing to Litmus around this...

Ehsan / Limi: This is currently by design, but would be great to get your input as to whether its correct.

I don't think I understand why it doesn't work in private browsing mode. I don't think there is any privacy risk with having tab matches in awesomebar in private browsing mode, and I think this is a bug which needs to be fixed. Assigning to Blair for now.

This needs the fix from bug 546253 to work as expected.

This seems pretty broken, since it's not intuitive to the user that it's by design - all their tabs are visible, but the thingy won't show them now. Actually, I don't understand either. Why was it designed this way?

the code in question is http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#5130 and yes, from the code that looks a request by design (dunno from who?). I think just removing the above code will do (and adding a test). Maybe there was some fear to have pages not correctly unregistered when exiting PB mode, so that we would sneak them into normale mode, we can add a protection level clearing moz_openpages_temp contents when we exit pb mode (history has already code listening for that).

Yeah, this doesn't feel intentional. If you have the tabs open, we should match on them, even in private browsing mode — unless this is impossible because of architectural issues or something.

It was intentional, but the reasoning wasn't very strong, IMO. I think one argument was that you might be in private browsing mode but only have "private" sites in one of your windows that's minimized, and wouldn't want to expose sites from that other window while you're interacting with the current one, and the other argument was paranoia about making sure we don't persist the data on disk somehow (did one of the earlier patches not use temp tables?). Which is all to say that I don't think there are any reasons not to fix this.

(In reply to comment #8) > disk somehow (did one of the earlier patches not use temp tables?). yes, initial patches were adding to Places history.

So this causes issues when using browser.privatebrowsing.keep_current_session=true (as happens in tests). One test in sessionstore (browser_248970_a.js) is entering PB mode, then closing a tab that was already open. So the url is never unregistered and so a further test fails.

(In reply to comment #10) > So this causes issues when using > browser.privatebrowsing.keep_current_session=true (as happens in tests). One > test in sessionstore (browser_248970_a.js) is entering PB mode, then closing a > tab that was already open. So the url is never unregistered and so a further > test fails. Unfortunately, we need to keep that pref working, I'm afraid. :(

(In reply to comment #11) > (In reply to comment #10) > > So this causes issues when using > > browser.privatebrowsing.keep_current_session=true (as happens in tests). One > > test in sessionstore (browser_248970_a.js) is entering PB mode, then closing a > > tab that was already open. So the url is never unregistered and so a further > > test fails. > > Unfortunately, we need to keep that pref working, I'm afraid. :( Agreed. That was my point, though my comment was poorly worded and I didn't actually say that! By blindly ignoring urls un/registered while in PB mode we break that pref.

Bug 480350 comment 114 seems to be the reason...although I don't understand it fully.

The reason for this bug is just comment 8 (leak of information across windows). There should be no other problem in supporting tab matches in PB mode today, apart bugs that could leak entries on entering/exiting PB mode, but those would be bugs to fix, not something we should use as a reason to decide on this bug. In the past there was another problem, since pages were registered in history, and that could had been a serious privacy leaking source.

(In reply to comment #12) > By blindly ignoring urls un/registered while in PB mode we break that pref. Yep, so fixing this bug (which is just removing 2 checks for PB, I think) will also fix that issue.

Pure code removal. There's a comment in mozIPlacesAutoComplete about registerOpenPage/unregisterOpenPage and private browsing mode which I have not removed, as it's still relevant (consumers still need to unregister pages if they're being closed/opened when entering/leaving private browsing mode). Note the lack of a test - I don't know of any way to test private browsing mode, so if someone could either point me in the right direction, or write the test, it would be appreciated. Failing that, it'd be a pretty simple litmus test.

Updated to remove the PB-service lazy getter and associated function that's no longer used.

well, you wrote http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_tabMatchesInAwesomebar.js and it seem to test tab matches in pb mode :) Did you run it?

(In reply to comment #18) > well, you wrote > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_tabMatchesInAwesomebar.js > and it seem to test tab matches in pb mode :) Did you run it? *sigh* It's been one of those days... will look at that tomorrow.

I've assigned bug 620290 to you since it is practicall fixed by implementing this bug :)

ps: the patch is fine, I'd like to have any sort of test, but ideally I think the above test is already covering this, so it's just matter of checking.

Comment on attachment 501587 [details] [diff] [review] Patch v1.1 review conditioned to verifying test goodness.

That test ran ok, but wasn't testing as much as it could/should have been. So I've added to it, testing with browser.privatebrowsing.keep_current_session disabled.

Comment on attachment 502398 [details] [diff] [review] Patch v2 >diff --git a/browser/base/content/test/browser_tabMatchesInAwesomebar.js b/browser/base/content/test/browser_tabMatchesInAwesomebar.js >+ function() { >+ info("Running step 12 - leave private browsing mode"); ... >+ for (let i = 1; i < gBrowser.tabs.length; i++) >+ waitForRestoredTab(gBrowser.tabs[i]); >+ nit: some trailing spaces here. > function loadTab(tab, url) { > // Because adding visits is async, we will not be notified immediately. > let visited = false; > let loaded = false; > > function maybeCheckResults() { >- if (visited && loaded && --gTabWaitCount == 0) { >+ if ((gPrivateBrowsing.privateBrowsingEnabled || visited) && I'd prefer if you init let visited = gPrivateBrowsing.privateBrowsingEnabled, so that it is already true in this case and the if stays readable. >+ if (!gPrivateBrowsing.privateBrowsingEnabled) { and here you can do if (!visited) { that is more logical (if it's not visited, let's wait for the visit)

Will checkin once the tree is less colorful.

Landed: https://hg.mozilla.org/mozilla-central/rev/6bbe54cb876e

Almost forgot: I don't expect this to need a litmus test, as the automated tests cover it pretty well.

Backed out on suspicion of permaorange (bug 624962).

Couldn't reproduce this locally (even with a 32bit OSX build), but a couple of TryServer runs later, I seem to have fixed it. A little background info: The test here was causing the next test to fail (browser_tab_dragdrop.js). But only on OSX 10.5 32bit - which is all sorts of weird.

Comment on attachment 503610 [details] [diff] [review] Patch v2.1 I'd suppose we could do that in registerCleanupFunction, but checking db you made it a valid test, so it's ok since it was not permaorange on try, and cleaning up sounds like a good idea regardless.

Pushed http://hg.mozilla.org/mozilla-central/rev/2f9ad4aa0795

Verified with Firefox 4.0b10build1.