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.

This bug is also present in the Firefox 24.0 release.

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.

I wonder if the fix for bug 858234 caused this?

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)

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.

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.

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.

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

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

(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?

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

Is the virus scan executed on Nightly?

(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

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?

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

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?

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

(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?

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?

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.

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

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.

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.

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

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?

26/27 unaffected per comment 8/comment 21

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)

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?

(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?

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

Can one of the affected folks try a build from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mchew@mozilla.com-dfc8e1257a55/try-win32/ http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mchew@mozilla.com-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.

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

(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/mchew@mozilla.com-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

Thanks so much Alice! Do you have time to see if one of http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mchew@mozilla.com-828a4a6efa90/try-win32/ http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mchew@mozilla.com-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

(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/mchew@mozilla.com- > 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 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

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

(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?

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.

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.

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

(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 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 on attachment 813207 [details] [diff] [review] Backout changes to ExecuteDesiredAction only in the download manager ( Thanks for clarifying Monica!

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

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

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

(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

(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?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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?

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.

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

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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

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.

(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?

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

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

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.

(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?

Hoping either of you can sanity check this.

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.

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

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.

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

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

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.

The IDL change seems to have somehow fixed the oranges? https://tbpl.mozilla.org/?tree=Try&rev=f969fb910d25

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

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

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!

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.

(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).

(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?

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

> 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

> 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/mchew@mozilla.com-828a4a6efa90/try-win32/ doesn't exist anymore. I get the 404 Page Not Found Error.

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.

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!

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

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

*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

(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/.

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

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

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?

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.

(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

(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?

> 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

(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?

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

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

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

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

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

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

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

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

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.

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.

(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

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.

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

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

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