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)
Tracking
()
People
(Reporter: iwantraybaby, Assigned: ehsan.akhgari)
References
Details
(Keywords: regression, reproducible)
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
jduell.mcbugs
:
review+
akeybl
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jdm
:
review+
ehsan.akhgari
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Component: Untriaged → Private Browsing
Comment 1•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Crap. I can reproduce this. Thank you very much for this report! This is almost certainly a regression from bug 722845.
Comment 6•12 years ago
|
||
I'm assuming subsequent versions are affected as well at this point.
status-firefox15:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
status-firefox18:
--- → affected
tracking-firefox15:
--- → ?
Keywords: regression
Comment 7•12 years ago
|
||
Curious; I see the memory device get cleared ~5 seconds after leaving PB mode in current nightlies.
Comment 8•12 years ago
|
||
It would be worth finding out whether Aurora and Beta see this problem too.
Keywords: qawanted
Updated•12 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Version: 14 Branch → 15 Branch
Updated•12 years ago
|
Updated•12 years ago
|
Assignee: nobody → josh
Assignee | ||
Comment 9•12 years ago
|
||
Yeah, totally reproducible. I wonder why our unit tests did not catch this?
Comment 10•12 years ago
|
||
If anybody can provide info on whether Fennec is similarly affected, it'd be much appreciated.
Assignee | ||
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Assignee | ||
Comment 13•12 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: josh → ehsan
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
tracking-firefox18:
--- → +
Assignee | ||
Comment 14•12 years ago
|
||
[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?
Assignee | ||
Comment 15•12 years ago
|
||
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•12 years ago
|
||
Assigning myself as QA contact. Please let me know if there is anything I can do to help here.
QA Contact: anthony.s.hughes
Comment 17•12 years ago
|
||
Can someone please clarify the STR and expected results without the CacheViewer add-on installed?
Assignee | ||
Comment 18•12 years ago
|
||
(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•12 years ago
|
||
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
Comment 20•12 years ago
|
||
Also reproduces with Firefox 15.0, Firefox 17.0a2 2012-09-04, and Firefox 18.0a1 2012-09-04.
Assignee | ||
Comment 21•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Keywords: reproducible
Assignee | ||
Comment 22•12 years ago
|
||
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)
Comment 23•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
(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!
Assignee | ||
Comment 27•12 years ago
|
||
(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.
Assignee | ||
Comment 28•12 years ago
|
||
This patch fixes what I was talking about in comment 21.
Attachment #658182 -
Flags: review?(josh)
Assignee | ||
Comment 29•12 years ago
|
||
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.
Assignee | ||
Comment 30•12 years ago
|
||
Comment on attachment 658110 [details] [diff] [review]
Backout patch
Try server push: https://tbpl.mozilla.org/?tree=Try&rev=aacc99e888c8
Comment 31•12 years ago
|
||
(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•12 years ago
|
||
Backout patch for mozilla-beta. I had to make some changes, so I'll put together a patch for review that highlights those.
Updated•12 years ago
|
Attachment #658110 -
Attachment is obsolete: true
Attachment #658110 -
Flags: review?(jduell.mcbugs)
Attachment #658110 -
Flags: approval-mozilla-release?
Comment 33•12 years ago
|
||
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?
Updated•12 years ago
|
Attachment #658202 -
Attachment description: Back out bug 722845 and bug 748890. → BETA: Back out bug 722845 and bug 748890.
Comment 34•12 years ago
|
||
Try push at https://tbpl.mozilla.org/?tree=Try&rev=230305ca4637.
Comment 35•12 years ago
|
||
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+
Comment 37•12 years ago
|
||
(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•12 years ago
|
||
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?
Comment 39•12 years ago
|
||
(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•12 years ago
|
||
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+
Comment 41•12 years ago
|
||
Aurora try push: https://tbpl.mozilla.org/?tree=Try&rev=4f75eeae2fe5
Comment 42•12 years ago
|
||
[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 43•12 years ago
|
||
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)
Assignee | ||
Comment 44•12 years ago
|
||
(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?
Assignee | ||
Comment 45•12 years ago
|
||
Updated•12 years ago
|
Attachment #658202 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•12 years ago
|
Attachment #658281 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 46•12 years ago
|
||
Assignee | ||
Comment 47•12 years ago
|
||
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?
Assignee | ||
Comment 48•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #658202 -
Attachment is obsolete: true
Comment 51•12 years ago
|
||
Updated•12 years ago
|
Attachment #658281 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #658487 -
Attachment description: Back out bug 722845. → Aurora: Back out bug 722845.
Updated•12 years ago
|
Attachment #658182 -
Flags: review?(josh) → review+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 52•12 years ago
|
||
Comment on attachment 658182 [details] [diff] [review]
Patch (v1)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dfafdd2f631
Attachment #658182 -
Flags: checkin+
Comment 53•12 years ago
|
||
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.
Assignee | ||
Comment 54•12 years ago
|
||
Josh, are the aurora and beta patches here ready for landing?
Comment 55•12 years ago
|
||
Yes.
Comment 56•12 years ago
|
||
Comment 57•12 years ago
|
||
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?
Updated•12 years ago
|
Attachment #658481 -
Flags: review?(ehsan)
Attachment #658481 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 58•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Attachment #658487 -
Flags: review?(ehsan) → review+
Comment 59•12 years ago
|
||
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)
Comment 60•12 years ago
|
||
(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.
Assignee | ||
Comment 61•12 years ago
|
||
(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•12 years ago
|
||
(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.
Updated•12 years ago
|
Flags: in-qa-testsuite? → in-qa-testsuite?(hskupin)
Updated•12 years ago
|
Attachment #658481 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•12 years ago
|
Attachment #658487 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 63•12 years ago
|
||
Comment 64•12 years ago
|
||
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?
Assignee | ||
Comment 65•12 years ago
|
||
(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•12 years ago
|
||
(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?
Assignee | ||
Comment 67•12 years ago
|
||
(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•12 years ago
|
||
Great, so we will take care of the Mozmill test on bug 789987.
Depends on: 789987
Comment 69•12 years ago
|
||
> 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?).
Assignee | ||
Comment 70•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
> 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•12 years ago
|
||
(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•12 years ago
|
||
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+
Comment 76•12 years ago
|
||
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)
Updated•12 years ago
|
Alias: pbchannelfail
Assignee | ||
Comment 77•12 years ago
|
||
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.
Assignee | ||
Comment 78•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 79•12 years ago
|
||
I forgot to verify this for Firefox 18 earlier. It is now verified against Firefox 18.0a2 2012-10-16.
Status: RESOLVED → VERIFIED
Comment 80•12 years ago
|
||
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+
Comment 81•12 years ago
|
||
(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?
Comment 82•12 years ago
|
||
(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•12 years ago
|
||
Sorry, that's comment 19.
Assignee | ||
Comment 84•12 years ago
|
||
Can you please explain the behavior that you're seeing a bit more clearly? Thanks!
Comment 85•12 years ago
|
||
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•12 years ago
|
||
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
Updated•12 years ago
|
Flags: needinfo?(ehsan)
Comment 87•12 years ago
|
||
Those don't expose any private information about the user's browsing history, so they're fine.
Flags: needinfo?(ehsan)
You need to log in
before you can comment on or make changes to this bug.
Description
•