463607 - Interaction of Clear Recent History dialog and the private browsing mode

Status: NEW

(Firefox :: Private Browsing, defect, P3)

Description

[(no longer active)]

The Clear Recent History dialog, when used inside the private browsing mode, might not have the expected affect.  For example, selecting downloads in that dialog would remove the downloads from the temporary in-memory DB which we're using inside the private browsing mode.  The same goes for history as well.  Selecting the cache would only empty the memory cache, as the disk and offline caches are turned off.

We need to first determine what we want to do, and then make sure that the current implementation would satisfy that.  I'm CCing the relevant people I can think of here.  Also, Johnath, the tests you wrote in bug 453440 should really handle this interaction with the PB service as well.

I think this is a critical issue which would confuse many people if we can't deliver what's expected here.  I think this should block the release of Firefox 3.1.  We should either come up with the decision that the current behavior is what we want, or fix it here.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Again, blocking for decision. Discussion should take place in dev-apps-firefox instead of in the bug, fwiw, and we should loop back here when we have a proposal.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Isn't the intended behavior pretty straightforward, Clear Recent History should act on the user's previous session while in Private Browsing Mode.  One caveat is that the time periods should still be based off of the present time, so if you entered private browsing mode 2 hours ago, and you clear the last hour, that effectively does nothing.  If you entered private browsing mode 4 hours ago, and you clear the last 6 hours, that would clear 2 hours of your session before entering private browsing mode.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Isn't the intended behavior pretty straightforward, Clear Recent History should
> act on the user's previous session while in Private Browsing Mode.  One caveat
> is that the time periods should still be based off of the present time, so if
> you entered private browsing mode 2 hours ago, and you clear the last hour,
> that effectively does nothing.  If you entered private browsing mode 4 hours
> ago, and you clear the last 6 hours, that would clear 2 hours of your session
> before entering private browsing mode.

This may require backend changes to at least the cookies and downloadmgr modules.  I think we would want specialized APIs for these modules instead of just exposing the non-private cookies and downloads through the normal API.

Comment 4

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 5

[(no longer active)]

Is the behavior described in comment 2 considered final for 3.1?  Need to get a patch here.

Comment 6

[Johnathan Nightingale [:johnath]]

Yes, given the expected user behaviour here, the desirable thing for CRH to do is clear as though PBM weren't around, clear the "real" history based on timespan. I'm a little worried about the potential for code risk, but if you can convince yourself and mconnor about the safety of a late-stage patch, so be it.

I'm not sure that this blocks since there isn't data loss (but rather surprising data retention) and since there's a workaround (CRH outside of private browsing mode) but I would very much like to see us fix it as, I'm sure, would you and Alex. 

Beltzner +'d this for discussion but I think there is a best choice here, so I'm re-nom'ng to give drivers a chance to decide whether it blocks or not. Keep in mind that whether it blocks or not, it is a highly desirable fix, and would likely get approval with sufficient test coverage and code-risk confidence.

Comment 7

[(no longer active)]

I'll try to come up with a patch in the next few days.  Like I said, this requires major back-end changes to at least the cookies and downloads modules, because the disk-based DBs behind these modules is turned off during the private browsing mode, so we would need new APIs for these modules.  The situation may be similar with other modules as well, I just haven't checked yet.

At any rate, I ***really*** think this should block, because we show the CRH button in about:privatebrowsing, and it would be a shame that the users find out that this only half-works -- especially since it is a privacy feature which users may seriously rely on.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

>I'm not sure that this blocks since there isn't data loss (but rather
>surprising data retention) and since there's a workaround (CRH outside of
>private browsing mode) but I would very much like to see us fix it as, I'm
>sure, would you and Alex. 

I think this should block, surprising data retention is the new data loss.

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

Also, if this doesn't block we should disable CRH when in private browsing mode, which would be unfortunate, since it is meant as an easy "undo" feature, and a big part of giving user's total control.

Comment 10

[(no longer active)]

I think it would be very nice to give this beta exposure as well, so I'll boost its priority on my queue...

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Something blocks here. It's either:

 - disable the CRH when we enter PB mode, or
 - make CRH from the welcome to Private Browsing Mode screen do what's expected

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

This is one of the highest risk front-end blockers; Ehsan, I know you've been busy, but back in comment 7 (Dec '08) you'd indicated that you really wanted to fix this and would have a patch in a few days.

Any update? We need to do one of the approaches from comment 11, and in order to mitigate risk I'm leaning towards the former.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, makin' a call here. We should disable CRH in PB mode and remove the link from the about:privatebrowsing page.

Then we can consider a follow-up to fix this the other way in a future release.

Ehsan: do you want to take this one or should I find someone else to do it?

Comment 14

[Nochum Sossonko [:Natch]]

I can take this Ehsan if you don't have time.

Comment 15

[(no longer active)]

I somehow missed comment 12 here, or I'd have posted a status update sooner.  This one slipped off in the pile of private browsing bugs, but I've prioritized my work on the PB bugs recently (and tagged this one as [PB-P1]), currently this is on the top of my queue, and I have in fact started to patch up the cookies end.  I will have a patch here by the weekend (and this time I *will*! ;-) ) which would most likely implement the second option in comment 11.  If for some reason that proves to be unfeasible in that time frame, I'll post a patch to disable CRH in the PB mode.

Sorry for the delay here, and thanks Natch for the offer.  Beltzner: considering that Beta 3 release date is not settled yet, would it be good enough for me to come up with a patch by the end of the week, and get people to review it?  The patch will come with sufficient automated test coverage to minimize the risk.

Comment 16

[[e-mail]]

i can turn out a quick review here.

Comment 17

[(no longer active)]

Attachment: Patch (v1)

Patch with unit tests.

The patch doesn't touch the sanitizer module except for the cookie part which switches to use the new nsICookieManager2::removeByTimeRange method.  Requesting review from mconnor on this part.

The fixes to the cookie manager back-end include a new API, removeByTimeRange which is nearly straightforward (with jumping through a few hoops to clear data from both the default and private tables).  It also fixes nsICookieManager::removeAll to remove all the data inside the DB and the default table when the private browsing mode is active. It also includes a unit test for the removeAll and removeByTimeRange APIs which covers them both in private and normal mode.  Requesting review from Daniel on the cookie part.

The fixes to the cache part make nsICacheService::evictEntries evict the disk and offline entries even if inside the private browsing mode.  The patch includes a unit test which tests the evictEntries API both inside and outside of the private browsing mode with all possible storage policies.  Requesting review from Christian on the cache part.

The fixes to the download manager make sure that the cleanUp and removeDownloadsByTimeframe APIs both clean the on-disk data inside the private browsing mode.  It also makes sure that the value of canCleanUp is in sync with the new changes.  The patch also adds test cases specific to the private browsing mode handling of removeDownloadsByTimeframe to the existing unit test, and introduces a new unit test for the cleanUp API both inside and outside of the private browsing mode.  Requesting review from Shawn on the download manager part.

Finally requesting superreview from Boris on the cookie and cache parts.

Comment 18

[(no longer active)]

Attachment: Patch (v1.1)

Fixed a compilation problem with gcc.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 363581 [details] [diff] [review]
Patch (v1.1)

What happened with the suggested implementation in comment 13?  I'm not happy with the complexity this adds to the download manager, since it adds more invariants we have to worry about.

Comment 20

[(no longer active)]

(In reply to comment #19)
> (From update of attachment 363581 [details] [diff] [review])
> What happened with the suggested implementation in comment 13?  I'm not happy
> with the complexity this adds to the download manager, since it adds more
> invariants we have to worry about.

Hmm, I think comment 13 was suggested because of my delay in coming up with a patch here, in order to make sure that the CRH dialog doesn't lead to confusion inside the private browsing mode for 3.1.

What kinds of invariants are you worried about?  The download manager patch consists of one refactoring (the GetDBFile function) and three similar changes which only affect the behavior of the APIs inside the private browsing mode.  If you think that we can mitigate the risk by more unit tests then I'm all for it.

And finally even if this won't be considered acceptable for 3.1 (which would be unfortunate), then I think we should take a variant of this patch for 3.2 and then do what comment 13 suggested for 3.1.

Comment 21

[Shawn Wilsher :sdwilsh]

(In reply to comment #20)
> Hmm, I think comment 13 was suggested because of my delay in coming up with a
> patch here, in order to make sure that the CRH dialog doesn't lead to confusion
> inside the private browsing mode for 3.1.
I think comment 13 is a reaction to how late we are in the 3.1 cycle and should only be taking small changes whenever necessary.  beltzner can comment more about that though.

> What kinds of invariants are you worried about?  The download manager patch
> consists of one refactoring (the GetDBFile function) and three similar changes
> which only affect the behavior of the APIs inside the private browsing mode. 
> If you think that we can mitigate the risk by more unit tests then I'm all for
> it.
Until this, we never had to worry about which database we are using.  Now we do.  Additionally, this is a behavior change on those APIs, which would require a uuid change.  We're not supposed to be doing that anymore for 3.1.

> And finally even if this won't be considered acceptable for 3.1 (which would be
> unfortunate), then I think we should take a variant of this patch for 3.2 and
> then do what comment 13 suggested for 3.1.
This bug is blocking 3.1, so we shouldn't be doing work that might not make 3.1 in this bug.

Comment 22

[(no longer active)]

OK, let's hear what Beltzner thinks here, in regard to taking something along the lines of this patch, or just disabling the CRH dialog in private browsing mode.

Comment 23

[Mike Connor [:mconnor]]

We'll make a call on whether this is or isn't the safe for branch once we have it landed and baking.  We should prep an alternate patch to just disable the feature in PB mode, but I'd prefer to get this into 3.1 if we're confident on the risk factors.

In the meantime, we should review the patch for trunk, and then decide if we're willing to take the risk for b3 or final once we have a final patch.

Comment 24

[(no longer active)]

I will attach an alternative patch to disable CRH inside the private browsing mode (which should be fairly simple) to make sure we have a patch ready if the main patch doesn't get into 3.1...

Comment 25

[(no longer active)]

If this gets into 3.1, it will be late-compat; marking it as such.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Late compat for consumers of PB or for add-ons that touch things like the download manager and history? If the latter, I think I've just talked myself out of taking this for 3.1, as I don't think we want to cause any add-ons to have to rebuild themselves for this.

Comment 27

[(no longer active)]

Late compat for consumers of nsICookieManager2 and nsIDownloadManager.  :(

I think there's one last hope of getting this for 3.1, and that is adding new interfaces for the changed APIs, such as nsICookieManager3, etc.  That way no extension needs to rebuild themselves.  Would it be OK?

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> Late compat for consumers of nsICookieManager2 and nsIDownloadManager.  :(
> 
> I think there's one last hope of getting this for 3.1, and that is adding new
> interfaces for the changed APIs, such as nsICookieManager3, etc.  That way no
> extension needs to rebuild themselves.  Would it be OK?
That wouldn't help us with the download manager since you are changing the semantics of an existing function.

Comment 29

[(no longer active)]

(In reply to comment #28)
> That wouldn't help us with the download manager since you are changing the
> semantics of an existing function.

If this is deemed acceptable, I will modify my patch to add new methods with the new semantics, instead of changing the semantics of existing methods...

Comment 30

[Mike Connor [:mconnor]]

I think we need to just punt this to fix on trunk, and disable CPH in PB.  I just don't think the benefit is worth the risk and compat hit.  We should do this right on trunk, and get the patch to disable the menuitem on PB entry.

Comment 31

[Mike Connor [:mconnor]]

spun off  Bug 480260 for the branch only disabling, this bug will track the trunk work.

Comment 32

[[e-mail]]

might want to do something with permissionmanager here (see e.g. bug 480811), since it currently doesn't partake in anything PB and we might want a mechanism for clearing recent hosts out of the list.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 363581 [details] [diff] [review]
Patch (v1.1)

>+++ b/netwerk/cache/src/nsCacheService.cpp
>+        PRBool privateBrowsingException = PR_FALSE;
>+        if (mObserver->InPrivateBrowsing()) {
>+            nsCOMPtr<nsIPrefBranch> branch = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+            if (branch) {
>+                (void) branch->GetBoolPref(DISK_CACHE_ENABLE_PREF,
>+                                           &privateBrowsingException);
>+            }
>+        }

Might be nice to factor this out into a little static method that takes mObserver and the pref name instead of copy/pasting the code twice.  Also, I'd prefer we check the return value of GetBoolPref here and explicitly set privateBrowsingException to whatever the default pref value is when the pref is not set.  For example, DISK_CACHE_ENABLE_PREF defaults to true if not set, whereas your code is defaulting it to false...  same for OFFLINE_CACHE_ENABLE_PREF.

>+++ b/netwerk/cookie/public/nsICookieManager2.idl
>+   * @param aRangeStart the start of the time range
>+   * @param aRangeEnd   the end of the time range
>+   */
>+  void removeByTimeRange(in PRInt64 aRangeStart, in PRInt64 aRangeEnd);

Start and end in seconds since the big bang?  In microseconds since the end of the battle of Lepanto?

Just use PRTime if that's what those values are.  If they're not, you need to very clearly document the exact units and the exact zero point.  Given the way this value is used, PRTime seems to be the appropriate argument here, I think.

>+++ b/netwerk/cookie/src/nsCookieService.cpp
>+nsCookieService::GetPrivateOrNormalDBConn() const
>+{
>+  if (mPrivateDBConn)
>+    return mPrivateDBConn.get();
>+  else if (mDBConn)
>+    return mDBConn.get();
>+  else
>+    return nsnull;

No need for else-after-return or for .get() calls here.

>+struct nsRemoveByTimeRangeData
>+{
>+  nsRemoveByTimeRangeData(PRInt64 aRangeStart, PRInt64 aRangeEnd,

Again, PRTime, I should think.  Or an explicit comment about what they are.

>+nsCookieService::RemoveByTimeRange(PRInt64 aRangeStart, PRInt64 aRangeEnd)
>+  nsCOMPtr<mozIStorageStatement> stmtDelete;
>+  nsresult rv = CreateDeleteStatement(getter_AddRefs(stmtDelete));

So we want to use the same dbconn here for both tables?  Why?

>+  PRUint32 count;

I think this part could use some documenting (in particular, that you want to update mCookieCount in some cases, but not others).

>+++ b/netwerk/cookie/src/nsCookieService.h

CloseDB shouldn't have an optional argument.  There aren't that many callers; just make them pass the right thing.

Document what aClosePrivate means for CloseDB, what all the new arguments for RemoveCookieFromList mean, and the new methods you're adding.

Document how mDBConn and mPrivateDBConn differ and which one may be null when.

I haven't looked at any of the non-netwerk code.

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 363581 [details] [diff] [review]
Patch (v1.1)

Think we're going to need a new patch first.

Comment 36

[Tom Lowenthal]

It would be lovely to fix this bug.

Comment 37

[(no longer active)]

I'm not really working on this bug...

Comment 39

[(no longer active)]

Given the fact that we now have support for per-window PB, is this still an issue?  Perhaps we should just hide this menu item in private windows (it is accessible from non-private windows.)

Comment 40

[Ioana (away)]

(In reply to :Ehsan Akhgari from comment #39)
> Given the fact that we now have support for per-window PB, is this still an
> issue?  Perhaps we should just hide this menu item in private windows (it is
> accessible from non-private windows.)

I tested the 02/18 Aurora (20130218042018) and:
- Clear Recent History is grayed out in the Tools menu,
- "clear your recent history" is still available in Options->Privacy and it opens the Clear Recent History dialog, but clearing history doesn't work.

The link in Options->Privacy should also be disabled.

Comment 41

[Ioana (away)]

Changing severity per comment 40.

Comment 46

[mdn]

I am also experiencing this. Is it agreeable that Options->Privacy should be grayed out in a PB window?

Comment 47

[(no longer active)]

(In reply to mdn from comment #46)
> I am also experiencing this. Is it agreeable that Options->Privacy should be
> grayed out in a PB window?

Sure.

Comment 50

[Timea Cernea [:tbabos][inactive]]

This is still reproducible as described in Comment 40. Updating severity based on the new severity system.

Comment 51

[Kevin Cox [@kevincox:matrix.org]]

I tried to clear cookies in a private-browsing window using the "URL Bar → 🔒 → Clear cookies and site data..." option in a private window and it didn't clear cookies but cleared them in the regular profile session! That was an unpleasant surprise.

It seems like it should:

1. Confirm with the user that it is clearing cookies in the private session.
2. Clear cookies in the private session and only in the private session.