916126 - Firefox 24.0b1+ does not mark downloaded executables as coming from Internet zone

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Jeroen van der Gun]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

Download any executable file on Windows, try to launch it.


Actual results:

In Firefox 24.0 beta 1 and later, a saved downloaded executable is not treated as an untrusted file by Windows, such that Windows asks no confirmation before execution.

Note that if browser.download.manager.alertOnEXEOpen is true (default), then Firefox does ask for confirmation, but this dialog does not provide information about digital signatures and is thus less useful than the Windows dialog that should follow afterwards if both confirmation dialogs are enabled.


Expected results:

In Firefox 23, the file is stored on the harddisk as coming from the Internet, such that Windows shows a confirmation prompt with information about digital signatures before launching the executable.

Comment 1

[Jeroen van der Gun]

This bug is also present in the Firefox 24.0 release.

Comment 2

[Teodor]

I can confirm this bug on multiple machines 100%. Win7 x64, FF24.
Please fix it and make it obey the system setting for zone.identifier information, so it can be turned on or off by user's discretion.
Or at least add a clear and sitinct advanced option so it can be turned on or off in FF itself.
And please make sure it finds its way into 24.0.1 or something, just before 25 comes out, please.

Comment 3

[mjh563]

I wonder if the fix for bug 858234 caused this?

Comment 4

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/43c0123f158b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604141615
Bad:
http://hg.mozilla.org/mozilla-central/rev/22cb668fd727
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604174517
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=43c0123f158b&tochange=22cb668fd727


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e5b6545901b0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604081014
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/51e50ba61026
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604100614
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e5b6545901b0&tochange=51e50ba61026

Regressed by 
b6cce1e41253	Monica Chew — Move execution from nsExternalAppHandler to nsDownload (b=858234, r=paolo)

Comment 5

[Alex Keybl [:akeybl]]

dveditz - how do you feel about this criticality of this bug? we're leaning towards fixing in FF25 but want to hear your opinion as well.

Comment 6

[Daniel Veditz [:dveditz]]

We should definitely get this fixed in Fx25 and 24.1(esr). I don't think this rises to Fx24 chemspill level but we may want to blog a warning about it.

Comment 7

[Daniel Veditz [:dveditz]]

I'm confused: the only place I see us creating the Zone.Identifier ADS was added in bug 909022, _after_ this regression. How were we creating these before? We definitely had them, I see them in my Downloads directory right up until bug 858234 landed. Were we calling some windows API for which that was a side-effect?

bug 909022 adds the ZI stream only to "executables"; before the regression I had these on everything I downloaded. Maybe it was a side-effect of the virus scanning, which we've stopped doing? A change like that should have gotten a security review.

Comment 8

[Alice0775 White]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> I'm confused: the only place I see us creating the Zone.Identifier ADS was
> added in bug 909022, _after_ this regression. How were we creating these
> before? We definitely had them, I see them in my Downloads directory right
> up until bug 858234 landed. Were we calling some windows API for which that
> was a side-effect?
> 
> bug 909022 adds the ZI stream only to "executables"; before the regression I
> had these on everything I downloaded. Maybe it was a side-effect of the
> virus scanning, which we've stopped doing? A change like that should have
> gotten a security review.

OK, my bad, Aurora26.0a2 and Nightly27.0a1 work properly.
I will reset flags for 26.0 and 27.0.

Comment 9

[Masatoshi Kimura [:emk]]

Then all we have to do is backporting bug 909022 to Fx25?

Comment 10

[:Paolo Amadini]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> Maybe it was a side-effect of the virus scanning, which we've stopped doing?

The zone information is set by the same Windows API that does the virus scanning, yes.

However, bug 858234 wasn't about removing virus scanning at all. I'm totally confused about the reason why that bug introduced this regression, the patch there shouldn't have prevented the virus scanning from being executed. Maybe there are race conditions or just an implementation bug.

> A change like that should have gotten a security review.

The planned virus scanning change is part of the introduction of the new Downloads API. We have a feature page tracking it:

https://wiki.mozilla.org/Features/Desktop/Downloads_API

What is the correct procedure to ensure the feature gets a full security review? Aren't feature pages enough to be tracking the needed general reviews of new features, including the security one?

Comment 11

[:Paolo Amadini]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> Then all we have to do is backporting bug 909022 to Fx25?

Since that is part of a different feature, this cannot be done. If we can't figure out the cause of the regression, we may consider backing out bug 858234 on Beta if it doesn't have too much code depending on it.

Comment 12

[Masatoshi Kimura [:emk]]

Is the virus scan executed on Nightly?

Comment 13

[:Paolo Amadini]

(In reply to Masatoshi Kimura [:emk] from comment #12)
> Is the virus scan executed on Nightly?

We don't call the Windows API that triggers the scan in Nightly, because it caused critical performance issues and was also redundant with the on-access scan that antivirus software does anyways by default. See the firefox-dev thread for more details:

https://mail.mozilla.org/pipermail/firefox-dev/2013-August/000848.html

Comment 14

[Masatoshi Kimura [:emk]]

You mean, bug 858234 wasn't meant to remove virus scanning, but virus scanning would have been removed as a part of migration to Download Manager anyway. Correct?

Comment 15

[:Paolo Amadini]

(In reply to Masatoshi Kimura [:emk] from comment #14)
> You mean, bug 858234 wasn't meant to remove virus scanning, but virus
> scanning would have been removed as a part of migration to Download Manager
> anyway. Correct?

Yes, the explicit scanning call is not done anymore as part of the migration to the new Downloads API in Firefox 26.

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

I don't think this bug is that simple. bug 858234 moved duplicate calls of ExecuteDesiredAction from the nsExternalAppHandler to nsDownload (no longer used, I don't think).

The old nsDownload code set state DOWNLOAD_SCANNING on windows which triggers the old windows-specific nsDownloadScanner.

bug 852482 adds the JS equivalent of ExecuteDesiredAction to the new JS download manager.

Is nsExternalAppHandler not triggering the same state machine changes in the new downloads API than the old one?

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Paolo answered my question. Now my question is, why is downloadDone not being reached to set the zone information?

Comment 18

[Masatoshi Kimura [:emk]]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #17)
> Paolo answered my question. Now my question is, why is downloadDone not
> being reached to set the zone information?

Because the code was not present until bug 909022?

Comment 19

[[:mmc] Monica Chew (no longer reading bugmail)]

Sorry, I'm not being clear. nsExternalAppHandler triggers nsITransfer->OnStateChange(STATE_STOP | STATE_IS_NETWORK | STATE_IS_REQUEST). The new Downloads nsITransfer (from toolkit/components/jsdownloads/src/DownloadLegacy) calls DownloadCore.onTransferFinished().

It seems like DownloadCore.onTransferFinished() should also set the zone information, rather than trying to roll back bug 858234, which I am not sure will fix the bug anyway since I don't know how the old nsDownload interacts with the new Download manager.

Paolo, is this correct?

Comment 20

[[:mmc] Monica Chew (no longer reading bugmail)]

Looking at this more, I think that the original call to set the zone comes from https://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadScanner.cpp#465

         // Provide the src for everything but data: schemes.
464         if (!mSkipSource)
465           (void)ae->SetSource(mOrigin.BeginWriting());

According to the IAttachmentExecute docs (http://msdn.microsoft.com/en-us/library/windows/desktop/bb776305%28v=vs.85%29.aspx) AttachmentExecute->SetSource and AttachmentExecute->SetReferrer both set the security zone, so that's what the original zone setting comes from, not an explicit call to set Zone.identifier.

I'm still not clear on why the bug bisected to bug 858234, because that bug was not supposed to change the state machine (e.g., still set the state to DOWNLOAD_SCANNING) but I still think the right fix is to hook the zone change sometime after DownloadLegacySaver.onTransferFinished.

Comment 21

[:Paolo Amadini]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #20)
> I'm still not clear on why the bug bisected to bug 858234, because that bug
> was not supposed to change the state machine (e.g., still set the state to
> DOWNLOAD_SCANNING)

Right.

> but I still think the right fix is to hook the zone
> change sometime after DownloadLegacySaver.onTransferFinished.

This is already done in Firefox 26 and 27 by bug 909022.

This bug is about Firefox 25 (and Firefox 24 ESR) where none of the code in the "jsdownloads" folder is executed, but we still use the old nsIDownloadManager state machine.

If you can figure out what went wrong in bug 858234 (assuming the bisection is correct), we can fix that rather than reverting the change. However, since the original change was just preliminary to other features that are not present in 24 and 25, backing out the bug in those versions might also work.

Comment 22

[[:mmc] Monica Chew (no longer reading bugmail)]

Sorry for the confusion. In that case, based on comment 11 and comment 6 I think backing this out in 25 is the right solution.

Comment 23

[[:mmc] Monica Chew (no longer reading bugmail)]

Timing question: is it ok for me to work on this next week? I want to do it well in advance of the merge date but could use the extra time this week, and I don't have a Windows machine at the moment.

Comment 24

[Alex Keybl [:akeybl]]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #23)
> Timing question: is it ok for me to work on this next week? I want to do it
> well in advance of the merge date but could use the extra time this week,
> and I don't have a Windows machine at the moment.

Landing next week (third week of beta) should be fine given comment 6 and the sec-moderate rating.

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

Working on this now. This requires rolling back

https://hg.mozilla.org/integration/mozilla-inbound/rev/7874d6104c5b (remove FixFilePermissions)
https://hg.mozilla.org/integration/mozilla-inbound/rev/81dd6f59a133 (fix crash in createTransfer)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cce1e41253 (merge ExecuteDesiredAction)

What's the right way to do this? hg backout on the corresponding changes in mozilla-beta?

Comment 26

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Backout changes to ExecuteDesiredAction only in the download manager (

Comment 27

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Backout changes to ExecuteDesiredAction only in the download manager (

Comment 28

[[:mmc] Monica Chew (no longer reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=828a4a6efa90

Comment 29

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

26/27 unaffected per comment 8/comment 21

Comment 30

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 813207 [details] [diff] [review]
Backout changes to ExecuteDesiredAction only in the download manager (

Review of attachment 813207 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -1958,2 @@
>  
> -  if (mCanceled) {

I wonder if this is the bug: if the download is canceled we don't notify transfer (fixed in bug 899102)

Comment 31

[[:mmc] Monica Chew (no longer reading bugmail)]

If that's the bug, I think there are two ways to proceed:

1) Uplift https://bugzilla.mozilla.org/show_bug.cgi?id=899102 to beta to see if that fixes the problem
2) Assuming try is green, check in https://bugzilla.mozilla.org/attachment.cgi?id=813207 to beta

I think it's early enough before the merge date that we could try 1), and if that doesn't work, try 2). Alex, Gavin -- what do you think?

Comment 32

[Alex Keybl [:akeybl]]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #31)
> If that's the bug, I think there are two ways to proceed:
> 
> 1) Uplift https://bugzilla.mozilla.org/show_bug.cgi?id=899102 to beta to see
> if that fixes the problem
> 2) Assuming try is green, check in
> https://bugzilla.mozilla.org/attachment.cgi?id=813207 to beta
> 
> I think it's early enough before the merge date that we could try 1), and if
> that doesn't work, try 2). Alex, Gavin -- what do you think?

Can we provide a try build to an impacted user for #1 prior to landing on Beta?

Comment 33

[[:mmc] Monica Chew (no longer reading bugmail)]

> 
> Can we provide a try build to an impacted user for #1 prior to landing on
> Beta?

Oh yeah, that's a much better idea.

https://tbpl.mozilla.org/?tree=Try&rev=dfc8e1257a55

Comment 34

[[:mmc] Monica Chew (no longer reading bugmail)]

Can one of the affected folks try a build from

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dfc8e1257a55/try-win32/
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dfc8e1257a55/try-win32-debug/

The build for windows 2012 requires a mozconfig change for beta which I'm not sure how to push to try. These binaries correspond to option #1 from comment 31. The try errors from that build are due to bug 919100.

Comment 35

[Alex Keybl [:akeybl]]

Alice, do you have the time to make sure this is resolved in the above build?

Comment 36

[Alice0775 White]

(In reply to Alex Keybl [:akeybl] from comment #35)
> Alice, do you have the time to make sure this is resolved in the above build?

The tryserver build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dfc8e1257a55/try-win32/firefox-25.0.en-US.win32.zip
DOES NOT FIX the problem.

http://hg.mozilla.org/try/rev/dfc8e1257a55
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 ID:20131002134354

Comment 37

[[:mmc] Monica Chew (no longer reading bugmail)]

Thanks so much Alice! Do you have time to see if one of

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-828a4a6efa90/try-win32/
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-828a4a6efa90/try-win32-debug/

fixes the problem? This maps to option 2) in comment 31. If it doesn't fix the problem, then the bisection is wrong.

Thanks,
Monica

Comment 38

[Alice0775 White]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #37)
> Thanks so much Alice! Do you have time to see if one of
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 828a4a6efa90/try-win32/

The tryserver build FIXES the problem :)

http://hg.mozilla.org/try/rev/828a4a6efa90
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 ID:20131002112804

Comment 39

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 813207 [details] [diff] [review]
Backout changes to ExecuteDesiredAction only in the download manager (

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 858234
User impact if declined: Windows users on FF 25 won't have downloaded files marked as coming from the Internet zone, which triggers an AV scan
Testing completed (on m-c, etc.): see comment 38
Risk to taking this patch (and alternatives if risky): We still don't understand the regression. We think the cause is that the original patch changed the entry point into nsIWebProgressListener.onStateChange but it's not clear what the bug is.
String or IDL/UUID changes made by this patch: nsITransfer.idl

Comment 40

[Jeroen van der Gun]

User impact if declined: also no prompt for checking digital signatures of downloaded executables before execution

Comment 41

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

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #39)

Per our discussion on IRC, I'm worried that this patch gets us to an untested intermediate state that is riskier than a forward fix, particularly since we still don't seem to understand the root cause.

Neil, were you able to look into this at all pre-Summit? If not, can you treat it as a high priority now?

Comment 42

[Neil Deakin]

The issue here is that the downloaded target file exists but has a filesize of 0 when IAttachmentExecute::Save is called. It should be called once the file is fully present.

The change in bug 858234 changed the way and when OnStateChange is called with STATE_STOP so I would guess that it is being sent to early. I confirmed that backing out that patch fixes the bug.

Comment 43

[[:mmc] Monica Chew (no longer reading bugmail)]

I chatted with Neil, and the only places that call NotifyTransfer (which is the only place OnStateChange is called with STATE_STOP) are when BackgroundFileSaver is done or canceled, so I am not sure what would be calling OnStateChange early.

Comment 44

[Alex Keybl [:akeybl]]

Does this approval request stand? Or are we still thinking about taking a forward fix?

Comment 45

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Alex Keybl [:akeybl] from comment #44)
> Does this approval request stand?

Yes. The attached patch from comment 27 does what Paolo recommends below, and also the recommendations in comments 22 and 25. The higher level of QA that Paolo recommends has been done in comment 38. From Paolo in email:

I think we have the best chances of success by rolling back to a "known
good" state rather than a speculative forward fix. We should back out
the patches touching nsDownloadManager and nsExternalHelperAppService
in reverse order. We can cross-check that the tree returned to a state
close to Firefox 23, and some high-level QA on downloads should suffice.

This amounts to backing out bug 880947 and bug 858234 for Firefox 25.
It might be the same for Firefox 24 ESR too, I've not checked in detail.

Other notes from Paolo:
I've checked the code based on Neil's comment. I think the reason is
that we used to call the MoveFile function in nsExternalHelperAppHandler
before we sent the completion notification, to copy the ".part" file to
the target. After the patch, we copied the file in nsDownloadManager,
when the download finished, after the IAttachmentExecute calls. It's
still unclear why we had code that moved the temp file to the target in
nsDownloadManager to begin with, and why it didn't cause an error if
the same operation was executed by nsExternalHelperAppHandler.

Comment 46

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 813207 [details] [diff] [review]
Backout changes to ExecuteDesiredAction only in the download manager (

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This also affects ESR 24. Please see comment 39.
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 47

[Alex Keybl [:akeybl]]

Comment on attachment 813207 [details] [diff] [review]
Backout changes to ExecuteDesiredAction only in the download manager (

Thanks for clarifying Monica!

Comment 48

[Ryan VanderMeulen [:RyanVM]]

This makes IDL changes. I got ba+ from akeybl over IRC. I'm assuming that whatever UUIDs this patch is changing are going to be OK. Leaving open for whatever work is still needed on 26+.

https://hg.mozilla.org/releases/mozilla-beta/rev/28bce17c7c50
https://hg.mozilla.org/releases/mozilla-esr24/rev/2f5ce92699e2

Comment 49

[[:mmc] Monica Chew (no longer reading bugmail)]

Setting flags for 25 and 26 to unaffected per Gavin's comment 29. Thanks for checking that in for me, Ryan :)

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta due to OSX xpcshell failures. ESR24 appears to be OK.
https://hg.mozilla.org/releases/mozilla-beta/rev/8111752d3c5c

https://tbpl.mozilla.org/php/getParsedLog.php?id=28937869&tree=Mozilla-Beta

Comment 51

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Backout changes to ExecuteDesiredAction only in the download manager (

Comment 52

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #50)
> Backed out from beta due to OSX xpcshell failures. ESR24 appears to be OK.
> https://hg.mozilla.org/releases/mozilla-beta/rev/8111752d3c5c
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=28937869&tree=Mozilla-Beta

I am sorry, I incorrectly interpreted previous try results as triggering existing bugs in test_DownloadLegacy. There is new test code in test_DownloadLegacy.js that relies on the behavior of the patch that was backed out, and I missed it.

New try on beta:
https://tbpl.mozilla.org/?tree=Try&rev=3b635fab909b

Comment 53

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48)
> This makes IDL changes. I got ba+ from akeybl over IRC.

Hrm, this looks dangerous to me. Didn't realize we were changing nsITransfer, that is quite likely to break some add-ons. :( Can we avoid that interface change in the backout by leaving setSha256Hash and just making the implementations no-ops?

Comment 54

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #53)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48)
> > This makes IDL changes. I got ba+ from akeybl over IRC.
> 
> Hrm, this looks dangerous to me. Didn't realize we were changing
> nsITransfer, that is quite likely to break some add-ons. :( Can we avoid
> that interface change in the backout by leaving setSha256Hash and just
> making the implementations no-ops?

The setSha256 change to nsITransfer was written for something that hasn't landed yet, and requires the new Downloads API to be useful. The only useful consumer of the sha256 is going to be in review, in bug 895476 and the hash is only available when BackgroundFileSaver is in the loop. Since the primary consumer of BackgroundFileSaver is the new Downloads API, and useJSTransfer which is incomplete in 25.

I think it's very unlikely that current addons are using setSha256 without being totally incorrect at this point.

Do you still want me to make this change?

Comment 55

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

It's not about add-ons using that method specifically, it's about avoiding a change to the nsITransfer IID/vtable this late in the game. Changes like those either mean crashes (if the compiled-code add-on does something nasty), failure to work (if the compiled-code add-on properly handles QI failures), or the need to re-compile the add-on (if you want things to work) for all add-ons that use the interface for anything (not just those calling setSha256).

Leaving the IID/vtable the same, even if it results in useless no-op methods hanging around, is a better choice.

Comment 56

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Backout changes to ExecuteDesiredAction only in the download manager (

Comment 57

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Backout changes to ExecuteDesiredAction only in the download manager (

Changes previous patch to cause addons that rely on nsITransfer.setSha256Hash
to fail silently than noisily and reverts idl changes.

Comment 58

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #55)
> It's not about add-ons using that method specifically, it's about avoiding a
> change to the nsITransfer IID/vtable this late in the game. Changes like
> those either mean crashes (if the compiled-code add-on does something
> nasty), failure to work (if the compiled-code add-on properly handles QI
> failures), or the need to re-compile the add-on (if you want things to work)
> for all add-ons that use the interface for anything (not just those calling
> setSha256).

The new version of the patch avoids nsITransfer idl changes, so that all addons that rely on setSha256 will fail silently. If I were an addon developer who relied on this, I'd rather have a known error than a no-op, but it's your call.

New try: https://tbpl.mozilla.org/?tree=Try&rev=e22c18fb4747

Comment 59

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

It's fine to have the methods return NS_ERROR_FAILURE, warn to the console, or whatever - I have no strong feelings about what it _does_. But we make guarantees to add-on authors that we won't make interface changes past beta1, and so we need to hold to that whenever possible.

Comment 60

[Masatoshi Kimura [:emk]]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #58)
> The new version of the patch avoids nsITransfer idl changes, so that all
> addons that rely on setSha256 will fail silently. If I were an addon
> developer who relied on this, I'd rather have a known error than a no-op,
> but it's your call.

Why don't you throw NS_ERROR_NOT_IMPLEMENTED or something in setSha256?

Comment 61

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Backout changes to ExecuteDesiredAction only in the download manager (

Comment 62

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Backout changes to ExecuteDesiredAction only in the download manager (

Make setsha256 fail with NS_ERROR_NOT_IMPLEMENTED instead of silently with NS_OK.
Try: https://tbpl.mozilla.org/?tree=Try&rev=2fc055896587

Comment 63

[[:mmc] Monica Chew (no longer reading bugmail)]

Please carry over Alex's approval for beta from comment 48.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1cb746254315
https://hg.mozilla.org/releases/mozilla-esr24/rev/98fd811a2517 (to keep beta and esr24 in sync)

Fixed on Fx26+ by bug 909022.

Comment 65

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

(In reply to Monica Chew [:mmc] (gone until Wednesday 10/16)) from comment #62)
> Created attachment 815667 [details] [diff] [review]
> Backout changes to ExecuteDesiredAction only in the download manager

Two concerns I failed to notice earlier:
1) why does this comment out some tests? The tests seem to cover functional behavior, and I was under the impression that we weren't introducing a functional regression with this backout.
2) why is nsIExternalHelperAppService.idl changed without an IID change? This may just be the fault of the patch being backed out. At this point we should probably add the method to the end of the interface, I can push a followup fix.

Paolo, do you have any insight into concern #1?

Comment 66

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

Attachment: followup patch to make a safer IDL change

Hoping either of you can sanity check this.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

And backed out again from beta for Windows mochitest-other failures.
https://hg.mozilla.org/releases/mozilla-beta/rev/97dde6a6d60b

https://tbpl.mozilla.org/php/getParsedLog.php?id=28993510&tree=Mozilla-Beta

Please run this through Try before requesting checkin again.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

The backout did fix the failures from the log in comment 67. The backout also appears to have fixed mochitest-bc leaks that appeared on the push.
https://tbpl.mozilla.org/php/getParsedLog.php?id=28995854&tree=Mozilla-Beta

Comment 69

[:Paolo Amadini]

Comment on attachment 815953 [details] [diff] [review]
followup patch to make a safer IDL change

If the concern here is doing the least possible changes to the IDL, then I
think there is no need to reintroduce the CloseProgressWindow function at all.
It didn't have any callers to start with, so we can leave it out. This applies
to both the Beta and the ESR branches.

By the way, I think we forgot to update the IID on mozilla-central for
nsIExternalHelperAppService.idl:

http://hg.mozilla.org/mozilla-central/diff/b6cce1e41253/uriloader/exthandler/nsIExternalHelperAppService.idl

Not sure it's worth updating it now, because the new interface is already on
Release and any possible breakage would have already happened.

Comment 70

[:Paolo Amadini]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #65)
> 1) why does this comment out some tests? The tests seem to cover functional
> behavior, and I was under the impression that we weren't introducing a
> functional regression with this backout.

The disabled tests are part of the Downloads API, that is not enabled in Firefox 25, even though we run automated tests for some of the functionality. As I noted in email, the Downloads API cannot be safely enabled in Beta 25 because it was a partial implementation meant for early testing, that can now be done in Aurora 26.

> 2) why is nsIExternalHelperAppService.idl changed without an IID change?
> This may just be the fault of the patch being backed out. At this point we
> should probably add the method to the end of the interface, I can push a
> followup fix.

Ah, see my previous comment.

Comment 71

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

(In reply to :Paolo Amadini from comment #70)
> The disabled tests are part of the Downloads API, that is not enabled in
> Firefox 25, even though we run automated tests for some of the
> functionality. As I noted in email, the Downloads API cannot be safely
> enabled in Beta 25 because it was a partial implementation meant for early
> testing, that can now be done in Aurora 26.

OK, thanks. Does this cover the failures in comment 67 too? Those seem to be in a pre-existing test...

Comment 72

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

(In reply to :Paolo Amadini from comment #69)
> If the concern here is doing the least possible changes to the IDL, then I
> think there is no need to reintroduce the CloseProgressWindow function at
> all.
> It didn't have any callers to start with, so we can leave it out. This
> applies
> to both the Beta and the ESR branches.

There's a caller being introduced in the patch - should we remove it as well? I am leaning heavily towards minimal changes here.

Comment 73

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #72)
> (In reply to :Paolo Amadini from comment #69)
> > If the concern here is doing the least possible changes to the IDL, then I
> > think there is no need to reintroduce the CloseProgressWindow function at
> > all.
> > It didn't have any callers to start with, so we can leave it out. This
> > applies
> > to both the Beta and the ESR branches.
> 
> There's a caller being introduced in the patch - should we remove it as
> well? I am leaning heavily towards minimal changes here.

Oh, I was confused - it was just referenced in a comment. Nevermind.

Comment 74

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

https://tbpl.mozilla.org/?tree=Try&rev=9522082e0c4b

Comment 75

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

Attachment: backout patch with safer-IDL rolled in

The IDL change seems to have somehow fixed the oranges?

https://tbpl.mozilla.org/?tree=Try&rev=f969fb910d25

Comment 76

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

Mentioned this to Ryan on IRC, but we should also make the "safer IDL" change on esr24 (to match what landed on beta).

Comment 77

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

https://hg.mozilla.org/releases/mozilla-beta/rev/5eede470990a

Comment 78

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

Pushed the IDL followup to esr24: https://hg.mozilla.org/releases/mozilla-esr24/rev/7df895f807ac

Comment 79

[Manuela Muntean [Away]]

Attachment: screenshot.JPG

On a XP 32-bit machine, I get the dialog shown in the attachement, with all: Firefox 23, 24 and 25 beta 8 (build ID: 20131015052812), after downloading an executable and launching it from the Downloads Panel.

I don't get any other warnings. Can please someone provide some guidance/more details, in order to reproduce this issue? Thanks!

Comment 80

[Manuela Muntean [Away]]

On the same machine and with the same builds used in comment 79, I don't get any kind of warnings (not even the one attached in comment 79)if I click on "Open Containing Folder" from the Downloads Panel and run the executable manually by double clicking it.

Comment 81

[Manuela Muntean [Away]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #80)
> On the same machine and with the same builds used in comment 79, I don't get
> any kind of warnings (not even the one attached in comment 79)if I click on
> "Open Containing Folder" from the Downloads Panel and run the executable
> manually by double clicking it.

Just to make this clear, I'm not getting the warning on Firefox 23, 24 and 25 beta 8 (where it should be fixed).

Comment 82

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #81)
> (In reply to Manuela Muntean [:Manuela] [QA] from comment #80)
> > On the same machine and with the same builds used in comment 79, I don't get
> > any kind of warnings (not even the one attached in comment 79)if I click on
> > "Open Containing Folder" from the Downloads Panel and run the executable
> > manually by double clicking it.
> 
> Just to make this clear, I'm not getting the warning on Firefox 23, 24 and
> 25 beta 8 (where it should be fixed).

Manuela, can you please try the build from comment 38 to see if you observe the same behavior?

Comment 83

[[:mmc] Monica Chew (no longer reading bugmail)]

Manuela, I am having trouble understanding your comments, sorry. The desired behavior is that the dialog https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=817706 should appear on FF 25b8. It should appear in FF 23 because the breakage did not appear until FF 24.

Can you please clarify whether or not this dialog appears in FF25b8? If it does not appear in FF24, then the steps to test manually are broken.

Thanks,
Monica

Comment 84

[[:mmc] Monica Chew (no longer reading bugmail)]

> Can you please clarify whether or not this dialog appears in FF25b8? If it
> does not appear in FF23, then the steps to test manually are broken.\

Correction: it should also appear in FF 23

Comment 85

[Manuela Muntean [Away]]

> Manuela, can you please try the build from comment 38 to see if you observe
> the same behavior?

The build from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-828a4a6efa90/try-win32/ doesn't exist anymore. I get the 404 Page Not Found Error.

Comment 86

[Manuela Muntean [Away]]

Please see below the 2 scenarios I've used while testing this

A) with all of these builds: Firefox 23, 24 and 25 beta 8:

1. Open http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b8-candidates/build1/win32/en-US/ and download an .exe file
2. After the download finishes, open the Downloads Panel and click the .exe file in order to install the build

Results: 

a) on XP 32-bit: after step 2, I can see https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=817706

b) on Win 7 64-bit: after step 2, I can't see https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=817706

B) with all of these builds: Firefox 23, 24 and 25 beta 8:

1. Open http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b8-candidates/build1/win32/en-US/ and download an .exe file
2. After the download finishes, open the Downloads Panel and click on "Open Containing Folder" 
3. After the folder opens, double click the .exe file in order to install the build

Results: 

a) both on XP 32-bit and Win 7 64-bit: after step 3, I can't see https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=817706

Monica, please let me know if you need any more info.

Comment 87

[:Paolo Amadini]

Hi Manuela,

there are two different warning dialogs that may or may not appear when opening an executable file.

The first is implemented inside Firefox (attachment 817706 [details]). There is an about:config preference, controlled by the checkbox in the message, that suppresses it in all cases. The patch in this bug is not expected to affect this dialog at all. While testing, using a new Firefox profile every time should ensure that the preference is not affecting your expected result.

The second is implemented by the operating system. There is an operating-system level preference, controlled by the checkbox in the message, that might be able to suppress it for the Firefox application or in all cases (I'm not sure about the exact behavior on all Windows versions).

On a pristine Windows installation, the dialog is commonly expected to appear. However, if the file has been saved to a FAT file system instead of an NTFS one (for example, an USB stick), it is expected that the warning will not appear.

The issue solved by this bug is that the operating-system level warning did not appear even if an ".exe" file was saved to an NTFS file system on a pristine Windows installation. The issue did not happen in Firefox 23, and in Firefox 24 the issue will not be fixed. The patch in this bug should fix this in Firefox 25.

Hope this helps!

Comment 88

[Manuela Muntean [Away]]

Considering comment 87 describing the warning dialog implemented by the operating system, I've done the following testing with the following results:

With Firefox 25 beta 7 and 25 beta 8, on the following OS: Win 7 32-bit, Win 8 32-bit and Vista 64-bit:

1. Open http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b8-candidates/build1/win32/en-US/ and download an .exe file
2. After the download finishes, open the Downloads Panel and click on "Open Containing Folder" 
3. After the folder opens, double click the .exe file in order to install the build

Results: 

- with both 25b7 and 25b8, and on all 3 OSs mentioned above, I don't get https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=817706 nor any other warning

Comment 89

[[:mmc] Monica Chew (no longer reading bugmail)]

I don't have a windows machine, unfortunately.

Alice, do you have time to describe the tests to reproduce (that should hopefully work on FF25b8, FF23, and not FF24)?

Manuela, from comment 86 part B), I think the steps to reproduce must be somehow wrong, because FF23 and FF24 should not have the same behavior.

Thanks,
Monica

Comment 90

[Alice0775 White]

*It works as expected in latest Firefox25b8 , Aurora26.0a2 and Nightly27.0a1.
http://hg.mozilla.org/releases/mozilla-beta/rev/f0c589ed7642
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 ID:20131015052812
http://hg.mozilla.org/releases/mozilla-aurora/rev/7b18d2da3414
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20131017004002
http://hg.mozilla.org/mozilla-central/rev/423b9c30c73d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131017030201

*However, the problem still exists in Firefox esr24.0.
http://hg.mozilla.org/releases/mozilla-esr24/rev/c5fc2f003e4d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 ID:20130910201120

Comment 91

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

(In reply to Alice0775 White from comment #90)
> *However, the problem still exists in Firefox esr24.0.
> http://hg.mozilla.org/releases/mozilla-esr24/rev/c5fc2f003e4d
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0
> ID:20130910201120

That looks like an ESR24 release build, which wouldn't have included the backouts from this bug, right? I think you need to test a build from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-esr24/.

Comment 92

[:Paolo Amadini]

Attachment: Test Result - Security Warning

(In reply to Manuela Muntean [:Manuela] [QA] from comment #88)
> - with both 25b7 and 25b8, and on all 3 OSs mentioned above, I don't get
> https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=817706 nor any
> other warning

I have installed the following build on a Windows 7 virtual machine:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b9-candidates/build1/win32/en-US/Firefox%20Setup%2025.0b9.exe

I downloaded the file again using that build, then opened it from the Downloads Panel. I immediately got the security warning shown in this attachment.

Comment 93

[:Paolo Amadini]

Attachment: Test Result - File Properties

The file properties show the "Unblock" button as expected.

Comment 94

[:Paolo Amadini]

Attachment: Test Result - Drive Properties

The properties of the "C:" drive show that I'm on an NTFS file system.

Manuela, can you verify the properties of the saved file and the drive in
your test case where you did not see the warning?

Comment 95

[:Paolo Amadini]

Also, other system-wide settings may affect whether the security warning is shown, including whether the source domain is included in the "trusted sites". Ideally, for reproducible results, this should be tested on a newly installed operating system.

Comment 96

[Manuela Muntean [Away]]

(In reply to :Paolo Amadini from comment #94)
> Manuela, can you verify the properties of the saved file and the drive in
> your test case where you did not see the warning?

- on Vista 64-bit and Win 7 32-bit (for both OSs, I'm on a NTFS file system, like shown here: https://bugzilla.mozilla.org/attachment.cgi?id=818961)

a) with 25 beta 7

- after opening the downloaded file, I can't see the warning dialog: https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=818957
- I can't see the "Unblock" button: https://bugzilla.mozilla.org/attachment.cgi?id=818959

b) with 25 beta 8

- after opening the downloaded file, I can indeed see the warning dialog: https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=818957
- I can indeed see the "Unblock" button: https://bugzilla.mozilla.org/attachment.cgi?id=818959

c) with 25 beta 9

- after opening the downloaded file, I can indeed see the warning dialog: https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=818957
- I can indeed see the "Unblock" button: https://bugzilla.mozilla.org/attachment.cgi?id=818959


- on Win 8 32-bit (I'm on a NTFS file system, like shown here: https://bugzilla.mozilla.org/attachment.cgi?id=818961)

a) with 25 beta 7, 25 beta 8 and 25 beta 9

- after opening the downloaded file, I can't see the warning dialog: https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=818957
- I can't see the "Unblock" button: https://bugzilla.mozilla.org/attachment.cgi?id=818959

Comment 97

[:Paolo Amadini]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #96)
> - on Win 8 32-bit
> 
> a) with 25 beta 7, 25 beta 8 and 25 beta 9
> 
> - after opening the downloaded file, I can't see the warning dialog
> - I can't see the "Unblock" button

I've no idea whether anything changed in the default configuration of Windows 8 compared to Windows 7 to justify this behavior. Is this the same behavior as Firefox 23?

Comment 98

[Manuela Muntean [Away]]

> I've no idea whether anything changed in the default configuration of
> Windows 8 compared to Windows 7 to justify this behavior. Is this the same
> behavior as Firefox 23?

Yes, the same behavior with Firefox: 23, 23.0.1, 24.

- on Win 8 32-bit (I'm on a NTFS file system, like shown here: https://bugzilla.mozilla.org/attachment.cgi?id=818961)

a) with Firefox 23, Firefox 23.0.1 and Firefox 24

- after opening the downloaded file, I can't see the warning dialog: https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=818957
- I can't see the "Unblock" button: https://bugzilla.mozilla.org/attachment.cgi?id=818959

Comment 99

[:Paolo Amadini]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #98)
> Yes, the same behavior with Firefox: 23, 23.0.1, 24.

Based on comment 96 and comment 98, do you think this is enough to consider the fix for this specific regression verified in Firefox 25 beta 8?

Can you test the latest ESR 24 build (from the branch that will be released) on Windows 7 as well?

Comment 100

[Masatoshi Kimura [:emk]]

I think the Windows 8 issue should be filed as a separate bug.

Comment 101

[Manuela Muntean [Away]]

(In reply to Masatoshi Kimura [:emk] from comment #100)
> I think the Windows 8 issue should be filed as a separate bug.

I've logged bug 928956 for this issue.

Comment 102

[u279076]

Can we call this verified fixed based on previous testing, in spite of bug 928956?

Comment 103

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #102)
> Can we call this verified fixed based on previous testing, in spite of bug
> 928956?

Yes. This bug is for tracking a regression caused by http://hg.mozilla.org/integration/mozilla-inbound/rev/b6cce1e41253. Bug 928956 is not a regression.

Comment 104

[Masatoshi Kimura [:emk]]

Note for the future QA:
It is unreliable to open the downloaded file to see the warning, because sometimes warnings will be omitted on Windows 8 or later. See bug 928956 for details.
Please see the file properties and find the "Unblock" button *without* opening the file (once the file is opened, the mark may be silently removed).

Comment 105

[:Paolo Amadini]

This is now pending verification for the ESR 24 branch (though I expect the bug to have been correctly fixed there as well).

Comment 106

[Matt Wobensmith [:mwobensmith][:matt:]]

Ioana/Manuela - can we look at ESR 24 to see if it's fixed there? Thank you.

Comment 107

[Manuela Muntean [Away]]

Verified as fixed with Firefox 24.1.0 ESR, on Win 7 64-bit and Vista 64-bit:

- after opening the downloaded file, I can see the warning dialog: https://bug916126.bugzilla.mozilla.org/attachment.cgi?id=818957
- I can also see the "Unblock" button, while checking the properties of the downloaded file: https://bugzilla.mozilla.org/attachment.cgi?id=818959

Comment 108

[Igor]

This fix is causing a regression (will undo bug 499448):

On my system I have set

[HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Policies\Attachments]
"SaveZoneInformation"=dword:00000001

But now Firefox is marking all downloaded executable files and don't honor the set Zone.Identifier setting.

Tested with: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0


I posted more information on bug 909022 which is causing this change.

Comment 109

[Anonymous]

A recent Firefox 26 beta has regressed.  Prior to this, I had browser.download.manager.scanWhenDone set to False to prevent Firefox from adding any information in alternate data streams.  I do not want executables marked in any way.  Now the setting is ignored, and Firefox always adds this noxious Zone Identifier nonsense to every file downloaded, even though I don't want it.

Whatever you did, undo it.  I am not a child, I know what I'm doing.  I don't need some Play-Skool kiddie browser getting in my way.

Comment 110

[u279076]

(In reply to Anonymous from comment #109)
> A recent Firefox 26 beta has regressed.  

Please file a new bug report and provide steps to reproduce along with the version number where this first occurred.

Please also familiarize yourself with our Bugzilla guidelines.
* https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
* https://developer.mozilla.org/en/Bug_writing_guidelines

Comment 111

[Anonymous]

Are you honestly telling me you expect regular contributors to perform bisections to determine the first version where a bug regressed?  I'm using Firefox 26.0b3.  I don't know when it first started occurring, but this is the first version where I saw it.

Steps to reproduce:

1. set browser.download.manager.scanWhenDone to False
2. download any .exe file
3. run Sysinternals' streams.exe on the file and see the line ":Zone.Identifier:$DATA" which should not be there.

Comment 112

[Igor]

Calm down. This will not change. See this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=909022#c11

You also should be able to set the new preference...

Comment 113

[u279076]

(In reply to Anonymous from comment #111)
> Are you honestly telling me you expect regular contributors to perform
> bisections to determine the first version where a bug regressed?

I have not asked you do to a bisect. I've asked you to file a new bug report and do some testing of other Firefox versions (freely available on our FTP server) to see if it reproduces. It's common practice for open source projects to seek out the assistance of reporters in the investigation of their issues. If you're unsure about how to do what we're asking of you then we'll be more than happy to teach you.

That said, what you are describing is not a bug. As Igor points out this is intended behaviour. If you think we've unintentionally introduced a new bug then please file a new bug report. If you just disagree with the decision we've made and wish for it to be reverted then please post your concerns on the mialing lists. Commenting further in *this* bug report will get you no closer to a solution for your problem.

If this seems unreasonable to you then I'm sorry, but that's how this process works.

Comment 114

[Masatoshi Kimura [:emk]]

I'm surprised noboby filed a bug about the regression after all... I've filed bug 952961