396701 - clicking on .exe file shows wrong download dialog

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[David J. Lennon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007091804 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007091804 Minefield/3.0a8pre

If you download a file but select "open this file with: exefile" without saving the file to disk, an error occures upon completion telling you to "try saving the file to disk first".

Reproducible: Always

Steps to Reproduce:
1. Select file to download on Interweb
2. Don't save the file, but "open the file with: exefile"
3. Wait for file to complete and observe error.
Actual Results:  
Error message asking to download the file and save it instead of opening it directly with exefile.

Expected Results:  
Opened the file with the appropriate program, i.e WinRAR, Explorer, EXE.

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007091916 Minefield/3.0a8pre ID:2007091916

I am seeing this too. It seems that the behaviour wrt downloading .exe files has changed.

On branch, if you left-click a .exe file, you get a dialog that reads:
     "Opening firefox-installer.exe"
     You have chosen to open firefox-installer.exe
     which is a: Application
     from: http://ftp.mozilla.org
     Would you like to save this file?
     [Save File] [Cancel]

So on branch, the only option is to save the file, or cancel the download.

However, on trunk builds, you get:
     "Opening firefox-installer.exe"
     firefox-installer.exe
     which is a: Application
     from: http://ftp.mozilla.org
     What should minefield do with this file?
     (o) Open With [ exefile (default) ]
     (o) Save File

Choosing Save File behaves in the same was as branch (The file is saved to your default DL dir, unless you've told firefox to 'always ask me where to save files', in which case you're shown the filepicker dialog)

But now there is also the "Open With [ exefile (defualt) ] dropdown. If you choose this option, then you get an error dialog saying:
     C:\Path\To\File\firefox-installer.exe could not be opened, because
     an unknown error occured.
     Try saving to disk first and then opening the file
     [OK]

(From the Open With dropdown, it also looks like you are able to try and open the .exe file with another application - which doesn't make a whole lot of sense.)

So now, for some reason, the special download dialog that branch uses when dealing with .exe files has been replaced with the normal download dialog, which also contains a silly option to apparently run the .exe file (even tho it doesn't work). I'm not sure if this was intended, but I assume not so I add regression kw.

Comment 2

[Steve England [:stevee]]

Works (left click on .exe gives [Save File] [Cancel] dialog:
- 20070916_0739_firefox-3.0a8pre.en-US.win32

Broken (left click on .exe gives 'What should minefield do with this file?' dialog:
- 20070916_0754_firefox-3.0a8pre.en-US.win32

Checkins to module PhoenixTinderbox between 2007-09-16 07:39 and 2007-09-16 07:53 :

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1189953540&maxdate=1189954439

So due to bug 395207 or bug 395308.

Comment 3

[Steve England [:stevee]]

David, do you see this problem with all filetypes (eg: zip, etc) or just with .exe files. Because for me, choosing to open .zip files with winzip works fine for me; the file is first downloaded and then opened as expected. It's just .exe files that are causing me a problem it seems.

Comment 4

[David J. Lennon]

i'll check - definately have problems with EXE's.

Comment 5

[David J. Lennon]

observed only with EXE files.

Comment 6

[David J. Lennon]

Ability to open EXE files should be added - I'll add a feature request.

Comment 7

[Steve England [:stevee]]

(In reply to comment #6)
> Ability to open EXE files should be added - I'll add a feature request.

I'd search first, because it's been filed numerous times, and resolved as WONTFIX. The inability of firefox to automatically launch an .exe file it has just finsihed downloading is a deliberate security measure.

Comment 8

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

(In reply to comment #7)
> WONTFIX. The inability of firefox to automatically launch an .exe file it has
> just finsihed downloading is a deliberate security measure.

That's correct.

However, still blocking on this bug, since we should just not be giving the user the option.

Comment 9

[Ed Lee :Mardak]

Definitely regressed with bug 395308 because now tempFile.isExecutable()
doesn't work.

These tryserver builds have bug 396477 and bug 396701 potentially fixed.

win32: https://build.mozilla.org/tryserver-builds/[e-mail]
macosx: https://build.mozilla.org/tryserver-builds/[e-mail]
linux: https://build.mozilla.org/tryserver-builds/[e-mail]2

Comment 10

[Ed Lee :Mardak]

Attachment: v1

Don't use this.mLauncher.targetFile.isExecutable because it'll give us the
wrong information.

Collapse shouldntRememberChoice branches because it's considered for openWithDefaultOK now.

initAppAndSaveToDiskValues doesn't need branches for openWithDefaultOK because it's only called from the else path of the first !this.openWithDefaultOK.

Comment 11

[Ed Lee :Mardak]

Example of a .exe not showing the "no choice" dialog from bug 249812..

http://mozilla.cs.utah.edu/pub/mozilla.org/firefox/releases/2.0.0.7/win32/en-US/Firefox%20Setup%202.0.0.7.exe

Shows up as default action? "exefile" on trunk and shows up as "save file" with tryserver.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 283068 [details] [diff] [review]
v1

I don't feel comfortable reviewing this part of the code actually.  I am going to defer to gavin.

Comment 13

[Ed Lee :Mardak]

So this version of the patch basically relies on the mimetype and ignores the IsExecutable because it won't work with the salted tempFile, but one concern is that the mimetype won't be right.

The reason why the IsExecutable doesn't work is tempFile is now "<random>.<ext>.part" instead of "<random>.<ext>". Where "<ext>" comes from MIMEInfo->GetPrimaryExtension.

At least for windows, IsExecutable just looks at the file extension and checks if it's one of 72 possible extensions including "exe".

So before, we were basically relying on GetPrimaryExtension [from exthandler] -> CreateUnique [from exthandler] -> IsExecutable [from nsHelperAppDlg through mLauncher.targetFile] to determine if the file is executable.

Should we try to maintain this logic through GetPrimaryExtension -> ?? -> IsExecutable [all from nsHelperAppDlg through mLauncher.MIMEInfo] instead of only relying on the mimetype from the channel and/or OS based on extension?

(For the ?? above, do we really need to CreateUnique to use the IsExecutable logic? If so, do we CreateUnique temporarily and remove?)

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> Should we try to maintain this logic through GetPrimaryExtension -> ?? ->
> IsExecutable [all from nsHelperAppDlg through mLauncher.MIMEInfo] instead of
> only relying on the mimetype from the channel and/or OS based on extension?
> 
> (For the ?? above, do we really need to CreateUnique to use the IsExecutable
> logic? If so, do we CreateUnique temporarily and remove?)

That approach does sound best. I don't see why you would need createUnique. Can't you just create an nsIFile that corresponds to the final destination, and check isExecutable on that? Is the problem that you don't know the final destination when this code runs?

Comment 15

[Ed Lee :Mardak]

(In reply to comment #14)
> Is the problem that you don't know the final destination when this code runs?
Right. exthandler's GetTargetFile doesn't provide the real target until SaveToDisk or OpenWithApplication and the target file changes depending on which one gets called.

But as we now know, the existing logic is just using GetPrimaryExtension + IsExecutable, so it doesn't really care about the real final target.

Comment 16

[Ed Lee :Mardak]

Attachment: v2

Make a temp file of the temp file but with the .part removed so that we get the correct isExecutable behavior.

Comment 17

[Ed Lee :Mardak]

gavin: Would it be better if we stored a mTempFileIsExecutable one time and reused that throughout?

biesi: Or should exthandler's interface provide a GetTargetIsExecutable which would compute it once from the real target and return that value?

Comment 18

[Ed Lee :Mardak]

And by exthandler's interface I mean nsIHelperAppLauncher ;)

Comment 19

[Ed Lee :Mardak]

I'm assuming this is wanted for the beta as it's a regression, so should I go with the changing nsIHelperAppLauncher interface path?

I suppose worst case is that we need to back out the regressing bug, but then now we have the security issue that bug was trying to address.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> gavin: Would it be better if we stored a mTempFileIsExecutable one time and
> reused that throughout?

Yeah, I think that would be better.

Comment 21

[Ed Lee :Mardak]

Attachment: v3

Set mTargetIsExecutable before we use it the first time and reuse the value if we need it later.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I don't like how this makes assumptions about the implementation details of isExecutable() on a specific platform, and how it creates a temp file only to check isExecutable(). Can we get SetUpTempFile to do something like first create the "normal" file, check isExecutable(), store that result, then rename the file to .part before passing that .part file on? And then expose some way to get the stored isExecutable value from the external helper app service?

Comment 23

[Ed Lee :Mardak]

So along the lines of what I was asking earlier?

> should [nsIHelperAppLauncher] provide a GetTargetIsExecutable which
> would compute it once from the real target and return that value?

So then from the helper dialog, it would just do this.mLauncher.targetIsExecutable.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #23)
> So along the lines of what I was asking earlier?

Yeah, I thought that's what I was approving in comment 20. I guess I misunderstood your comment 17, I thought you were asking biesi and I the same thing.

Comment 25

[Ed Lee :Mardak]

With M9 code freeze at 0 blockers, we should still have ~1 week, so I hope to get a new patch with the nsIHelperAppLauncher idl change over the weekend.. perhaps earliest friday night. If someone wants to hop in, that would be nice. :)

Just to make sure gavin, general idea is for the current patch's uses of |this.mTargetIsExecutable|, switch them to |this.mLauncher.targetIsExecutable| and add in bits to store executableness when creating the temp file.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #25)
> Just to make sure gavin, general idea is for the current patch's uses of
> |this.mTargetIsExecutable|, switch them to |this.mLauncher.targetIsExecutable|
> and add in bits to store executableness when creating the temp file.

That sounds good.

Comment 27

[Ed Lee :Mardak]

Attachment: v4

I haven't tried this on windows.. builtbot is timing out.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

That patch still makes assumptions about the implementation of IsExecutable per-platform, because of the #ifdef XP_WIN. That seems a bit fragile; I think I'd prefer taking the XP_WIN path cross-platform and creating the "dummy" file unconditionally. Maybe biesi can chime in here.

Is it also guaranteed that the code that renames a completed download to remove the ".part" will give us a file with the same "executable" characteristics as the "dummy file" created by this code? Maybe we can assume that the effects of CreateUnique won't affect "executableness", but we should at least add a comment to that effect.

Comment 29

[Ed Lee :Mardak]

(In reply to comment #28)
> That patch still makes assumptions about the implementation of IsExecutable
> per-platform, because of the #ifdef XP_WIN
Well, it's pretty much just an optimization so that non-windows code will assume not-executable because we're making it 0600. But interestingly enough, this would have also fixed the other bug of files incorrectly being considered executable on linux for NTFS mounts -- this code would assume not-executable even though IsExecutable would look at the wrong rwxrwxrwx bits.

> Is it also guaranteed that the code that renames a completed download to remove
> the ".part" will give us a file with the same "executable" characteristics as
> the "dummy file" created by this code?
Well, at least right now, the executable behavior is just based on the file extension for windows, so the real target should have the same executable-ness. On other platforms, the file will be 0600 (or 0400 if open-with), so it shouldn't change.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Yes, I know that the #ifdef is an optimization based on knowing the current implementation details of IsExecutable on Windows. I'm saying that it's a bad idea to rely on those implementation details because they're not guaranteed by the nsIFile API, and because they could change in the future (see e.g. bug 209392).

Comment 31

[:Gavin Sharp [email: [e-mail]]]

To clarify, my two concerns with this patch are:

1) comment 30
2) The process of adding .part, finishing the download, and then removing the .part once the download completes would somehow alter the "executableness" of the target file, such that mTargetFileIsExecutable's value (obtained before the download) would not reflect the final saved file's actual "executableness".

Comment 32

[Ed Lee :Mardak]

Attachment: v4.1

(In reply to comment #30)
> I'm saying that it's a bad idea to rely on those implementation details 
> because they're not guaranteed by the nsIFile API
Removed the ifdefs to do the same for all platforms.

(In reply to comment #31)
> The process of adding .part, finishing the download, and then removing the
> .part once the download completes would somehow alter the "executableness" of
> the target file, such that mTargetFileIsExecutable's value (obtained before the
> download) would not reflect the final saved file's actual "executableness".
Changed to call the final target's IsExecutable if the file is set.

Comment 33

[Ed Lee :Mardak]

Comment on attachment 286460 [details] [diff] [review]
v4.1

sr? for the exthandler part. Seems like nsExternalHelperAppService is the only implementation of nsIHelperAppLauncher.

Comment 34

[Ed Lee :Mardak]

For those who want to help test, there's a tryserver build with the latest patch (v4.1) applied:

https://build.mozilla.org/tryserver-builds/2007-10-28_12:[e-mail]-bug396701/

The windows version correctly shows the smaller "Save File" dialog instead of "Open with" when clicking on .exe links like

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-3.0a9pre.en-US.win32.installer.exe

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 286460 [details] [diff] [review]
v4.1

how about making this temp file stuff #ifdef XP_WIN? other platforms don't use the extension in considering whether something is executable, they could continue using mTargetFile->IsExecutable

Comment 36

[:Gavin Sharp [email: [e-mail]]]

We discussed the conflict between biesi's comment and mine on IRC, and biesi pointed out that not having the ifdef present similarly assumes that the file content doesn't matter (checking before completing the download vs. after). I suppose I can live with the ifdef approach, but I like the mFinalFileDestination optimization of attachment 286460 [details] [diff] [review] (v.4.1). I'll r+ a patch that combines those two (v4 with the GetTargetFileIsExecutable from v4.1).

Comment 37

[Ed Lee :Mardak]

Attachment: v4.2

> patch that combines those two (v4 with the GetTargetFileIsExecutable from
> v4.1).
Combined.

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 286877 [details] [diff] [review]
v4.2

>diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp

>+NS_IMETHODIMP nsExternalAppHandler::GetTargetFileIsExecutable(PRBool *aExec)
>+{
>+  // Use the real target if it's been set
>+  if (mFinalFileDestination) {
>+    *aExec = PR_TRUE;
>+    mFinalFileDestination->IsExecutable(aExec);
>+  } else {
>+    // Otherwise, use the stored executable-ness of the temporary
>+    *aExec = mTargetFileIsExecutable;
>+  }
>+
>+  return NS_OK;

Seems like we probably want to be propagating any errors here rather than defaulting to PR_TRUE in the mFinalFileDestination case.

>diff --git a/uriloader/exthandler/nsExternalHelperAppService.h b/uriloader/exthandler/nsExternalHelperAppService.h

>+  /**
>+   * Track the executable-ness of the target file based on the temporary file
>+   * before we tinker with the extension that might make it not executable.
>+   */
>+  PRBool mTargetFileIsExecutable;

I think mTempFileIsExecutable would be a better name.

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 286877 [details] [diff] [review]
v4.2

+    *aExec = PR_TRUE;
+    mFinalFileDestination->IsExecutable(aExec);

I agree with gavin here

+  mTargetFileIsExecutable = PR_FALSE;

I'd really prefer it if you just called IsExecutable here, even if that means that you have to move the code down a bit

Comment 40

[Ed Lee :Mardak]

Attachment: v4.3

I'll check with biesi to make sure if I did what he was describing before checking in.

(In reply to comment #38)
> >+  if (mFinalFileDestination) {
> >+    *aExec = PR_TRUE;
> >+    mFinalFileDestination->IsExecutable(aExec);
> Seems like we probably want to be propagating any errors here rather than
> defaulting to PR_TRUE in the mFinalFileDestination case.
Propagated (and collapsed the if blocks).

> >+  PRBool mTargetFileIsExecutable;
> I think mTempFileIsExecutable would be a better name.
Renamed (and fixed comments).

(In reply to comment #39)
> +  mTargetFileIsExecutable = PR_FALSE;
> I'd really prefer it if you just called IsExecutable here, even if that means
> that you have to move the code down a bit
I added a new #ifndef XP_WIN block below the CreateUnique and updated the comments.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

The mTempFileIsExecutable header comment is a bit too specific to the Windows case, I think, especially now that you actually call IsExecutable for non-Windows. I think just "Track the executable-ness of the temporary file" is fine.

Comment 42

[Ed Lee :Mardak]

Checking in toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in,v  <--  nsHelperAppDlg.js.in
new revision: 1.57; previous revision: 1.56
done
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.354; previous revision: 1.353
done
Checking in uriloader/exthandler/nsExternalHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.h,v  <--  nsExternalHelperAppService.h
new revision: 1.89; previous revision: 1.88
done
Checking in uriloader/exthandler/nsIExternalHelperAppService.idl;
/cvsroot/mozilla/uriloader/exthandler/nsIExternalHelperAppService.idl,v  <--  nsIExternalHelperAppService.idl
new revision: 1.25; previous revision: 1.24
done

Comment 43

[Ed Lee :Mardak]

Attachment: as checked in

Fixed the comment per gavin's suggestion.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Hmm, we should also probably initialize mTempFileIsExecutable just in case SetUpTempFile throws before setting it, or isn't called?

Comment 45

[Marcia Knous [:marcia]]

I am a little confused when trying to verify this because I don't get consistent results when left clicking on an .exe file that I want to download (I am using the mozilla.com site to download). Sometimes when I left click I get just the save file/cancel dialog, but on a few occasions i have seen the dialog come up that has the choice of open as .exe. I need clarification of what expected behavior I should see here.  I am using  Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007110205 Minefield/3.0a9pre to verify. thanks.

Comment 46

[Ed Lee :Mardak]

Woah. O.O. I have no idea what's going on, but I see the "open as .exe file" as well.

From http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

if I keep clicking the firefox-3.0a9pre.en-US.win32.installer.exe link and canceling the download, approximately 1 out of ever 15 clicks will result in the wrong download dialog.

At least compared to before, every time the dialog would be the wrong one.

gavin: Would this be related to SetUpTimeFile not getting called early enough? But even if we initialize mTempFileIsExecutable before that.. would it be correct to assume "yes it's executable"?

Comment 47

[:Gavin Sharp [email: [e-mail]]]

Hmm, yeah. Is it possible that onStartRequest (which calls SetUpTempFile) is not called before the code that checks targetFileIsExecutable?

If that's the case, I have an idea. From what I can tell SetUpTempFile doesn't actually depend on onStartRequest having been called (and doesn't seem to use it's aChannel param), so perhaps we could have the targetFileIsExecutable getter call it if it hasn't already been called? And rename it "ensureTempFile", or something? Need to make sure that calling SetUpTempFile before onStartRequest isn't a problem (i.e. doesn't leave around temp files should the download fail). Is OnStartRequest garanteed to be called, even if the download fails? Need input from someone who knows this code more, I think... biesi?

Comment 48

[Ed Lee :Mardak]

I put up a simple test patch where I add to nsExternalAppHandler's initialization list:
+, mTempFileIsExecutable(PR_TRUE)

https://build.mozilla.org/tryserver-builds/2007-11-02_12:[e-mail]-try396701/

I still get the "openwith exefile" from time to time, so it doesn't seem to be a default value issue.. ?

Comment 49

[Ed Lee :Mardak]

Ok... turns out in the cases where it shows "exefile", mLauncher.tempFile is
something like "c:\doc....\local settings\Temp-1", Temp-2, etc. So it seems
like  SetUpTempFile isn't getting called...

However, from my understanding of dialogs/contract id/etc..

nsExternalAppHandler::OnStartRequest first calls SetUpTempFile
http://mxr.mozilla.org/mozilla/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1301

and /later/ does it create an instance of NS_IHELPERAPPLAUNCHERDLG_CONTRACTID
http://mxr.mozilla.org/mozilla/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1436

(which gets us nsHelperAppDlg.js.in) that has a factory that does..
new nsUnknownContentTypeDialog()
http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#1152

and eventually it loads causing..
<dialog ... onload="dialog.initDialog();"
http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/content/unknownContentType.xul#55

which then shows the open with dialog... So shouldn't SetUpTempFile already
have mTempFile correctly set?

Comment 50

[Ed Lee :Mardak]

I've tested this bug with this build with bug 402298's patched applied and none of my 50 clicks resulted in the wrong download dialog. (So I'm 99+% sure it's fixed... :p)

https://build.mozilla.org/tryserver-builds/2007-11-02_18:[e-mail]/

So this bug can get resolved fix if bug 402298 gets reviewed and approved.

Comment 51

[Reed Loden [:reed]]

Resolving as fixed again, as bug 402298 is fixed.

Comment 52

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007110605 Minefield/3.0a9pre. To verify I repeated the process x25 and each time I received the correct dialog, Save File or cancel.

Comment 53

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=5006

in-litmus+