The default bug view has changed. See this FAQ.

Use IAttachmentExecute antivirus API when it is available

VERIFIED FIXED in mozilla1.9beta4

Status

()

Toolkit
Downloads API
P1
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: stephend, Assigned: jimm)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla1.9beta4
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.9 +
in-testsuite -
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
See bug 393305 for background info.

In a nutshell, at least the following vendors have ceased to implement the IOfficeAntiVirus interface:

Norton 2008 (AntiVirus and Internet Security)
McAfee VirusScan Plus 2008
Panda AntiVirus 2008

There are likely others; AVG and Windows Defender *DO* implement this API in their latest versions.

This bug was borne out of https://bugzilla.mozilla.org/show_bug.cgi?id=393305#c11 and https://bugzilla.mozilla.org/show_bug.cgi?id=393305#c13.
Flags: blocking-firefox3?

Comment 1

9 years ago
I just thought I'd add that this may not require an API implementation at all. Some software allows you to enter a command line execution path which most anti-virus software supports, when the download finishes the command is then executed and the file is scanned by the anti-virus software.

This isn't really helpful, there may not be any feedback from the call and it would require knowledge of the anti-virus software in order to determine the information to be used but its better than nothing if all else fails.
(In reply to comment #1)
We don't want to do that though because that's vendor specific, while using a windows API is just vendor agnostic.

Comment 3

9 years ago
That is true and would require the user to be relatively advanced, in knowing how to change the settings look up the information and so on. I'm sure that some products do this only because they feel that there are no other sure alternatives, hopefully this will not be the case.
Well, any virus scanner that's worth anything should be scanning every file on your system anyway.  This just ensures that it's getting done...
(Assignee)

Comment 5

9 years ago
Created attachment 293139 [details]
eicar virus signature file

My guess is that these vendors are tapped into the system in other ways in which files firefox downloads are still being protected. I'd bet McAfee and Norton are latched into various places - at the firewall level, through hooks like a shell execute hook, etc.. Attached is a virus signature file that should trigger these protections when hit on.
FWIW I currently get the following behaviour downloading the example signature file:

AV: F Secure Internet Security 2007
Browser: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007121405 Minefield/3.0b3pre ID:2007121405

Firefox displays:
"C:\Users\Jaime\Desktop\eicar.com could not be saved, because you cannot change the contents of that folder.

Change the folder properties and try again, or try saving in a different location."

F-Secure displays "Virus Detected" and offers to Disinfect, Delete etc..

I have windows Defender installed.

So currently with Firefox and F-Secure I am protected however the message from the browser is slightly confusing.

With IE 7 I get the Virus Detected alert *before* I select if I want to Run/Save/Cancel the download. I am still able to select save however it needs elevated privileges and after elevation I was not able to save it. So IE 7 is a bit more confusing than Firefox.
I don't think we can block on something when we don't know if there's any reasonable solution. I think we're doing the right thing by supporting IOfficeAntiVirus, and we definitely want to figure out what the '08 applications are doing, but let's treat that opportunistically; seems like, for now, they are still doing the scans.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
What about using IAttachmentExecute when it's aviailable and enumerating IOfficeAntiVirus manually when IAttachmentExecute is unavailabe?
IAttachmentExecute will enumerate IOfficeAntiVirus on our behalf.
IE6SP2 and IE7 seem to use only IAttachmentExecute. They don't care about IOfficeAntiVirus because they support only WinXPSP2 or later. But we will have to continue to keep manual enumerating path unless we drop the Win2k support.
I think supporting IAttachmentExecute if available is the right solution. It doesn't seem terribly difficult to implement and it maintains compatability with Win2k and XP pre sp2.
(Assignee)

Updated

9 years ago
Blocks: 103487
Created attachment 299617 [details] [diff] [review]
implementing IAttachmentExecute
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #299617 - Flags: review?(sdwilsh)
Duplicate of this bug: 236771
Comment on attachment 299617 [details] [diff] [review]
implementing IAttachmentExecute

>+  // At present all Mozilla products use GUID form
>+  UuidFromString((unsigned char *)id.get(), &mClientGUID);

Well, yes and no... all "Mozilla" products, yes.

But not all products based on Mozilla, (songbird for instance has songbird@songbirdnest.com as their id)

We have to think about more than just Mozilla products, as we can't predict other users as well.

Also is the dependancy on rpcrt4 really needed, I don't see it mentioned in the msdn docs I was looking at?

(I don't know enough about IAE to provide comment on the rest of your patch though)

Updated

9 years ago
Attachment #299617 - Flags: review?(jmathies)
Jim should review this first since I'm not a windows guy.
(In reply to comment #12)
> We have to think about more than just Mozilla products, as we can't predict
> other users as well.
Hm, how can I get a product-by-product GUID?
Do I have to add a new field to application.ini?
> Also is the dependancy on rpcrt4 really needed, I don't see it mentioned in the
> msdn docs I was looking at?
UuidFromString() requires rpcrt4.
But it will not be required anymore because I found nsID::Parse().
(Assignee)

Comment 15

9 years ago
The more I read about this interface the more I like the idea of supporting it. (Though I don't think it'll tap us into the 2008 products that dropped support for IOfficeAntiVirus - those I believe are handling scanning on their own. We'll know more about that hopfully in a bit re bug 412204.)  It does though give us better integration with the os's underlying security mechanisms for file downloads. For example our downloads would start supporting the security prompts you see on downloads from browsers like IE.

I'm going to run this a bit and see what the various side effects are. Will post back once I've had a chance to play with it a bit.

One note - in light of some of the feedback we've received (which is still under discussion) regarding virus scanning some users may not appreciate this kind of support.    

  
Comment on attachment 299617 [details] [diff] [review]
implementing IAttachmentExecute

>+PRBool
>+nsDownloadScanner::IsAESAvailable()
>+{
>+  HRESULT hr;
>+  nsRefPtr<IAttachmentExecute> ae;
>+  hr = CoCreateInstance(CLSID_AttachmentServices, NULL, CLSCTX_INPROC,
>+                        IID_IAttachmentExecute, (void**)&ae);

This will leak the COM object. Following the example in DoScanOAV, use getter_addRefs(ae) instead of (void**)&ae.

> void
>-nsDownloadScanner::Scan::DoScan()
>+nsDownloadScanner::Scan::DoScanAES()
>+{
>+  HRESULT hr;
>+  nsRefPtr<IAttachmentExecute> ae;
>+  hr = CoCreateInstance(CLSID_AttachmentServices, NULL, CLSCTX_INPROC,
>+                        IID_IAttachmentExecute, (void**)&ae);

Same comment as above.

>     nsString mPath;
>     nsString mName;
>+    GUID mClientGUID;
>     nsString mOrigin;
>     // Also true if it is an ftp download
>     PRBool mIsHttpDownload;
>     PRBool mIsReadOnlyRequest;

I see mClientGUID defined in the Scan class, but you initialize it as if it were part of the nsDownloadScanner class in ListCLSID:

> +  // Get the app id for IAES Client ID
> +  nsCAutoString id;
> +  rv = appinfo->GetID(id);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  // Remove curly braces
> +  id.Cut(0, 1);
> +  id.Truncate(id.Length() - 1);
> +  // At present all Mozilla products use GUID form
> +  UuidFromString((unsigned char *)id.get(), &mClientGUID);

This initialization should go in the Scan constructor where all the other members of the Scan class are initialized.
Comment on attachment 299617 [details] [diff] [review]
implementing IAttachmentExecute

r=sdwilsh for a very general review.  I don't know windows bits though, so this needs r from jimm or robarnold before it can land.
Attachment #299617 - Flags: review?(tellrob)
Attachment #299617 - Flags: review?(sdwilsh)
Attachment #299617 - Flags: review+
Also - how soon do you think you could get a new patch?  We have a code freeze for beta 3 tomorrow, and I'd *really* like to get this in if it turns out to be nice.
(Assignee)

Comment 19

9 years ago
Masatoshi - we're thinking we want to get this in before the freeze tonight, so we'll go ahead and start on the needed changes. If you're online, please post back and let us know if your actively working on this. 
(Assignee)

Comment 20

9 years ago
>>+  // At present all Mozilla products use GUID form
>>+  UuidFromString((unsigned char *)id.get(), &mClientGUID);
>>
>Well, yes and no... all "Mozilla" products, yes.
>
>But not all products based on Mozilla, (songbird for instance has
>songbird@songbirdnest.com as their id)
>
>We have to think about more than just Mozilla products, as we can't predict
>other users as well.

The GUID settings apply to prompts. 

http://msdn2.microsoft.com/en-us/library/bb776301.aspx

I'd say define a generic guid here, which means applications would all share the same prompt settings. If a particular mozilla app wanted to customize that for themselves, they could override the default.

(Assignee)

Comment 21

9 years ago
Created attachment 300116 [details] [diff] [review]
IAttachExecute patch v.2
Attachment #299617 - Attachment is obsolete: true
Attachment #299617 - Flags: review?(tellrob)
Attachment #299617 - Flags: review?(jmathies)
(Assignee)

Comment 22

9 years ago
A couple notes on this - 

On the prompt GUID, I just hardcoded it here and commented it explaining it's purpose. We could pull that from someplace else although I'd rather spin that out to another bug than hold this up.

Save on IAE does not give "dirty" results. It either fails or it succeeds. The user is promped by their anti-virus software, so they know the file was dirty. Although with the way I've set this up our download manager will not display anything specific to this. We could change the final else on the result to AVSCAN_UGLY, however, there is the risk that if the scan fails for some other reason we would incorrectly report it as a virus.

As it stands though our virus warning UI will not show up with this patch. 

Comments welcome on both.
Comment on attachment 300116 [details] [diff] [review]
IAttachExecute patch v.2

>+// IAttachementExecute supports user definable settings for certain
>+// security related prompts. This defines a general GUID for use in
>+// all projects. Individual projects can define an individual guid
>+// if they want to.
>+static const GUID GUID_MozillaVirusScannerPromptGeneric = 
>+{ 0xb50563d1, 0x16b6, 0x43c2, { 0xa6, 0x6a, 0xfa, 0xe6, 0xd2, 0x11, 0xf2, 0xea } };
How would they define this?  Wouldn't it be best to have something like this?

#ifndef MOZ_VIRUS_SCANNER_PROMPT_GUID
#define MOZ_VIRUS_SCANNER_PROMPT_GUID \
  { 0xb50563d1, 0x16b6, 0x43c2, { 0xa6, 0x6a, 0xfa, 0xe6, 0xd2, 0x11, 0xf2, 0xea } }
#endif
static const GUID GUID_MozillaVirusScannerPromptGeneric =
  MOZ_VIRUS_SCANNER_PROMPT_GUID;

this way they can actually specify their own :)

(In reply to comment #22)
> Save on IAE does not give "dirty" results. It either fails or it succeeds. The
> user is promped by their anti-virus software, so they know the file was dirty.
> Although with the way I've set this up our download manager will not display
> anything specific to this. We could change the final else on the result to
> AVSCAN_UGLY, however, there is the risk that if the scan fails for some other
> reason we would incorrectly report it as a virus.
Talking to UX folks on this RSN.

> As it stands though our virus warning UI will not show up with this patch. 
It returns Failed if it can't clean it too, right?

r=sdwilsh otherwise (rob has comments that he's adding to the bug too)
Attachment #300116 - Flags: review+
This patch will bitrot my patch for bug 409815, so in the interest of making my life easier (and also for style reasons), could you eliminate the multiple return statements in DoScanAES? The error handling in but 409185 is now more complicated than just setting mStatus to AVSCAN_FAILED.

> As it stands though our virus warning UI will not show up with this patch.
I would hope that we can throw up some sort of UI for this if the scan fails, but I suppose we need some QA data to know how often this would happen.
(In reply to comment #23)
> Talking to UX folks on this RSN.
let's set it to dirty

Updated

9 years ago
Summary: Figure out which AV/file-scanning API to implement, since IOfficeAntiVirus isn't universally supported → Use IAttachmentExecute when it is available

Updated

9 years ago
Priority: -- → P1
Whiteboard: [needs new patch]
Target Milestone: --- → Firefox 3 beta3
Sorry for the late response. I'll be offline again for a while. Feel free to modify the patch to land on Fx3 beta 3.
> *  * Use all available virus scanners instead of just the first one that is
> *    enumerated (or use some heuristic)
I thought this is fixed by bug 393305. Is it still valid?
Assignee: VYV03354 → jmathies
Status: ASSIGNED → NEW
(In reply to comment #26)
> > *  * Use all available virus scanners instead of just the first one that is
> > *    enumerated (or use some heuristic)
> I thought this is fixed by bug 393305. Is it still valid?
> 

It's invalid and should be taken out
(Assignee)

Comment 28

9 years ago
>I thought this is fixed by bug 393305. Is it still valid?

yep, sorry missed that when applying your patch manually.

>This patch will bitrot my patch for bug 409815, so in the interest of making my
>life easier (and also for style reasons), could you eliminate the multiple
>return statements in DoScanAES?

will do.

>How would they define this?  Wouldn't it be best to have something like this?

wfm, i'll add it.

>let's set it to dirty

done!
(Assignee)

Comment 29

9 years ago
Created attachment 300144 [details] [diff] [review]
IAttachExecute patch v.3
Attachment #300116 - Attachment is obsolete: true
Comment on attachment 300144 [details] [diff] [review]
IAttachExecute patch v.3

>+  if (SUCCEEDED(hr)) {
>+    ae->SetClientGuid(GUID_MozillaVirusScannerPromptGeneric);
>+    ae->SetLocalPath(mPath.BeginWriting());
>+    ae->SetSource(mOrigin.BeginWriting());
nit: do these return something?  if so, and you want to ignore them, please cast to void.

r=sdwilsh with that nit fixed.  Get a new patch up so we can get approval to land this :)
Attachment #300144 - Flags: review+
(Assignee)

Comment 31

9 years ago
Created attachment 300151 [details] [diff] [review]
IAttachExecute patch v.4

If any of those fail, Save would fail so I don't see any reason to check those results.
Attachment #300144 - Attachment is obsolete: true
Comment on attachment 300151 [details] [diff] [review]
IAttachExecute patch v.4

r=sdwilsh

Drivers - this change is a fairly local change that gives us a big platform integration win on XP SP2 and greater.  It only affects windows code.
Attachment #300151 - Flags: review+
Attachment #300151 - Flags: approval1.9?
This looks good to me. It shouldn't cause any serious performance regressions (it may even be faster in a few cases). One possible downside, however, is that we may no longer be invoking Windows Defender on downloaded files. I don't know a good way to test this however since Defender does not raise any warning for the eicar.com test file.
Comment on attachment 300151 [details] [diff] [review]
IAttachExecute patch v.4

a1.9+=damons
Attachment #300151 - Flags: approval1.9? → approval1.9+

Updated

9 years ago
Whiteboard: [needs new patch] → [has patch][has reviews][can land]
(Assignee)

Comment 35

9 years ago
>One possible downside, however, is that
>we may no longer be invoking Windows Defender on downloaded files. I don't know
>a good way to test this however since Defender does not raise any warning for
>the eicar.com test file.

I can trigger that by pulling the file from a different location. I think once you hit "ignore" in WD's dialog it remembers you aren't worried about it. In general, WD is getting called, I've been testing with it today.
Thanks for the work Masatoshi and Jim on getting this in for the beta!

Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
new revision: 1.8; previous revision: 1.7
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
new revision: 1.3; previous revision: 1.2
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite-
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][can land]

Updated

9 years ago
Summary: Use IAttachmentExecute when it is available → Use IAttachmentExecute antivirus API when it is available
Windows tinderboxen no like

Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
new revision: 1.9; previous revision: 1.8
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
new revision: 1.4; previous revision: 1.3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 38

9 years ago
Created attachment 300246 [details] [diff] [review]
IAttachExecute patch v.5

waiting on try server results to confirm, but I believe this should do it.
Attachment #300151 - Attachment is obsolete: true

Updated

9 years ago
Attachment #300246 - Flags: approval1.9+
Checking in toolkit/components/downloads/src/AttachmentServices.h;
initial revision: 1.1
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
new revision: 1.10; previous revision: 1.9
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
new revision: 1.5; previous revision: 1.4
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Boy it sure is annoying that try server worked and tinderbox didn't....

Removing toolkit/components/downloads/src/AttachmentServices.h;
new revision: delete; previous revision: 1.1
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
new revision: 1.11; previous revision: 1.10
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
new revision: 1.6; previous revision: 1.5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 41

9 years ago
Just a recap for future reference - 

First burn was related to the IAttachmentExecute interface being defined in the 6.0 SDK but not on what is currently used in WINNT 5.2 fx-win32-tbox Dep Nightly. 

The second patch ifdef'd in the interface in cases where it wasn't defined. This ran through try server fine, but when it landed on fx-win32-tbox, the consts  broke the build due to multiple definitions. <- This I want to try and track down at some point, these shouldn't have been defined. 

The next patch will mangle the names of the two consts so they don't conflict. Unfortunately all this insures this patch isn't going to make the freeze. :/    
(Assignee)

Comment 42

9 years ago
Related - 
https://bugzilla.mozilla.org/show_bug.cgi?id=414778
(Assignee)

Comment 43

9 years ago
Created attachment 300295 [details] [diff] [review]
IAttachExecute patch v.6

The same patch, I've just mangled the the names so there's no chance of a conflict. This just passed try server as well.
Attachment #300246 - Attachment is obsolete: true

Comment 44

9 years ago
Any chance of also using the SetSource/SetReferrer methods to tell Explorer where the file came from? That way you (should) get the nice warnings when you open the files through explorer.
We do have that information available to us (via nsDownload), so if we can, we probably should.
(Assignee)

Comment 46

9 years ago
We already set the source. Referrer isn't needed - from msdn:

IAttachmentExecute::SetReferrer and IAttachmentExecute::SetSource have similar functionality. If both are set, the least-trusted zone of the two is used.

IAttachmentExecute::SetReferrer is used by container files to indicate indirect inheritance and avoid zone elevation. It can also be used with shortcut files to limit elevation based on parameters.

Calling IAttachmentExecute::SetReferrer is optional.

IAttachmentExecute::SetReferrer is only used to determine the security zone and its associated policies.
well, if the referrer is less trusted than the source, wouldn't we want to set the referrer?
re-requesting blocking for this since I think we *really* want it.
Flags: blocking-firefox3- → blocking-firefox3?
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
(In reply to comment #41)
You should not define IID and CLSID by yourself. They are already built into uuid.lib. Just use an external declaration.
And IAttachmentExecute is defined in shobjidl.h which is included from shlobj.h even in 5.2 SDK headers. I'm not sure why fx-win32-tbox burned.

(In reply to comment #47)
> well, if the referrer is less trusted than the source, wouldn't we want to set
> the referrer?
In short:
SetSource is only for archivers such as zip folder. That's none of DM's business.
Flags: blocking-firefox3?
Target Milestone: Firefox 3 beta4 → Firefox 3 beta3
Oops.
> SetSource is only for archivers
SetReferrer is only for archivers
(Assignee)

Comment 51

9 years ago
>And IAttachmentExecute is defined in shobjidl.h which is included from shlobj.h
>even in 5.2 SDK headers. I'm not sure why fx-win32-tbox burned.

That was my original assumption as well, but we wanted it in. In the end we still didn't make it. I have a to-do item to try and find out why it wasn't defined.

Updated

9 years ago
Flags: blocking-firefox3?
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
(Assignee)

Comment 52

9 years ago
Ok, the original patch - 

IAttachExecute patch v.4

just went through try server just fine.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1201731026.1201733699.25664.gz

yet it broke WINNT 5.2 fx-win32-tbox Depend Nightly last night - 

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1201653660.1201654666.25187.gz

Spoke to build, they couldn't find the cause. I think we should just stick with
the final patch.

I'll do a final release / static build of this to make sure it's working there
too.
Created attachment 301652 [details] [diff] [review]
 IAttachExecute patch v.7

OK, I realized what's going on.
SDK6 headers #ifdef'ed IAttachmentExecute definitions, but MSVS8 headers did't.
TryServer includes MSVS8 headers first, but real tinderbox includes SDK6 headers first.
I could reproduce the problem by modifying include order.
I bet this will fix the tinderbox burning.
Attachment #301652 - Flags: review?(jmathies)

Updated

9 years ago
Whiteboard: [has patch][needs review jimm]
(Assignee)

Comment 54

9 years ago
Hey Masatoshi -

>TryServer includes MSVS8 headers first, but real tinderbox includes SDK6
>headers first.

Can you fill me in on where/how you came to this conclusion? If so, we should fix the try server so that it matches tinderbox. 
(Assignee)

Comment 55

9 years ago
> I could reproduce the problem by modifying include order.

Sorry, wasn't paying attention. I'll take a look.
(Assignee)

Comment 56

9 years ago
You're right they ifdef'd a bunch of junk out.

Just spoke with build, try server had a malformed include string-

d:\sdks\v6.0\binD:\sdks\v6.0\include;D:\sdks\v6.0\include\atl;D:\msvs8\VC\ATLMFC\INCLUDE;D:\msvs8\VC\INCLUDE;D:\msvs8\VC\PlatformSDK\include

it's been fixed.

I'm still waiting on some feedback on including these com headers. Once I get the final word on that we can decide on the right patch.
Flags: blocking-firefox3? → blocking-firefox3+
(Assignee)

Updated

9 years ago
Attachment #301652 - Flags: review?(jmathies) → review+
Jim - does your review mean that we are OK with the patch as is (without the extra headers)?
(Assignee)

Comment 58

9 years ago
yeah we could go ahead and land it. if we get a ton of complaints we can add that header stuff once we get the ok on it. As things stand now this is the right way to do it.
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review jimm]

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [has patch][has reviews][can land]
(In reply to comment #23)
> (From update of attachment 300116 [details] [diff] [review])
> >+// IAttachementExecute supports user definable settings for certain
> >+// security related prompts. This defines a general GUID for use in
> >+// all projects. Individual projects can define an individual guid
> >+// if they want to.
> >+static const GUID GUID_MozillaVirusScannerPromptGeneric = 
> >+{ 0xb50563d1, 0x16b6, 0x43c2, { 0xa6, 0x6a, 0xfa, 0xe6, 0xd2, 0x11, 0xf2, 0xea } };
> How would they define this?  Wouldn't it be best to have something like this?
> 
> #ifndef MOZ_VIRUS_SCANNER_PROMPT_GUID
> #define MOZ_VIRUS_SCANNER_PROMPT_GUID \
>   { 0xb50563d1, 0x16b6, 0x43c2, { 0xa6, 0x6a, 0xfa, 0xe6, 0xd2, 0x11, 0xf2,
> 0xea } }
> #endif
> static const GUID GUID_MozillaVirusScannerPromptGeneric =
>   MOZ_VIRUS_SCANNER_PROMPT_GUID;
> 
> this way they can actually specify their own :)

maybe we could move the stringified guid to a pref (someday)
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp,v  <--  nsDownloadScanner.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h,v  <--  nsDownloadScanner.h
new revision: 1.7; previous revision: 1.6
done
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][can land]

Updated

9 years ago
Depends on: 416501

Comment 61

9 years ago
This seems to have caused bug 416501

Comment 62

9 years ago
Could this have caused bug 416683?
Depends on: 416683
Building with VC7.1, I repeatably cannot finish the compile.  IAttachmentExecute is undeclared...
(Assignee)

Comment 64

9 years ago
Alex, are you working with the latest SDK? Our build environment was recently upgraded to the Vista SDK, so code is now landing that is not compatible with previous SDKs.

Also, can you paste in your compile errors?
Created attachment 304622 [details]
compile errors, VC7.1, WinXP
(Assignee)

Comment 66

9 years ago
an upgrade to the latest sdk should fix that. make sure the sdk include, bin, and lib folders are first in your exports.
I saw the same errors, and upgrading to the "Windows Server 2003 R2 Platform SDK" available at  http://www.microsoft.com/downloads/details.aspx?FamilyId=0BAF2B35-C656-4969-ACE8-E4C0C0716ADB&displaylang=en fixed them for me.
(Reporter)

Comment 68

9 years ago
Verified FIXED (functionality-wise), per my testing in https://bugzilla.mozilla.org/show_bug.cgi?id=416501#c11 with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre

-and-

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre
Status: RESOLVED → VERIFIED
Depends on: 422920
(Assignee)

Comment 69

9 years ago
Just noticed something while working on bug 420230 - 

  nsCOMPtr<nsIURI> innerURI = NS_GetInnermostURI(uri);
  (void)innerURI->SchemeIs("http", &isHttp);
  (void)innerURI->SchemeIs("ftp", &isFtp);
  (void)innerURI->SchemeIs("https", &isHttps);

NS_GetInnermostURI can fail and return nsnull but I'm not sure if that would ever happen. Why do we use NS_GetInnermostURI here?

(In reply to comment #69)
> NS_GetInnermostURI can fail and return nsnull but I'm not sure if that would
> ever happen. Why do we use NS_GetInnermostURI here?
We should have a null check there.  Edward added this, and there was a good reason, but I cannot recall why - check blame for the bug (we should probably add a comment there as to why as well).
(Assignee)

Updated

9 years ago
Depends on: 425946
(Assignee)

Updated

9 years ago
Blocks: 443215
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.