514214 - Do not update page titles for places already in history inside the Private Browsing mode

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[Samik Mody]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)

I have noticed this problem on Gmail's inbox page (https://mail.google.com/mail/?shva=1#inbox).

The problem is that when you go to a gmail inbox using normal browsing, and so that the URL is saved in the history.  If you start private browsing, and go to a different gmail inbox (different email address), then when you exit private browsing -- the private browsing's email address will show up in the history next to the URL above.  Since the URL is the same and exists in the history from normal browsing session, it displays the page title of the page loaded in the private browsing session.

Reproducible: Always

Steps to Reproduce:
1.Go to gmail inbox of email address A
2.Sign out.  Start Private Browsing Session.
3. GO to a different gmail inbox ([e-mail]).
4. Sign out, exit private browsing mode.
5. Look in the history, it will still show the page title of the private browsing session.
Actual Results:  
You can still see the private browsing page title.

Expected Results:  
No page title from private browsing be shown.

This is a huge problem that needs to be fixed!

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[(no longer active)]

Attachment: Patch (v2)

Moved the check to nsNavHistory::SetPageTitleInternal.  Also, in addition to the xpcshell test which provides API level testing, I added a browser chrome test to test the actual user-facing scenario.

Comment 3

[Marco Bonardo [:mak]]

This is a real edge case, but supposing i'm calling SetPageTitle, it will by default add a lazy message, that after 3s will call SetPageTitleInternal.
If i exit PB mode between the call to SetPageTitle and the real SetPageTitleInternal call the title will be updated.
Or otherwise if a lazy message is sent before pb mode starts, but executed after that, we miss a title change.

AddPageWithDetails iirc is/should_be used only by migrators so i don't know if it's a point we need to handle in PB mode. But sure, extension devs could make use of it.

I fear you will have to handle PB mode in both AddPageWithDetails and SetPageTitle.

Comment 4

[(no longer active)]

(In reply to comment #3)
> This is a real edge case, but supposing i'm calling SetPageTitle, it will by
> default add a lazy message, that after 3s will call SetPageTitleInternal.
> If i exit PB mode between the call to SetPageTitle and the real
> SetPageTitleInternal call the title will be updated.
> Or otherwise if a lazy message is sent before pb mode starts, but executed
> after that, we miss a title change.

Thanks for mentioning this.  Indeed it's an edge case, but I'm sure it will bite someone some day, so let's take it serious right now!  :-)

BTW, what do you feel about an alternative solution: flushing Places lazy messages on PB mode switches?  Could this be beneficial in that it might prevent similar edge cases in the future?

> AddPageWithDetails iirc is/should_be used only by migrators so i don't know if
> it's a point we need to handle in PB mode. But sure, extension devs could make
> use of it.

Yes, I'd prefer us to be thorough here.

> I fear you will have to handle PB mode in both AddPageWithDetails and
> SetPageTitle.

That is, instead of in SetPageTitleInternal, right?

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> (In reply to comment #3)
> > This is a real edge case, but supposing i'm calling SetPageTitle, it will by
> > default add a lazy message, that after 3s will call SetPageTitleInternal.
> > If i exit PB mode between the call to SetPageTitle and the real
> > SetPageTitleInternal call the title will be updated.
> > Or otherwise if a lazy message is sent before pb mode starts, but executed
> > after that, we miss a title change.
> 
> Thanks for mentioning this.  Indeed it's an edge case, but I'm sure it will
> bite someone some day, so let's take it serious right now!  :-)
> 
> BTW, what do you feel about an alternative solution: flushing Places lazy
> messages on PB mode switches?  Could this be beneficial in that it might
> prevent similar edge cases in the future?

That would work just fine, we have the same requirement in other points (see bug 511207, that you could even fix at the same time, since testing would be just hard or damn slow, lazy timer is 3s)

The only concern i have about this approach is that our long term plan for 3.7-4.0 is to remove all of this lazyness (LAZY_ADD messages), in such a case we would probably move to async statements, and there would be no way to force committing pending statements (could be Shawn will implement a storage method to force committing all async statements in queue).  But i'd say to move on with this commitLazyMessages approach and leave the resolution to future when the requirements for that change will be known.
Just add a clear comment on why you do commit lazy messages on PB mode switch.

Comment 6

[Samik Mody]

So can one of you offer a layman's synopsis of what's wrong, what's the fixing plan, etc? I am not a programmer so can't really understand the comments so far -- but just looking for a simple (and short) update.  When will it get fixed, etc.

Comment 7

[Daniel Veditz [:dveditz]]

This won't "block" a 1.9.1 (ff3.5) release, but once it's reviewed and landed on the trunk and 1.9.2 please request approval for 1.9.1.x and we'll get it in whatever release we're on at that time.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 398855 [details] [diff] [review]
Patch (v2)

as said above, this requires a new patch

Comment 9

[(no longer active)]

Attachment: Patch (v3)

New patch containing the requested changes.

Comment 10

[Marco Bonardo [:mak]]

Sorry for late, i'll review the patch, but unfortunatly commitPendingChanges method is going away in bug 478718. can we find an alternative solution for that?

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 404225 [details] [diff] [review]
Patch (v3)

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js

>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.

2009

>+function test() {
>+  // initialization
>+  let pb = Cc["@mozilla.org/privatebrowsing;1"].
>+           getService(Ci.nsIPrivateBrowsingService);
>+  let bhist = Cc["@mozilla.org/browser/global-history;2"].
>+              getService(Ci.nsIBrowserHistory).
>+              QueryInterface(Ci.nsIGlobalHistory2);

is the QI really needed? iirc nsIBrowserHistory extends nsIGlobalHistory2

>+  let histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                getService(Ci.nsINavHistoryService).
>+                QueryInterface(Ci.nsPIPlacesDatabase);

due to the problem above, commitPendingChanges is going to be removed, so we need an alternative way

>+  let observer = {

>+    onTitleChanged: function(aURI, aPageTitle) {
>+      if (aURI.spec != testURL)
>+        return;
>+      switch (this.pass++) {
>+      case 1: // the first time that the page is loaded
>+        is(aPageTitle, "No Cookie", "The page should be loaded without any cookie for the first time");
>+        gBrowser.selectedTab = gBrowser.addTab(testURL);
>+        break;
>+      case 2: // the second time that the page is loaded
>+        is(aPageTitle, "Cookie", "The page should be loaded with a cookie for the second time");
>+        cleanup();
>+        gBrowser.selectedTab = gBrowser.addTab(testURL);
>+        break;
>+      case 3: // before entering the private browsing mode
>+        is(aPageTitle, "No Cookie", "The page should be loaded without any cookie for the first time");

the comment looks equal to case 1, bad copy/paste?

>+        // enter private browsing mode
>+        pb.privateBrowsingEnabled = true;
>+        gBrowser.selectedTab = gBrowser.addTab(testURL);
>+        histsvc.commitPendingChanges();

So, is this only for cleanup purposes?
If so removeAllPages will take care of it.

>diff --git a/browser/components/privatebrowsing/test/unit/do_test_placesTitleNoUpdate.js b/browser/components/privatebrowsing/test/unit/do_test_placesTitleNoUpdate.js

>+
>+function do_test()
>+{
>+  let pb = Cc[PRIVATEBROWSING_CONTRACT_ID].
>+           getService(Ci.nsIPrivateBrowsingService);
>+  let bhist = Cc["@mozilla.org/browser/global-history;2"].
>+              getService(Ci.nsIBrowserHistory).
>+              QueryInterface(Ci.nsIGlobalHistory2);

is the QI really needed? iirc nsIBrowserHistory extends nsIGlobalHistory2

>+  let histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                getService(Ci.nsINavHistoryService);

then you could just let bhist = histsvc.QueryInterface(Ci.NsIBrowserHistory);

>+
>+  const testURI = uri("http://mozilla.com/privatebrowsing");
>+  const title1 = "Title 1";
>+  const title2 = "Title 2";

please make consts uppercase and separated with _ like TEST_URI per coding style

>+  function getVisitedPageTitle() {
>+    let options = histsvc.getNewQueryOptions();
>+    options.expandQueries = 1;
>+    let query = histsvc.getNewQuery();
>+    let result = histsvc.executeQuery(query, options);
>+    let node = result.root;
>+    node.containerOpen = true;
>+    do_check_eq(node.childCount, 1);
>+    let item = node.getChild(0);
>+    do_check_eq(item.itemId, -1); // should be a history item
>+    do_check_eq(item.type, item.RESULT_TYPE_URI);
>+    do_check_eq(item.uri, testURI.spec);
>+    return item.title;
>+  }

I'm not sure why you don't use histsvc.getPageTitle
but in case you really need this:
- please close the container before returning
- s/node/rootNode/
- s/item/node/
- let rootNode = histsvc.executeQuery(query, options).root;

Comment 12

[(no longer active)]

(In reply to comment #10)
> Sorry for late, i'll review the patch, but unfortunatly commitPendingChanges
> method is going away in bug 478718. can we find an alternative solution for
> that?

Is there an alternative solution to that?  I don't know the Places API that well to judge here.

Comment 13

[Marco Bonardo [:mak]]

what's the exact purpose of calling it?

Comment 14

[(no longer active)]

(In reply to comment #13)
> what's the exact purpose of calling it?

In order to make sure that any pending transactions such as those we discussed in comment 5 are committed.

Comment 15

[Marco Bonardo [:mak]]

i know what it is doing, but i need to know why we need to do that in the test.
Looks like we just do that in the purpose of cleaning up correctly the environment after test finishes. Is that correct? In such a case you could just call RemoveAllPages

Comment 16

[(no longer active)]

Attachment: Patch (v4)

(In reply to comment #11)
> (From update of attachment 404225 [details] [diff] [review])
> >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
> 
> >+ * Portions created by the Initial Developer are Copyright (C) 2008
> >+ * the Initial Developer. All Rights Reserved.
> 
> 2009

Boy, I'm living in the past! ;-)
 
> >+function test() {
> >+  // initialization
> >+  let pb = Cc["@mozilla.org/privatebrowsing;1"].
> >+           getService(Ci.nsIPrivateBrowsingService);
> >+  let bhist = Cc["@mozilla.org/browser/global-history;2"].
> >+              getService(Ci.nsIBrowserHistory).
> >+              QueryInterface(Ci.nsIGlobalHistory2);
> 
> is the QI really needed? iirc nsIBrowserHistory extends nsIGlobalHistory2

Yes, you're right.  Removed it.

> >+  let histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
> >+                getService(Ci.nsINavHistoryService).
> >+                QueryInterface(Ci.nsPIPlacesDatabase);
> 
> due to the problem above, commitPendingChanges is going to be removed, so we
> need an alternative way

Without commitPendingChanges, the test seems to pass successfully.  I've submitted a try server build as well to make sure that it's passing on all platforms.

> >+  let observer = {
> 
> >+    onTitleChanged: function(aURI, aPageTitle) {
> >+      if (aURI.spec != testURL)
> >+        return;
> >+      switch (this.pass++) {
> >+      case 1: // the first time that the page is loaded
> >+        is(aPageTitle, "No Cookie", "The page should be loaded without any cookie for the first time");
> >+        gBrowser.selectedTab = gBrowser.addTab(testURL);
> >+        break;
> >+      case 2: // the second time that the page is loaded
> >+        is(aPageTitle, "Cookie", "The page should be loaded with a cookie for the second time");
> >+        cleanup();
> >+        gBrowser.selectedTab = gBrowser.addTab(testURL);
> >+        break;
> >+      case 3: // before entering the private browsing mode
> >+        is(aPageTitle, "No Cookie", "The page should be loaded without any cookie for the first time");
> 
> the comment looks equal to case 1, bad copy/paste?

Yes, fixed this as well.

> >+        // enter private browsing mode
> >+        pb.privateBrowsingEnabled = true;
> >+        gBrowser.selectedTab = gBrowser.addTab(testURL);
> >+        histsvc.commitPendingChanges();
> 
> So, is this only for cleanup purposes?
> If so removeAllPages will take care of it.

Actually I think I had made a mistake here, and it shouldn't be necessary...

> >diff --git a/browser/components/privatebrowsing/test/unit/do_test_placesTitleNoUpdate.js b/browser/components/privatebrowsing/test/unit/do_test_placesTitleNoUpdate.js
> 
> >+
> >+function do_test()
> >+{
> >+  let pb = Cc[PRIVATEBROWSING_CONTRACT_ID].
> >+           getService(Ci.nsIPrivateBrowsingService);
> >+  let bhist = Cc["@mozilla.org/browser/global-history;2"].
> >+              getService(Ci.nsIBrowserHistory).
> >+              QueryInterface(Ci.nsIGlobalHistory2);
> 
> is the QI really needed? iirc nsIBrowserHistory extends nsIGlobalHistory2

Fixed.

> >+  let histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
> >+                getService(Ci.nsINavHistoryService);
> 
> then you could just let bhist = histsvc.QueryInterface(Ci.NsIBrowserHistory);

Done.

> >+
> >+  const testURI = uri("http://mozilla.com/privatebrowsing");
> >+  const title1 = "Title 1";
> >+  const title2 = "Title 2";
> 
> please make consts uppercase and separated with _ like TEST_URI per coding
> style

Done.

> >+  function getVisitedPageTitle() {
> >+    let options = histsvc.getNewQueryOptions();
> >+    options.expandQueries = 1;
> >+    let query = histsvc.getNewQuery();
> >+    let result = histsvc.executeQuery(query, options);
> >+    let node = result.root;
> >+    node.containerOpen = true;
> >+    do_check_eq(node.childCount, 1);
> >+    let item = node.getChild(0);
> >+    do_check_eq(item.itemId, -1); // should be a history item
> >+    do_check_eq(item.type, item.RESULT_TYPE_URI);
> >+    do_check_eq(item.uri, testURI.spec);
> >+    return item.title;
> >+  }
> 
> I'm not sure why you don't use histsvc.getPageTitle
> but in case you really need this:
> - please close the container before returning
> - s/node/rootNode/
> - s/item/node/
> - let rootNode = histsvc.executeQuery(query, options).root;

Well, probably because I din't notice this API being available!

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 408515 [details] [diff] [review]
Patch (v4)

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js

>+function test() {
>+  // initialization
>+  let pb = Cc["@mozilla.org/privatebrowsing;1"].
>+           getService(Ci.nsIPrivateBrowsingService);
>+  let bhist = Cc["@mozilla.org/browser/global-history;2"].
>+              getService(Ci.nsIBrowserHistory);
>+  let histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                getService(Ci.nsINavHistoryService).
>+                QueryInterface(Ci.nsPIPlacesDatabase);
>+  let cm = Cc["@mozilla.org/cookiemanager;1"].
>+           getService(Ci.nsICookieManager);
>+  waitForExplicitFinish();
>+
>+  const testURL = "http://localhost:8888/browser/browser/components/privatebrowsing/test/browser/title.sjs";

this should be TEST_URL per coding style

>+      case 3: // before entering the private browsing mode
>+        is(aPageTitle, "No Cookie", "The page should be loaded without any cookie again");
>+        // enter private browsing mode
>+        pb.privateBrowsingEnabled = true;
>+        gBrowser.selectedTab = gBrowser.addTab(testURL);
>+        executeSoon(function() {
>+          histsvc.removeObserver(observer);
>+          pb.privateBrowsingEnabled = false;
>+          //gBrowser.removeAllTabsBut(gBrowser.selectedTab);

What's this commented line? you forgot to remove it or it needs some comment

Looks good, r=me
Thanks.

Comment 18

[(no longer active)]

One thing I noted about this patch was that it makes the failure in bug 493962 appear every time.  I'm not sure why that is, though.  Marco, Dietrich, do you have any ideas?

Comment 19

[Marco Bonardo [:mak]]

for now the only thing i can tell is that its onLoad handler should probably executeSoon its contents. but i never looked at it so i can maybe try to reproduce your failure locally.

Comment 20

[Dietrich Ayala (:dietrich)]

marco, have you had a chance to test this locally yet?

Comment 21

[Marco Bonardo [:mak]]

hope to do that by the end of the day, bumping in my priority list.

Comment 22

[Marco Bonardo [:mak]]

i can't reproduce the failure locally, do you see the failure just running the Places tests, or running the full tests suite?
Did you try on Tryservers?

Comment 23

[Marco Bonardo [:mak]]

There is some difference in the code paths with this patch.
IsURIStringVisited checks first in the lazy queue and later in the database, before this patch i can suppose it was finding the page in the lazy queue most of the times, thus i'm starting thinking it could not find the visit in the database.
This patch forces a lazy queue submission when entering or leaving PB, so the queue is empty when we hit that method, and we fallback to checking the database.

the mDBIsPageVisited query uses UNION ALL, thinking to this as the scary bug 507790, could be the 2 sides of the UNION ALL are working on different underlying data, thus none of them see the visit. That could explain some randomness but not a perma-failure. if it's so you could try replacing the UNION ALL with a UNION and check if it's still failing. That said the isVisited query will be slower (sigh), and we will probably need bug 511960.

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 408515 [details] [diff] [review]
Patch (v4)

>   // if aTitle is empty we want to clear the previous title.
>   // We don't want to set it to an empty string, but to a NULL value,
>   // so we use SetIsVoid and SetPageTitleInternal will take care of that
>@@ -5594,9 +5602,25 @@ nsNavHistory::Observe(nsISupports *aSubj
>   else if (strcmp(aTopic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0) {
>     if (NS_LITERAL_STRING(NS_PRIVATE_BROWSING_ENTER).Equals(aData)) {
>       mInPrivateBrowsing = PR_TRUE;
>+
>+#ifdef LAZY_ADD
>+      // Commit all lazy messages in order to protect against edge cases where a
>+      // lazy message which is not allowed in private browsing mode has been
>+      // added before entering the private browsing mode, and is going to be
>+      // scheduled to be processed after entering the private browsing mode.
>+      CommitLazyMessages();
>+#endif
>     }


so, after playing a bit in linux mochitests the above code is wrong, you should commit messages BEFORE setting mInPRivateBrowsing, otherwise the queued visits visit won't be added.

Comment 25

[Marco Bonardo [:mak]]

and it's the same for LEAVE, you should first commit, then set the property.

so i suggest

 else if (strcmp(aTopic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0) {
-> commit here

>     if (NS_LITERAL_STRING(NS_PRIVATE_BROWSING_ENTER).Equals(aData))
>       mInPrivateBrowsing = PR_TRUE;
else if ...
>       mInPrivateBrowsing = PR_FALSE;

PS: does exist a third state? otherwise i don't see why this is not an else.

Comment 26

[(no longer active)]

Attachment: Patch (v5)

(In reply to comment #24)
> so, after playing a bit in linux mochitests the above code is wrong, you should
> commit messages BEFORE setting mInPRivateBrowsing, otherwise the queued visits
> visit won't be added.

Thanks for helping me with debugging this.  Here is the new patch with these fixes, and I've also pushed a try server build because I can't run the tests locally yet...

Comment 27

[(no longer active)]

(In reply to comment #25)
> and it's the same for LEAVE, you should first commit, then set the property.
> 
> so i suggest
> 
>  else if (strcmp(aTopic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0) {
> -> commit here
> 
> >     if (NS_LITERAL_STRING(NS_PRIVATE_BROWSING_ENTER).Equals(aData))
> >       mInPrivateBrowsing = PR_TRUE;
> else if ...
> >       mInPrivateBrowsing = PR_FALSE;
> 
> PS: does exist a third state? otherwise i don't see why this is not an else.

There is no third state for now, but there may be in the future, so this is why I'm assuming that there _are_ other states everywhere in our code....

Comment 28

[Marco Bonardo [:mak]]

Comment on attachment 410004 [details] [diff] [review]
Patch (v5)

Nice, thanks for quick roundup, sorry for slowness and for not having catched that on first review.

Comment 29

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3865fe768390

Comment 30

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/437d2de19500