558321 - Tab Matches are not honoured in Private Browsing mode

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Aaron Train [:aaronmt]]

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

Comment 1

[u279076]

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

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 3

[(no longer active)]

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.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 5

[Dietrich Ayala (:dietrich)]

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?

Comment 6

[Marco Bonardo [:mak]]

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).

Comment 7

[Alex Limi (:limi) — Firefox UX Team]

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.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 9

[Marco Bonardo [:mak]]

(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.

Comment 10

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

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.

Comment 11

[(no longer active)]

(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.  :(

Comment 12

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

(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.

Comment 13

[Nochum Sossonko [:Natch]]

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

Comment 14

[Marco Bonardo [:mak]]

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.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

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.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1

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

Comment 18

[Marco Bonardo [:mak]]

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?

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 20

[Marco Bonardo [:mak]]

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

Comment 21

[Marco Bonardo [:mak]]

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 22

[Marco Bonardo [:mak]]

Comment on attachment 501587 [details] [diff] [review]
Patch v1.1

review conditioned to verifying test goodness.

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

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 24

[Marco Bonardo [:mak]]

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)

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.01 - ready for checkin

Will checkin once the tree is less colorful.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 28

[Justin Dolske [:Dolske]]

Backed out on suspicion of permaorange (bug 624962).

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.1

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 30

[Marco Bonardo [:mak]]

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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

Comment 32

[u279076]

Verified with Firefox 4.0b10build1.