787743 - (pbchannelfail) Private Browsing mode not working in Firefox 15.0

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[RayBaby]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

Manually set to Private Browsing mode and visited various websites.   Then I turned off Private Browsing mode, and returned to regular web browsing.   


Actual results:

I checked my cache file using the CacheViewer addon, and found the sites I had visited using Private Browsing mode, were added to my cache.   


Expected results:

Those sites should not have been added to my cache, as they should have cleared when I exited Private Browsing mode.

I have since gone back to Firefox 14.1 as the Private Browsing mode works fine in that version.

Comment 1

[Josh Matthews [:jdm]]

If you look at about:cache in Firefox 15, can you understand the output well enough to perform the same test without the use of CacheViewer? Could you try reproducing the problem in safe mode?

Comment 2

[RayBaby]

Thank You for responding to me!  I tried loading Firefox 15 in safemode.  I checked the cache size and found it growing even while using the Private Browsing mode.

Comment 3

[RayBaby]

Of course, I tried loading Firefox 15 and found cached everything - Private Browsing included.

Comment 4

[Josh Matthews [:jdm]]

I would expect the cache to grow while in PB mode, since we use it. It's the clearing that's supposed to happen that I'm worried about. In about:cache, I would specifically be interested in finding out if the memory cache is being cleared when you exit PB mode in safe FF 15 while in safe mode.

Comment 5

[Josh Matthews [:jdm]]

Crap. I can reproduce this. Thank you very much for this report! This is almost certainly a regression from bug 722845.

Comment 6

[Josh Matthews [:jdm]]

I'm assuming subsequent versions are affected as well at this point.

Comment 7

[Josh Matthews [:jdm]]

Curious; I see the memory device get cleared ~5 seconds after leaving PB mode in current nightlies.

Comment 8

[Josh Matthews [:jdm]]

It would be worth finding out whether Aurora and Beta see this problem too.

Comment 9

[(no longer active)]

Yeah, totally reproducible.  I wonder why our unit tests did not catch this?

Comment 10

[Alex Keybl [:akeybl]]

If anybody can provide info on whether Fennec is similarly affected, it'd be much appreciated.

Comment 11

[(no longer active)]

If I'm reading https://hg.mozilla.org/mozilla-central/rev/0e92b352474b correctly, that patch is totally wrong.  We should never store private entries in the disk cache device in the first place.  That patch includes code to remove them once we leave the PB mode, and why that doesn't work is still worth investigating, but the bigger problem here is that we store those entries on disk in the first place.

Comment 12

[(no longer active)]

(In reply to Alex Keybl [:akeybl] from comment #10)
> If anybody can provide info on whether Fennec is similarly affected, it'd be
> much appreciated.

Fennec doesn't support PB mode, so it's not affected.

Comment 13

[(no longer active)]

If we're going to chemspill for this, I think we should back out bug 722845 instead of attempting to fix this on the release channel.  We can evaluate the riskiness of the real fix for Aurora and Beta separately.

Comment 14

[(no longer active)]

Attachment: Backout patch

[Approval Request Comment]
Regression caused by (bug #): bug 722845
User impact if declined: PB mode is severely broken
Testing completed (on m-c, etc.): I have tested this patch locally.  Without this patch, the bug happens as described in comment 0.  Without it, it doesn't.
Risk to taking this patch (and alternatives if risky):  This is not the smallest patch, but it will take us back to the Firefox 14 behavior as far as handling the private browsing mode in Necko is concerned.  I am much more comfortable with this backout than any other patch to spot fix it.

Comment 15

[(no longer active)]

OK, it seems like some of the entry objects in PB mode do not have the private bit set.  Example when loading news.yahoo.ca in PB mode (watch the mFlags member):

(gdb) p/x *entry
$18 = {<PRCListStr> = {next = 0x11cdb7b00, prev = 0x11cdb7b00}, mKey = {<nsACString_internal> = {mData = 0x115a32808, mLength = 0x448, mFlags = 0x5}, <No data fields>}, 
  mFetchCount = 0x1, mLastFetched = 0x504622fc, mLastModified = 0x504622fc, mLastValidated = 0xa5a5a5a5, mExpirationTime = 0x63120bd7, mFlags = 0x7a00, mPredictedDataSize = 0x2ca2, 
  mDataSize = 0x0, mCacheDevice = 0x0, mCustomDevice = 0x0, mSecurityInfo = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}, mData = 0x0, mThread = {mRawPtr = 0x0}, 
  mMetaData = {mBuffer = 0x11c76b6a0, mBufferSize = 0x185, mMetaSize = 0x185}, mRequestQ = {next = 0x11cdb7b80, prev = 0x11cdb7b80}, mDescriptorQ = {next = 0x11817bb88, 
    prev = 0x11817bb88}}

Comment 16

[u279076]

Assigning myself as QA contact. Please let me know if there is anything I can do to help here.

Comment 17

[u279076]

Can someone please clarify the STR and expected results without the CacheViewer add-on installed?

Comment 18

[(no longer active)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #17)
> Can someone please clarify the STR and expected results without the
> CacheViewer add-on installed?

You can clear the browser cache outside of the PB mode, go into PB mode, browse some websites (such as news.yahoo.ca) and then leave PB mode and open about:cache?device=disk.  That page should show no entries.

Comment 19

[u279076]

Thanks Ehsan. I can at least confirm this now on Firefox 16.0b1. Just to be completely explicit...

Steps to Reproduce:
1. Open about:cache
2. Clear cache through Tools > Clear Recent History
> Disk cache should report 0KB and have no entries
3. Start Private Browsing
4. Navigate to www.yahoo.ca and www.amazon.ca
5. Open about:cache and click "List cache entries" under Disk Cache
> Should see a couple cached items, particularly from yahoo and amazon
6. Stop private browsing mode
7. Open about:cache and click "List cache entries" under Disk Cache
> Disk cache should report 0KB and have no entries

Actual Result:
Disk cache entries from Private Browsing mode are visible in Normal Browsing mode.

Comment 20

[u279076]

Also reproduces with Firefox 15.0, Firefox 17.0a2 2012-09-04, and Firefox 18.0a1 2012-09-04.

Comment 21

[(no longer active)]

So, debugging this a bit further, here is the call stack to one place where we end up creating a HttpCacheQuery object with the incorrect PB information:

(gdb) bt 10
#0  mozilla::net::HttpCacheQuery::HttpCacheQuery (this=0x13fd851e0, channel=0x13edc0000, clientID=@0x7fff5fbf66f0, storagePolicy=0, usingPrivateBrowsing=false, 
    cacheKey=@0x7fff5fbf6690, accessToRequest=3, noWait=false, usingSSL=false, loadedFromApplicationCache=false)
    at /Users/ehsanakhgari/moz/tmp/netwerk/protocol/http/nsHttpChannel.cpp:229
#1  0x000000010136d443 in mozilla::net::nsHttpChannel::OpenNormalCacheEntry (this=0x13edc0000, usingSSL=false)
    at /Users/ehsanakhgari/moz/tmp/netwerk/protocol/http/nsHttpChannel.cpp:2556
#2  0x0000000101360ded in mozilla::net::nsHttpChannel::OpenCacheEntry (this=0x13edc0000, usingSSL=false) at /Users/ehsanakhgari/moz/tmp/netwerk/protocol/http/nsHttpChannel.cpp:2464
#3  0x000000010135fbec in mozilla::net::nsHttpChannel::Connect (this=0x13edc0000) at /Users/ehsanakhgari/moz/tmp/netwerk/protocol/http/nsHttpChannel.cpp:427
#4  0x000000010137361d in mozilla::net::nsHttpChannel::AsyncOpen (this=0x13edc0000, listener=0x11c7ff2e0, context=0x0)
    at /Users/ehsanakhgari/moz/tmp/netwerk/protocol/http/nsHttpChannel.cpp:4424
#5  0x0000000101373817 in non-virtual thunk to mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*) ()
    at /Users/ehsanakhgari/moz/tmp/netwerk/protocol/http/nsHttpChannel.cpp:4443
#6  0x00000001017b1ebb in mozilla::css::Loader::LoadSheet (this=0x13f778c00, aLoadData=0x13ebe8cc0, aSheetState=mozilla::css::eSheetNeedsParser)
    at /Users/ehsanakhgari/moz/tmp/layout/style/Loader.cpp:1560
#7  0x00000001017b6d7d in mozilla::css::Loader::LoadStyleLink (this=0x13f778c00, aElement=0x13f144b70, aURL=0x11c8aba00, aTitle=@0x7fff5fbf75e8, aMedia=@0x7fff5fbf74a8, 
    aHasAlternateRel=false, aObserver=0x0, aIsAlternate=0x7fff5fbf74a7) at /Users/ehsanakhgari/moz/tmp/layout/style/Loader.cpp:1926
#8  0x0000000101af50ac in nsStyleLinkElement::DoUpdateStyleSheet (this=0x13f144bf8, aOldDocument=0x0, aObserver=0x0, aWillNotify=0x7fff5fbf773e, aIsAlternate=0x7fff5fbf773d, 
    aForceUpdate=false) at /Users/ehsanakhgari/moz/tmp/content/base/src/nsStyleLinkElement.cpp:279
#9  0x0000000101af52af in nsStyleLinkElement::UpdateStyleSheetInternal (this=0x13f144bf8, aOldDocument=0x0, aForceUpdate=false)
    at /Users/ehsanakhgari/moz/tmp/content/base/src/nsStyleLinkElement.cpp:189

On frame 6 at that stack, the PB flag on the docshell seems to be correctly set:

(gdb) p loadGroup.mRawPtr->mCallbacks.mRawPtr->mWeakPtr.mRawPtr->mReferent->mInPrivateBrowsing
$102 = true

That channel does not have notification callbacks set up on it yet, but it does have a load group set up on it.  My theory is that we should set mPrivateBrowsing in HttpBaseChannel::SetLoadGroup in addition to SetNotificationsCallback.  I'll test that theory in a sec.

Comment 22

[(no longer active)]

Comment on attachment 658110 [details] [diff] [review]
Backout patch

Jason, can you please take a look at this and imagine ways in which this would break some stuff?  Note that this is a straight backout of bug 722845 with some conflict fixes, so there's really no code changes to review.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Uh, yes.  mPrivateBrowsing definitely needs to be reset when changing the loadgroup...  In particular, lots of loads don't have notification callbacks of their own at all!

Comment 24

[Josh Matthews [:jdm]]

(In reply to Ehsan Akhgari [:ehsan] from comment #18)
> (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #17)
> > Can someone please clarify the STR and expected results without the
> > CacheViewer add-on installed?
> 
> You can clear the browser cache outside of the PB mode, go into PB mode,
> browse some websites (such as news.yahoo.ca) and then leave PB mode and open
> about:cache?device=disk.  That page should show no entries.

The STR that I was focusing on in my testing was watching to see if the memory cache would be cleared after leaving PB mode. In 15 this did not occur, in 18 it did. There may be several issues that need to be sorted out here.

Comment 25

[Josh Matthews [:jdm]]

Also, if we had not stored a cached mPrivateBrowsing value and had simply queried the callbacks on demand, I don't think this would have been a problem on trunk.

Comment 26

[Alex Keybl [:akeybl]]

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> (In reply to Alex Keybl [:akeybl] from comment #10)
> > If anybody can provide info on whether Fennec is similarly affected, it'd be
> > much appreciated.
> 
> Fennec doesn't support PB mode, so it's not affected.

It does have "Clear private data" though, which has the option to clear the cache. Just wanted to make sure. Thanks!

Comment 27

[(no longer active)]

(In reply to Alex Keybl [:akeybl] from comment #26)
> (In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > (In reply to Alex Keybl [:akeybl] from comment #10)
> > > If anybody can provide info on whether Fennec is similarly affected, it'd be
> > > much appreciated.
> > 
> > Fennec doesn't support PB mode, so it's not affected.
> 
> It does have "Clear private data" though, which has the option to clear the
> cache. Just wanted to make sure. Thanks!

Yeah, that should be fine.

Comment 28

[(no longer active)]

Attachment: Patch (v1)

This patch fixes what I was talking about in comment 21.

Comment 29

[(no longer active)]

I discussed this with Josh in detail today.  It turns out that we need to audit all of the call sites where we create a channel both from C++ and JS, and make sure that they have a sane load context or load group.  This makes trying to fix this bug on the branches very scary.  We also have a bunch of b2g changes on top of bug 722845 which means that we can't back out that patch on trunk.  What I will do here is to mark all of the bugs which are caused by channels improperly being set up as tracking18+, and I'll CC akeybl on them (and I will also make them block this bug).  We need to fix all of them in order to address this bug in Firefox 18.

For Firefox 16 and 17, we think that we can safely come up with backout patches.  Josh is currently working on that.

Comment 30

[(no longer active)]

Comment on attachment 658110 [details] [diff] [review]
Backout patch

Try server push: https://tbpl.mozilla.org/?tree=Try&rev=aacc99e888c8

Comment 31

[u279076]

(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> Comment on attachment 658110 [details] [diff] [review]
> Backout patch
> 
> Try server push: https://tbpl.mozilla.org/?tree=Try&rev=aacc99e888c8

I've done some initial testing with the Linux builds and so far this is no longer reproducible. At the very least, the test in comment 19 no longer reproduces this bug with the tryserver build.

Comment 32

[Josh Matthews [:jdm]]

Attachment: BETA: Back out bug 722845 and bug 748890.

Backout patch for mozilla-beta. I had to make some changes, so I'll put together a patch for review that highlights those.

Comment 33

[Josh Matthews [:jdm]]

Comment on attachment 658110 [details] [diff] [review]
Backout patch

Sorry, bugzilla flail on my part. See the earlier approval request for Ehsan's comments.

Comment 34

[Josh Matthews [:jdm]]

Try push at https://tbpl.mozilla.org/?tree=Try&rev=230305ca4637.

Comment 35

[Jason Duell]

Comment on attachment 658110 [details] [diff] [review]
Backout patch

Review of attachment 658110 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see anything obvious that could cause breakage.

Comment 37

[u279076]

(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> Comment on attachment 658110 [details] [diff] [review]
> Backout patch
> 
> Try server push: https://tbpl.mozilla.org/?tree=Try&rev=aacc99e888c8

Initially having cleared my cache, I've spent the last half-hour browsing to various websites including but not limited to Youtube, Facebook, Amazon, Yahoo, GMail, Hotmail, CNN, NYTimes, The Globe and Mail, CBC.ca, Engadget, and Arstechnica. I made sure to switch out and in to Private Browsing mode every few minutes. I have yet to see any cache data leak from my Private Browsing session into a normal session using the try-server builds.

The backout at least seems to have "fixed" the regression from Firefox 14.

Comment 38

[Josh Matthews [:jdm]]

Comment on attachment 658202 [details] [diff] [review]
BETA: Back out bug 722845 and bug 748890.

Jason, I had to introduce a bit of new code in this because I reintroduced an old cache lock, which now has telemetry associated with it. See new histogram added and use in nsCacheService::OnEnterExitPrivateBrowsing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 722845
User impact if declined: PB mode leaks information in current betas.
Testing completed (on m-c, etc.): Awaiting QA hammering; tested STR in local build and didn't see buggy results.
Risk to taking this patch (and alternatives if risky): Small. Reverting to known behaviour, not much work depended on the work being reverted (and it too is reverted).
String or UUID changes made by this patch: None.

Comment 39

[u279076]

(In reply to Josh Matthews [:jdm] from comment #34)
> Try push at https://tbpl.mozilla.org/?tree=Try&rev=230305ca4637.

This build looks good as well, no PB cache leakage detected with similar testing to comment 37.

Comment 40

[Alex Keybl [:akeybl]]

Comment on attachment 658110 [details] [diff] [review]
Backout patch

[Triage Comment]
Please land on mozilla-release for a go to build tomorrow morning PT. Thanks!

Comment 41

[Josh Matthews [:jdm]]

Aurora try push: https://tbpl.mozilla.org/?tree=Try&rev=4f75eeae2fe5

Comment 42

[Josh Matthews [:jdm]]

Attachment: AURORA: Back out bug 722845.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 722845
User impact if declined: Information-leaky PB on Aurora.
Testing completed (on m-c, etc.): Tested STR locally, could not reproduce.
Risk to taking this patch (and alternatives if risky): Same as backouts on other branches.
String or UUID changes made by this patch: None.

Comment 43

[Josh Matthews [:jdm]]

Comment on attachment 658202 [details] [diff] [review]
BETA: Back out bug 722845 and bug 748890.

A review here shouldn't be necessary, I just ended up fixing up a place that needed a telemetry histogram for the main thread cache lock.

Comment 44

[(no longer active)]

(In reply to Alex Keybl [:akeybl] from comment #40)
> Comment on attachment 658110 [details] [diff] [review]
> Backout patch
> 
> [Triage Comment]
> Please land on mozilla-release for a go to build tomorrow morning PT. Thanks!

Does this also need to get on a relbranch?

Comment 45

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-release/rev/37b83cdbc68e

Comment 46

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/829a13b9968c
https://hg.mozilla.org/releases/mozilla-beta/rev/849137ee7609

Comment 47

[(no longer active)]

The beta patch did not stick because of build failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=14964127&tree=Mozilla-Beta&full=1

So I backed it out: https://hg.mozilla.org/releases/mozilla-beta/rev/98bde6121840

Josh, did beta build successfully for you locally?

Comment 48

[(no longer active)]

And I backed out from Aurora as well because of xpcshell test failures :(

https://hg.mozilla.org/releases/mozilla-aurora/rev/d5acbc9f90a6
https://tbpl.mozilla.org/php/getParsedLog.php?id=14965375&tree=Mozilla-Aurora&full=1

Comment 49

[Josh Matthews [:jdm]]

Sorry, I apparently qrefed some changes in the patch containing the try message for the Beta push. I definitely ran the modified cache xpcshell test on my Aurora build; I'll figure out what's up with the other one.

Comment 50

[Josh Matthews [:jdm]]

Attachment: Beta backout

Comment 51

[Josh Matthews [:jdm]]

Attachment: Aurora: Back out bug 722845.

Comment 52

[(no longer active)]

Comment on attachment 658182 [details] [diff] [review]
Patch (v1)

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dfafdd2f631

Comment 53

[u279076]

Overnight QA of the try builds looks promising as this issue appears to have been resolved and no new regressions were found. Details of the testing can be viewed here:

https://wiki.mozilla.org/Releases/Firefox_15/Test_Plan#Private_Browsing

I think it's safe to proceed with generating 15.0.1 candidate builds.

Comment 54

[(no longer active)]

Josh, are the aurora and beta patches here ready for landing?

Comment 55

[Josh Matthews [:jdm]]

Yes.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6dfafdd2f631

Comment 57

[Alex Keybl [:akeybl]]

Comment on attachment 658487 [details] [diff] [review]
Aurora: Back out bug 722845.

Can we get an r+ so that we can uplift to branches today?

Comment 58

[(no longer active)]

Comment on attachment 658481 [details] [diff] [review]
Beta backout

These are just backouts.  They don't need r+.  In fact I was about to push them to Aurora and Beta since I thought that the previous approval is still withstanding.

Comment 59

[u279076]

Now that this is confirmed to be resolved in the 15.0.1 candidate build I'm flagging for tests. I think we definitely need a manual MozTrap test to verify this in pre-release builds. However, I'm also flagging this to see if we can automate a Mozmill test or if this can be handled by a mochitest.

For the purposes of testing, Alex Keybl suggested we "grep the profile" for references to domains visited during Private Browsing.

Comment 60

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #59)
> Now that this is confirmed to be resolved in the 15.0.1 candidate build I'm
> flagging for tests. I think we definitely need a manual MozTrap test to
> verify this in pre-release builds. However, I'm also flagging this to see if
> we can automate a Mozmill test or if this can be handled by a mochitest.

I do not see a reason why we would need a moztrap test if this can be automated. As Ehsan pointed out in comment 9 there are unit tests already. If those haven't covered this regression those will have to be enhanced. So preferable we should do: in-testsuite -> in-qa-testsuite -> in-moztrap.

Comment 61

[(no longer active)]

(In reply to comment #60)
> I do not see a reason why we would need a moztrap test if this can be
> automated. As Ehsan pointed out in comment 9 there are unit tests already. If
> those haven't covered this regression those will have to be enhanced. So
> preferable we should do: in-testsuite -> in-qa-testsuite -> in-moztrap.

Our unit tests are never going to be able to test all of the ways in which we make a network request from inside Firefox.  The unit tests specific to the cache are already in the tree, and did not manage to catch this bug.

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Ehsan Akhgari [:ehsan] from comment #61)
> Our unit tests are never going to be able to test all of the ways in which
> we make a network request from inside Firefox.  The unit tests specific to
> the cache are already in the tree, and did not manage to catch this bug.

Ok, so lets figure out what needs we have to get one or more Mozmill tests implemented. Anthony, as we talked about please start a thread in [e-mail] so we do not pollute this bug.

Comment 63

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/410f6b49cea9
https://hg.mozilla.org/releases/mozilla-beta/rev/1588253a38da

Comment 64

[Martijn Wargers (dead)]

I guess private browsing sessions carried out with the buggy Firefox15.0 version are still visible in the cache?
Should users be warned about that?

Comment 65

[(no longer active)]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #64)
> I guess private browsing sessions carried out with the buggy Firefox15.0
> version are still visible in the cache?

Yes.  Unfortunately we can't differentiate them with non-private cache entries, so there is no good way of clearing them.

> Should users be warned about that?

Doesn't hurt to do so, but how would we do that?

Comment 66

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> Steps to Reproduce:
> 1. Open about:cache
> 2. Clear cache through Tools > Clear Recent History
> > Disk cache should report 0KB and have no entries
> 3. Start Private Browsing
> 4. Navigate to www.yahoo.ca and www.amazon.ca
> 5. Open about:cache and click "List cache entries" under Disk Cache
> > Should see a couple cached items, particularly from yahoo and amazon
> 6. Stop private browsing mode
> 7. Open about:cache and click "List cache entries" under Disk Cache
> > Disk cache should report 0KB and have no entries
> 
> Actual Result:
> Disk cache entries from Private Browsing mode are visible in Normal Browsing
> mode.

Ehsan, can you please confirm that those steps would also apply for a Mozmill test? Personally I would like to get rid of the CLR dialog and about:cache by directly using the back-end API. Would that work?

Comment 67

[(no longer active)]

(In reply to comment #66)
> (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> > Steps to Reproduce:
> > 1. Open about:cache
> > 2. Clear cache through Tools > Clear Recent History
> > > Disk cache should report 0KB and have no entries
> > 3. Start Private Browsing
> > 4. Navigate to www.yahoo.ca and www.amazon.ca
> > 5. Open about:cache and click "List cache entries" under Disk Cache
> > > Should see a couple cached items, particularly from yahoo and amazon
> > 6. Stop private browsing mode
> > 7. Open about:cache and click "List cache entries" under Disk Cache
> > > Disk cache should report 0KB and have no entries
> > 
> > Actual Result:
> > Disk cache entries from Private Browsing mode are visible in Normal Browsing
> > mode.
> 
> Ehsan, can you please confirm that those steps would also apply for a Mozmill
> test? Personally I would like to get rid of the CLR dialog and about:cache by
> directly using the back-end API. Would that work?

Yes, that should be fine.

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great, so we will take care of the Mozmill test on bug 789987.

Comment 69

[Jason Duell]

> I guess private browsing sessions carried out with the buggy Firefox15.0
> version are still visible in the cache?

Yes.  The only way to get rid of these would be to also ship another dot-release with a new cache version number, which would cause all users' caches to get dumped.  It's possible to do but a fairly drastic step to take.

OTOH our stats seem to show that something like 10-15% of browser restarts occur with cache that's marked "corrupt" (i.e. didn't close cleanly, so blown away).  I suppose that could be an argument for not shipping a new cache version (PB entries will go away soon enough from crashes), or an argument for doing so (we already blow away caches often, so what's new?).

Comment 70

[(no longer active)]

(In reply to comment #69)
> > I guess private browsing sessions carried out with the buggy Firefox15.0
> > version are still visible in the cache?
> 
> Yes.  The only way to get rid of these would be to also ship another
> dot-release with a new cache version number, which would cause all users'
> caches to get dumped.  It's possible to do but a fairly drastic step to take.
> 
> OTOH our stats seem to show that something like 10-15% of browser restarts
> occur with cache that's marked "corrupt" (i.e. didn't close cleanly, so blown
> away).  I suppose that could be an argument for not shipping a new cache
> version (PB entries will go away soon enough from crashes), or an argument for
> doing so (we already blow away caches often, so what's new?).

Hmm, I didn't know that it's possible to bump the cache version this way..  Do you think it makes sense for us to do this version bump for Firefox 16 though, on the off-chance that there are still users with these types of cache entries?  If yes, then I can file that bug, and then ask you how to bump the cache version.  :-)

Comment 71

[Jason Duell]

Personally I don't think it's worth flushing all firefox users' caches for this.  But I can imagine others disagreeing.  Who gets to make this sort of decision?  Do we have a privacy tsar?

Changing the cache version is just a matter of modifying kCurrentVersion in nsDiskCache.h, FYI...

Comment 72

[Michael Lefevre]

(In reply to Ehsan Akhgari [:ehsan] from comment #70)
> Hmm, I didn't know that it's possible to bump the cache version this way.. 
> Do you think it makes sense for us to do this version bump for Firefox 16
> though, on the off-chance that there are still users with these types of
> cache entries?  If yes, then I can file that bug, and then ask you how to
> bump the cache version.  :-)

If that is going to happen, it might make sense to do it with Firefox 17, or backport bug 709297 (where there is extra work being done to avoid blowing away entire caches). Wouldn't make much sense to blow away the cache with Firefox 16, let it grow back to 1GB and then spend time trying to reduce it gradually starting from Firefox 17...

Comment 73

[Jason Duell]

> If [we're going to blow away cache for PB leftover issues] it might make sense to
> do it with Firefox 17, or backport bug 709297... Wouldn't make much sense to blow
> away the cache with Firefox 16, let it grow back to 1GB and then spend time trying 
> to reduce it gradually starting from Firefox 17...

That's a very good point (bug 709297 reduces the cache size--but only after crashes--to 320 MB going forward instead of 1 GB).   OTOH a backport of an unconditional "blow away all users' caches and set max size for everyone to 320 MB) patch would be much easier to write than the kludgery we did in bug 709297, and would let us rip that code out.

But I'd still lean toward not nuking all caches for these PB leftover, which (given our current cache crash stats) seem likely to exist on very few machines by the time FF17 ships.   But if privacy is a big enough deal here, the above would be my vote for a plan.

Comment 74

[Alex Keybl [:akeybl]]

(In reply to Jason Duell (:jduell) from comment #71)
> Personally I don't think it's worth flushing all firefox users' caches for
> this.  But I can imagine others disagreeing.  Who gets to make this sort of
> decision?  Do we have a privacy tsar?
> 
> Changing the cache version is just a matter of modifying kCurrentVersion in
> nsDiskCache.h, FYI...

I also don't feel strongly about this - if the user is affected & concerned, they can clear the cache on their own.

Comment 75

[u279076]

I've set up a manual testcase in MozTrap until we have this fully automated:
https://moztrap.mozilla.org/manage/cases/_detail/6614/

I'll verify the fix on the branches tomorrow.

Comment 76

[u279076]

Verified fixed using the Moztrap testcase with:
 * Firefox 15.0.1
 * Firefox 16.0b3
 * Firefox 17.0a2 2012-09-12

I can still reproduce this with the following build: 
 * Firefox 18.0a1 2012-09-12 (assumed expected based on status flag)

Comment 77

[(no longer active)]

So I just confirmed that b2g is going to ship based on Gecko 18, which means that we will not have the option of fixing this by backing out bug 722845 in Firefox 18.  So we need to fix this as soon as possible.

Josh, we should chat tomorrow to see how I can help here.

Comment 78

[(no longer active)]

Bug 792582 is on inbound now, and bug 789987 is about a test suite.  So I guess we can finally say that this is fixed now for 18! \o/

Comment 79

[u279076]

I forgot to verify this for Firefox 18 earlier. It is now verified against Firefox 18.0a2 2012-10-16.

Comment 80

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Anthony, this is now in the mozmill testsuite for each supported branch. Feel free to disable the appropriate Moztrap test.

Comment 81

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #80)
> Anthony, this is now in the mozmill testsuite for each supported branch.
> Feel free to disable the appropriate Moztrap test.

Thank you very much, Henrik.

Comment 82

[VanillaMozilla]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #79)
> I forgot to verify this for Firefox 18 earlier. It is now verified against
> Firefox 18.0a2 2012-10-16.

Actually it is NOT fixed in version 18.  Fail the steps to reproduce in comment 18.  Should this be reopened?

Comment 83

[VanillaMozilla]

Sorry, that's comment 19.

Comment 84

[(no longer active)]

Can you please explain the behavior that you're seeing a bit more clearly?  Thanks!

Comment 85

[VanillaMozilla]

Well, what I do see in disk cache is data from digicert and verisign, etc.  If that counts, I'll provide more details.

And my face is very red.  I saw lots of images stored in private mode, but I'm thinking I may have inadvertently checked the memory cache.  I can't duplicate it.  Sorry about that.

Comment 86

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, I can see the same. So what I did is:

1. Clear caches and check about:cache that those are empty
2. Start private browsing mode
3. Load https://www.amazon.de
4. Stop private browsing mode
5. Reload about:cache

After step 5 we have the following items listed (dates removed):

anon&id=50fd22d5&uri=http://ocsp.verisign.com/ 	1596 bytes 	1 	anon&id=50fd22d3&uri=http://ocsp.usertrust.com/ 471 bytes 	2 	anon&id=50fd22d2&uri=http://ocsp.comodoca.com/ 	471 bytes 	1 	anon&id=50fd22d1&uri=http://ocsp.verisign.com/ 	1596 bytes 	1

Comment 87

[Josh Matthews [:jdm]]

Those don't expose any private information about the user's browsing history, so they're fine.