465756 - Ignore browser.helperApps.deleteTempFileOnExit not being set in private browsing

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[(no longer active)]

Bug 460609 enabled deletion of temporary help application files when exiting the private browsing mode if the browser.helperApps.deleteTempFileOnExit pref is set.  If it's not (which is the default in Mac OS X for example), then these files won't get deleted.  We need to enhance this logic to make sure this works regardless of the browser.helperApps.deleteTempFileOnExit pref being set.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Trivial patch.  Requesting r from sdwilsh on downloadmgr part, and r/sr from bz on urihandler part.

Comment 2

[(no longer active)]

Attachment: Patch (v1.1)

Boris: to my great shame, the previous patch didn't even compile! :/

Sorry for the inconvenience, here's a patch which actually compiles.

/me hides in embarrassment...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 348999 [details] [diff] [review]
Patch (v1.1)

Details, details.  ;)

Comment 4

[Shawn Wilsher :sdwilsh]

What's the likelihood of getting a unit test for this?  It doesn't seem that hard to do (and you don't even need to use the http server if you use a data uri).

Comment 5

[(no longer active)]

(In reply to comment #4)
> What's the likelihood of getting a unit test for this?  It doesn't seem that
> hard to do (and you don't even need to use the http server if you use a data
> uri).

I'd write one; I just don't know how to trigger this code path in a unit test without opening an external helper application.  Note that this behavior is only for the downloads which are supposed to open up an external helper application, not for usual downloads.

Comment 6

[(no longer active)]

Attachment: Patch (v1.2)

Feel free to slap me!  I had made the exact same mistake in the download manager part!!!

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 349139 [details] [diff] [review]
Patch (v1.2)

>+  if (deleteTempFileOnExit ||
>+      nsDownloadManager::gDownloadManagerService->mInPrivateBrowsing) {
nsDownload has mDownloadManager (should reconsider that, but use it here please)

r=sdwilsh with that change.  Don't worry about the test, since I can't think of a way to automate that.

Comment 8

[(no longer active)]

(In reply to comment #7)
> nsDownload has mDownloadManager (should reconsider that, but use it here
> please)

Sure, I'll do that here.  Do you want me to file a new bug for nsDownload to use the global singleton pointer to nsDownloadManager instead of keeping a member variable?

Comment 9

[(no longer active)]

Comment on attachment 349139 [details] [diff] [review]
Patch (v1.2)

Would be nice to take for Beta 2 as a complement to functionality implemented in bug 460609.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Sure, I'll do that here.  Do you want me to file a new bug for nsDownload to
> use the global singleton pointer to nsDownloadManager instead of keeping a
> member variable?
You can file it, but I'm not sure we want to do it.

(In reply to comment #9)
> (From update of attachment 349139 [details] [diff] [review])
> Would be nice to take for Beta 2 as a complement to functionality implemented
> in bug 460609.
The tree is currently closed now since we want to ship b2.  I suspect this won't get approval.

Comment 11

[(no longer active)]

(In reply to comment #10)
> You can file it, but I'm not sure we want to do it.

Why?

> (In reply to comment #9)
> The tree is currently closed now since we want to ship b2.  I suspect this
> won't get approval.

OK.  In case it gets minused, I'll request approval on the next milestone.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> Why?
because mDownloadManager looks nicer that nsDownloadManager::gDownloadManager whenever you have to use it.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349139 [details] [diff] [review]
Patch (v1.2)

Shawn is a good predictor of things :)

Comment 14

[Shawn Wilsher :sdwilsh]

Per blocking rankings, this needs to be fixed by release, but doesn't need to
block the proposed beta 3.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349139 [details] [diff] [review]
Patch (v1.2)

(Blockers don't need explicit approval at this time, removing flag)

Comment 16

[(no longer active)]

Attachment: mq patch for check-in

Comment 17

[Shawn Wilsher :sdwilsh]

Please make sure you qrefresh with -U in the future.  I almost pushed this as me because there was no username stored in the patchfile.

http://hg.mozilla.org/mozilla-central/rev/f8ade83cc5fb

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081204 Shiretoko/3.1b3pre ID:20081204020603

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Added as Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=7440

Comment 20

[(no longer active)]

This test does not cover this bug, since it explicitly mentions the need for setting the browser.helperApps.deleteTempFileOnExit pref.  In fact, the test case should be modified to make sure that this behavior is observed with the value of browser.helperApps.deleteTempFileOnExit set both true and false.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, thanks for mentioning that! I've updated the testcase.

Comment 22

[(no longer active)]

There is one more problem with the test case description.  Nothing of what was done in this bug is Mac-specific, so you shouldn't mention OS X in the description at all.  Suggestion: s/on OS X //.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The ExternalHelperAppService always deletes files on other operating systems as OS X for now as you can see here:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2186

I've never told that it should be all/all. So I'm a bit confused now. Do we really have to obey this pref for Windows/Linux?

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh I see. When the pref is set to false then temporary files shouldn't also be deleted on the other OS.

Comment 25

[(no longer active)]

Henrik corrected the testcase description on Litmus: <https://litmus.mozilla.org/show_test.cgi?id=7440>.

Thanks Henrik for tolerating my nits.  :-)

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

These nits are more than welcome. I was even able to learn from. Thanks.