Closed Bug 465756 Opened 12 years ago Closed 12 years ago

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

Categories

(Toolkit :: Downloads API, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 2 obsolete files)

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.
Flags: blocking1.9.1?
Attached patch Patch (v1) (obsolete) — Splinter Review
Trivial patch.  Requesting r from sdwilsh on downloadmgr part, and r/sr from bz on urihandler part.
Attachment #348993 - Flags: superreview?(bzbarsky)
Attachment #348993 - Flags: review?(sdwilsh)
Attachment #348993 - Flags: review?(bzbarsky)
Attachment #348993 - Flags: superreview?(bzbarsky)
Attachment #348993 - Flags: superreview+
Attachment #348993 - Flags: review?(bzbarsky)
Attachment #348993 - Flags: review+
Attached patch Patch (v1.1) (obsolete) — Splinter Review
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...
Attachment #348993 - Attachment is obsolete: true
Attachment #348999 - Flags: superreview?(bzbarsky)
Attachment #348999 - Flags: review?(sdwilsh)
Attachment #348999 - Flags: review?(bzbarsky)
Attachment #348993 - Flags: review?(sdwilsh)
Flags: in-litmus?
Comment on attachment 348999 [details] [diff] [review]
Patch (v1.1)

Details, details.  ;)
Attachment #348999 - Flags: superreview?(bzbarsky)
Attachment #348999 - Flags: superreview+
Attachment #348999 - Flags: review?(bzbarsky)
Attachment #348999 - Flags: review+
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).
(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.
Attached patch Patch (v1.2)Splinter Review
Feel free to slap me!  I had made the exact same mistake in the download manager part!!!
Attachment #348999 - Attachment is obsolete: true
Attachment #349139 - Flags: review?(sdwilsh)
Attachment #348999 - Flags: review?(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.
Attachment #349139 - Flags: review?(sdwilsh) → review+
(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 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.
Attachment #349139 - Flags: approval1.9.1b2?
(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.
(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.
Target Milestone: mozilla1.9.1b1 → mozilla1.9.1b2
(In reply to comment #11)
> Why?
because mDownloadManager looks nicer that nsDownloadManager::gDownloadManager whenever you have to use it.
Attachment #349139 - Flags: approval1.9.1b2?
Attachment #349139 - Flags: approval1.9.1b2-
Attachment #349139 - Flags: approval1.9.1?
Comment on attachment 349139 [details] [diff] [review]
Patch (v1.2)

Shawn is a good predictor of things :)
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: mozilla1.9.1b2 → mozilla1.9.1
Attachment #349139 - Flags: approval1.9.1?
Per blocking rankings, this needs to be fixed by release, but doesn't need to
block the proposed beta 3.
Priority: -- → P2
Whiteboard: [has patch][has review][can land]
Attachment #349139 - Flags: approval1.9.1?
Comment on attachment 349139 [details] [diff] [review]
Patch (v1.2)

(Blockers don't need explicit approval at this time, removing flag)
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
Whiteboard: [has patch][has review][can land]
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: fixed1.9.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
Status: RESOLVED → VERIFIED
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.
Flags: in-litmus+ → in-litmus?
Ehsan, thanks for mentioning that! I've updated the testcase.
Flags: in-litmus? → in-litmus+
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 //.
Flags: in-litmus+ → in-litmus?
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?
Oh I see. When the pref is set to false then temporary files shouldn't also be deleted on the other OS.
Henrik corrected the testcase description on Litmus: <https://litmus.mozilla.org/show_test.cgi?id=7440>.

Thanks Henrik for tolerating my nits.  :-)
Flags: in-litmus? → in-litmus+
These nits are more than welcome. I was even able to learn from. Thanks.
You need to log in before you can comment on or make changes to this bug.