Closed Bug 787743 (pbchannelfail) Opened 12 years ago Closed 12 years ago

Private Browsing mode not working in Firefox 15.0

Categories

(Core :: Networking: HTTP, defect)

15 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox15 + verified
firefox16 + verified
firefox17 + verified
firefox18 + verified

People

(Reporter: iwantraybaby, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression, reproducible)

Attachments

(4 files, 2 obsolete files)

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.
Severity: normal → critical
Component: Untriaged → Private Browsing
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?
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.
Of course, I tried loading Firefox 15 and found cached everything - Private Browsing included.
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.
Crap. I can reproduce this. Thank you very much for this report! This is almost certainly a regression from bug 722845.
Blocks: 722845
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm assuming subsequent versions are affected as well at this point.
Curious; I see the memory device get cleared ~5 seconds after leaving PB mode in current nightlies.
It would be worth finding out whether Aurora and Beta see this problem too.
Keywords: qawanted
OS: Windows XP → All
Hardware: x86 → All
Version: 14 Branch → 15 Branch
Assignee: nobody → josh
Yeah, totally reproducible. I wonder why our unit tests did not catch this?
If anybody can provide info on whether Fennec is similarly affected, it'd be much appreciated.
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.
(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.
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.
Assignee: josh → ehsan
Keywords: qawanted
Attached patch Backout patch (deleted) — Splinter Review
[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.
Attachment #658110 - Flags: approval-mozilla-release?
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}}
Assigning myself as QA contact. Please let me know if there is anything I can do to help here.
QA Contact: anthony.s.hughes
Can someone please clarify the STR and expected results without the CacheViewer add-on installed?
(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.
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.
Keywords: reproducible
Also reproduces with Firefox 15.0, Firefox 17.0a2 2012-09-04, and Firefox 18.0a1 2012-09-04.
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.
Component: Private Browsing → Networking: HTTP
Keywords: reproducible
Product: Firefox → Core
Keywords: reproducible
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.
Attachment #658110 - Flags: review?(jduell.mcbugs)
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!
(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.
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.
(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!
(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.
Attached patch Patch (v1) (deleted) — Splinter Review
This patch fixes what I was talking about in comment 21.
Attachment #658182 - Flags: review?(josh)
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.
Depends on: 741059
(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.
Attached patch BETA: Back out bug 722845 and bug 748890. (obsolete) (deleted) — Splinter Review
Backout patch for mozilla-beta. I had to make some changes, so I'll put together a patch for review that highlights those.
Attachment #658110 - Attachment is obsolete: true
Attachment #658110 - Flags: review?(jduell.mcbugs)
Attachment #658110 - Flags: approval-mozilla-release?
Comment on attachment 658110 [details] [diff] [review] Backout patch Sorry, bugzilla flail on my part. See the earlier approval request for Ehsan's comments.
Attachment #658110 - Attachment is obsolete: false
Attachment #658110 - Flags: review?(jduell.mcbugs)
Attachment #658110 - Flags: approval-mozilla-release?
Attachment #658202 - Attachment description: Back out bug 722845 and bug 748890. → BETA: Back out bug 722845 and bug 748890.
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.
Attachment #658110 - Flags: review?(jduell.mcbugs) → review+
(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.
Depends on: 788275
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.
Attachment #658202 - Flags: review?(jduell.mcbugs)
Attachment #658202 - Flags: approval-mozilla-beta?
(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 on attachment 658110 [details] [diff] [review] Backout patch [Triage Comment] Please land on mozilla-release for a go to build tomorrow morning PT. Thanks!
Attachment #658110 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attached patch AURORA: Back out bug 722845. (obsolete) (deleted) — Splinter Review
[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.
Attachment #658281 - Flags: approval-mozilla-aurora?
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.
Attachment #658202 - Flags: review?(jduell.mcbugs)
(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?
Attachment #658202 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #658281 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
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.
Attached patch Beta backout (deleted) — Splinter Review
Attachment #658202 - Attachment is obsolete: true
Attachment #658281 - Attachment is obsolete: true
Attachment #658487 - Attachment description: Back out bug 722845. → Aurora: Back out bug 722845.
Attachment #658182 - Flags: review?(josh) → review+
Whiteboard: [leave open]
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.
Josh, are the aurora and beta patches here ready for landing?
Yes.
Depends on: 788923
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?
Attachment #658487 - Flags: review?(ehsan)
Attachment #658487 - Flags: approval-mozilla-aurora?
Attachment #658481 - Flags: review?(ehsan)
Attachment #658481 - Flags: approval-mozilla-beta?
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.
Attachment #658481 - Flags: review?(ehsan) → review+
Attachment #658487 - Flags: review?(ehsan) → review+
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.
Flags: in-testsuite?
Flags: in-qa-testsuite?
Flags: in-moztrap?(anthony.s.hughes)
(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.
(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.
(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 dev-automation@mozilla.org so we do not pollute this bug.
Flags: in-qa-testsuite? → in-qa-testsuite?(hskupin)
Blocks: 787759
Attachment #658481 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #658487 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
(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?
(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?
(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.
Great, so we will take care of the Mozmill test on bug 789987.
Depends on: 789987
> 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?).
(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. :-)
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...
Depends on: 790181
No longer blocks: 788317
(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...
> 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.
(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.
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.
Flags: in-moztrap?(anthony.s.hughes) → in-moztrap+
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)
Depends on: 791361
Depends on: 791367
Depends on: 791369
Depends on: 791372
Depends on: 791378
Depends on: 791394
Depends on: 791391
Alias: pbchannelfail
Depends on: 792513
Depends on: 792517
Depends on: 792524
Depends on: 792528
Depends on: 792542
Depends on: 792582
Blocks: mobilepb
Depends on: 794602
Depends on: 795015
Depends on: 795571
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.
No longer depends on: 792513
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/
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Whiteboard: [leave open]
Depends on: 800952
Depends on: 801049
Depends on: 722978
No longer depends on: 722978
I forgot to verify this for Firefox 18 earlier. It is now verified against Firefox 18.0a2 2012-10-16.
Status: RESOLVED → VERIFIED
Anthony, this is now in the mozmill testsuite for each supported branch. Feel free to disable the appropriate Moztrap test.
Flags: in-qa-testsuite?(hskupin) → in-qa-testsuite+
(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.
Flags: in-testsuite?
(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?
Sorry, that's comment 19.
Can you please explain the behavior that you're seeing a bit more clearly? Thanks!
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.
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
Flags: needinfo?(ehsan)
Those don't expose any private information about the user's browsing history, so they're fine.
Flags: needinfo?(ehsan)
Blocks: 972347
No longer blocks: 972347
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: