Closed
Bug 541739
Opened 15 years ago
Closed 15 years ago
Flaws in download cancellation checks when switching the private browsing mode
Categories
(Firefox :: Private Browsing, defect)
Tracking
()
RESOLVED
FIXED
Firefox 3.7a5
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
(Keywords: dataloss)
Attachments
(1 file)
(deleted),
patch
|
sdwilsh
:
review+
christian
:
approval1.9.2.7-
|
Details | Diff | Splinter Review |
I discovered two major flaws in the private browsing implementation inside the download manager module. 1. We should prompt for all active downloads when the user is leaving the private browsing mode, not just non-resumable ones. Because once the user leaves the private browsing mode, they will no longer be able to access their downloads inside the PB mode. 2. We fail to register the NS_PRIVATE_BROWSING_REQUEST_TOPIC observer, which means that the prompting code has never been in effect, and so far the download manager had never prompted any user while switching the PB mode. I have a patch which fixes both these issues, and also adds a test to make sure that this behavior would not regress in the future.
Comment 2•15 years ago
|
||
Won't take this in 3.5, will take a reviewed/tested/baked patch in a future 3.6 release but not "blocking".
Comment 3•15 years ago
|
||
Comment on attachment 423192 [details] [diff] [review] Patch (v1) nit: (void) the unchecked AddObserver call please r=sdwilsh
Attachment #423192 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 4•15 years ago
|
||
Landed with the nit in comment 3 on trunk: <http://hg.mozilla.org/mozilla-central/rev/4d7bde383df5>.
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Assignee | ||
Updated•15 years ago
|
Attachment #423192 -
Flags: approval1.9.2.1?
Comment 5•15 years ago
|
||
Backed out due to test failures, please stick around in #developers for a time after landing to deal with any problems that occur. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264558062.1264560235.20213.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264557643.1264561123.30692.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•15 years ago
|
Attachment #423192 -
Flags: approval1.9.2.1?
Comment 6•15 years ago
|
||
Also, I presume, http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264559611.1264561277.32595.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1264560178.1264561611.4192.gz
Assignee | ||
Comment 8•15 years ago
|
||
So, the test actually passes, but asserts after being finished: TEST-INFO | (xpcshell/head.js) | test 1 pending TEST-INFO | (xpcshell/head.js) | test 2 pending *** Throwing trying to get TmpD *** Throwing trying to get ProfD *** Throwing trying to get UHist TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [run_test : 154] 0 == 0 pldhash: for the table at address 0x582e8c0, the given entrySize of 48 probably favors chaining over double hashing. *** Throwing trying to get cachePDir *** Throwing trying to get ProfLD *** Throwing trying to get cachePDir TEST-INFO | (xpcshell/head.js) | test 2 finished TEST-INFO | (xpcshell/head.js) | running event loop TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 164] false == false TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 171] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 172] false == false TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 175] 0 != 4 pldhash: for the table at address 0x46dbb78, the given entrySize of 52 probably favors chaining over double hashing. pldhash: for the table at address 0x46de180, the given entrySize of 80 definitely favors chaining over double hashing. TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 182] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 183] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 186] 3 == 3 TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 190] false == false *** Throwing trying to get UMimTyp *** Throwing trying to get UMimTyp TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 202] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 206] false == false TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 207] true == true *** Throwing trying to get UMimTyp TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 219] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 226] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 227] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 230] 0 != 4 TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 202] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 237] true == true TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 238] false == false TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [anonymous : 241] 4 == 4 TEST-PASS | /Users/ehsanakhgari/moz/obj-ff-dbg/_tests/xpcshell/test_dm/unit/test_privatebrowsing_cancel.js | [finishTest : 118] 0 == 0 TEST-INFO | (xpcshell/head.js) | test 1 finished TEST-INFO | (xpcshell/head.js) | exiting test TEST-PASS | (xpcshell/head.js) | 21 (+ 0) check(s) passed ###!!! ASSERTION: ### mem cache leaking entries?: 'mEntryCount == 0', file /Users/ehsanakhgari/moz/src/netwerk/cache/src/nsMemoryCacheDevice.cpp, line 130 nsMemoryCacheDevice::Shutdown()+0x00000261 [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/XUL +0x001DA8B3] nsMemoryCacheDevice::~nsMemoryCacheDevice()+0x00000026 [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/XUL +0x001DA8F4] nsCacheService::Shutdown()+0x000000D4 [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/XUL +0x001D5B6E] nsCacheProfilePrefObserver::Observe(nsISupports*, char const*, unsigned short const*)+0x000000CB [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/XUL +0x001D6E5B] nsObserverList::NotifyObservers(nsISupports*, char const*, unsigned short const*)+0x00000061 [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/XUL +0x01346F5B] nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*)+0x0000014C [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/XUL +0x01347FC8] mozilla::ShutdownXPCOM(nsIServiceManager*)+0x000001ED [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/XUL +0x01337172] NS_ShutdownXPCOM_P+0x00000011 [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/XUL +0x01337654] NS_ShutdownXPCOM+0x00000011 [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/libxpcom.dylib +0x000019FF] main+0x00000CF5 [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/xpcshell +0x0000580A] start+0x00000036 [/Users/ehsanakhgari/moz/obj-ff-dbg/dist/bin/xpcshell +0x000019E6] Shawn, do you have any idea what might be going on here?
Comment 9•15 years ago
|
||
(In reply to comment #8) > Shawn, do you have any idea what might be going on here? I know zero about that code.
Assignee | ||
Comment 11•15 years ago
|
||
I found out that the assertion in comment 8 was caused by a bug in the cache code. I filed that as bug 567680 with a patch. Once that patch lands, this one can also reland without modification.
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 12•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5a0ac42539cd
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.7a1 → Firefox 3.7a5
Assignee | ||
Comment 13•15 years ago
|
||
Comment on attachment 423192 [details] [diff] [review] Patch (v1) The unit test for this bug requires the patch in bug 567680 as well (which should be pretty safe.) I'll apply that patch for approval as well.
Attachment #423192 -
Flags: approval1.9.2.5?
Assignee | ||
Comment 14•15 years ago
|
||
This seemed to have turned three browser-chrome boxes orange, so I backed it out again: http://hg.mozilla.org/mozilla-central/rev/b89301b59521 The failure logs: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274727181.1274727881.26342.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274727171.1274728361.28455.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274727330.1274728455.28851.gz I'll investigate this tomorrow.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•15 years ago
|
||
FWIW, I've verified that my patch in bug 567821 solves the unit test failures. I'm currently waiting for Gavin's review of that patch to land this bug.
Assignee | ||
Updated•15 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 16•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5aa26fc3f3f5
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 17•14 years ago
|
||
Comment on attachment 423192 [details] [diff] [review] Patch (v1) We will not be taking this for 3.6.6. Moving approval request forward. If you disagree, send me an email.
Attachment #423192 -
Flags: approval1.9.2.7?
Attachment #423192 -
Flags: approval1.9.2.6-
Attachment #423192 -
Flags: approval1.9.2.5?
Updated•14 years ago
|
blocking2.0: ? → final+
Assignee | ||
Comment 21•14 years ago
|
||
Christian, what happened to the approval request on this bug? Did it get lost somewhere along the way?
Updated•14 years ago
|
Attachment #423192 -
Flags: approval1.9.2.9?
You need to log in
before you can comment on or make changes to this bug.
Description
•