(no longer active) Assignee
	Description • 15 years ago

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.

	Daniel Veditz [:dveditz]
	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".

	Shawn Wilsher :sdwilsh
	Comment 3 • 15 years ago

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

nit: (void) the unchecked AddObserver call please

r=sdwilsh

	(no longer active) Assignee
	Comment 4 • 15 years ago

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

	Dave Townsend [:mossop]
	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

	Phil Ringnalda (:philor)
	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

	(no longer active) 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?

	Shawn Wilsher :sdwilsh
	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.

	(no longer active) 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.

	(no longer active) Assignee
	Comment 12 • 15 years ago

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

	(no longer active) 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.

	(no longer active) 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.

	(no longer active) 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.

	(no longer active) Assignee
	Comment 16 • 15 years ago

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

	LegNeato
	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.

	(no longer active) Assignee
	Comment 21 • 14 years ago

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