541739 - Flaws in download cancellation checks when switching the private browsing mode

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[(no longer active)]

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 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Daniel Veditz [:dveditz]]

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

[Shawn Wilsher :sdwilsh]

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

nit: (void) the unchecked AddObserver call please

r=sdwilsh

Comment 4

[(no longer active)]

Landed with the nit in comment 3 on trunk: <http://hg.mozilla.org/mozilla-central/rev/4d7bde383df5>.

Comment 5

[Dave Townsend [:mossop]]

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

Comment 6

[Phil Ringnalda (:philor)]

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

Comment 8

[(no longer active)]

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

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Shawn, do you have any idea what might be going on here?
I know zero about that code.

Comment 11

[(no longer active)]

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.

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/5a0ac42539cd

Comment 13

[(no longer active)]

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.

Comment 14

[(no longer active)]

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.

Comment 15

[(no longer active)]

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.

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/5aa26fc3f3f5

Comment 17

[LegNeato]

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.

Comment 21

[(no longer active)]

Christian, what happened to the approval request on this bug?  Did it get lost somewhere along the way?