Closed Bug 837199 Opened 11 years ago Closed 11 years ago

write an interface to query the application reputation service

Categories

(Toolkit :: Downloads API, defect)

Product:
Toolkit
Component:
Downloads API
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: mmc, Assigned: mmc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 16 obsolete files)

Write interface to query application reputation
325.37 KB, patch
Paolo
: review+
Details | Diff | Splinter Review 
For tracking changes to nsDownloadManager to support application reputation.
Assignee: nobody → mmc
Blocks: downloadprotection
You may be interested in bug 835890 as well.
Oops, wrong bug, ignore last comment!
This is equivalent to Chrome's CheckClientDownloadRequest here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/safe_browsing/download_protection_service.cc&q=checkclientdownload&l=809

There are other parts to this protocol, this is only the wire portion. I am filing bugs for the rest.
I am restarting work on this on Monday.
It seems the codepath for downloading files has changed significantly in the last couple months. I'm not seeing where nsDownloadManager is used at all.

Paolo, do you know why this line does not assert on single-click downloads? I've been testing on http://testsafebrowsing.appspot.com/.
Flags: needinfo?(paolo.mozmail)
Please ignore that comment, I think it's because enable-debug is turned off.
Flags: needinfo?(paolo.mozmail)
Even with enable-debug, I am not hitting the assertion when nsDownloadManager is initialized.
I have a somewhat working patch (hashes are passed to nsDownloadManager and query reputation API is working) but am running into a problem. Currently, the codepath is broken for downloads with end state DOWNLOAD_DIRTY, the existing state for dirty files according to windows virus scanners.

The code goes:
1) nsExternalAppHelperService invokes backgroundfilesaver to save the file
2) nsExternalAppHelperService::OnSaveComplete gets called *before* nsExternalAppHelperService::InitializeDownload, which calls the query reputation service in nsDownloadManager
3) OnSaveComplete calls ExecuteDesiredAction which may goes ahead and saves the file to disk before nsDownloadManager can reach a dirty state and cancel the download.

Additionally there are two codepaths for ExecuteDesiredAction -- both nsExternalHelperAppService and nsDownloadManager call this, depending on whether or not the download was resumed.

I need to track down all the calls to ExecuteDesiredAction and see if we can pass this functionality to nsDownloadManager instead, which should simplify things somewhat.
The other option would be to delete the file in nsDownloadManager upon reaching a dirty state. This leaves malware in ~/Downloads for a short period of time but may be easier than trying to consolidate ExecuteDesiredAction codepaths, and in any case the downloaded malware must exist on disk for a period of time before the call to the malware reputation service returns.
Paolo, do you have an opinion on the right way to fix this? I am close to a patch for the second option (deleting the dirty file after reaching a dirty state) so we could wait to evaluate that.
Flags: needinfo?(paolo.mozmail)
No, deleting the file after reaching a dirty state is the wrong fix. I don't want to make this code any worse. The description in comment 8 is a little wrong: when the dialog for "Save file to disk?" comes up that runs CreateProgressListener -> SetupProgressListener -> ExecuteDesiredAction if the file is already done downloading, which is going to be true frequently for small files. And in any case, we don't want users to be able to launch applications before the malware query returns, either.
(In reply to Monica Chew [:mmc] from comment #8)
> I need to track down all the calls to ExecuteDesiredAction and see if we can
> pass this functionality to nsDownloadManager instead, which should simplify
> things somewhat.

I think this is the right thing to do. Once upon a time we used to support builds
where the code in exthandler could be compiled without nsDownloadManager, but this
is no longer the case so we should try to move the action execution there, which
is also consistent with the direction of the new JavaScript API.

As you've noticed, the difficult part here is figuring out all the code paths,
but you may be able to simplify things under the assumption that we are always
able to create an nsITransfer (that is, the call in CreateProgressListener
succeeds, except for rare out of memory circumstances, for which we don't care
whether the action is actually executed).

Almost all the code paths should go through an nsITransfer creation, as soon as
the action selection dialog (if any) is closed and the network request has
stopped successfully. In ExecuteDesiredAction, the OnStateChange call triggers
the logic for the download to move to the completed state, and this will happen
asynchronously based on the logic in nsDownloadManager. This may wait for the
reply from the reputation service as needed.

The only exception may be opening from "file://", for which we may not want to
create an nsITransfer, neither do any additional malware check.
Flags: needinfo?(paolo.mozmail)
Thanks Paolo. I got further over the weekend but maybe in the wrong direction, moving the query check to nsExternalAppHandler because of threading issues -- but I will try moving it back.

If nsDownloadManager is responsible for launching the application, it's not clear to me what the point of having nsExternalAppHandler is... but I'll try going that way.

Thanks,
Monica
(In reply to Monica Chew [:mmc] from comment #13)
> Thanks Paolo. I got further over the weekend but maybe in the wrong
> direction, moving the query check to nsExternalAppHandler because of
> threading issues -- but I will try moving it back.

Sorry for the late reply here, I was traveling. Hope it wasn't too much work.

> If nsDownloadManager is responsible for launching the application, it's not
> clear to me what the point of having nsExternalAppHandler is... but I'll try
> going that way.

It still helps in selecting which application to run for a given file type.
But in the long term, we may even want to move that role somewhere else as well.
(In reply to Paolo Amadini [:paolo] from comment #14)
> (In reply to Monica Chew [:mmc] from comment #13)
> > Thanks Paolo. I got further over the weekend but maybe in the wrong
> > direction, moving the query check to nsExternalAppHandler because of
> > threading issues -- but I will try moving it back.
> 
> Sorry for the late reply here, I was traveling. Hope it wasn't too much work.

Not a problem, I fully expect to write everything at least 2 times :)
Attached patch Query application reputation (obsolete) — — Splinter Review 
Not rebased, needs lots of work
Attachment #725477 - Attachment is obsolete: true
After talking to Paolo, we decided to split out the thing to query application reputation into its own interface. When that's done (hopefully very soon), depending on how the new JS API for the download manager is progressing, we can either hook into the old download manager or the new one. In either case, the hook should be trivial.
Summary: nsDownloadManager should query a malware reputation service → write an interface to query the application reputation service
Attached patch Query application reputation (obsolete) — — Splinter Review 
Needs tests. I did not stick this in toolkit/components/jsdownloads, because, um, I could not figure out our build system.
Attachment #756708 - Attachment is obsolete: true
Attached patch Query application reputation (obsolete) — — Splinter Review 
Hmm, I am having trouble testing this. I think it might be XPCOM stupidity, because doing a quick and dirty hook into the download manager shows that the querying/parsing code is working, but I am running into 

2101327616[7f037e8156a0]: WARNING: cannot post event if not initialized: file /home/mchew/mozilla-central/netwerk/protocol/http/nsHttpConnectionMgr.cpp, line 173

when I try to mock it in JS.
Attachment #760028 - Attachment is obsolete: true
Comment on attachment 761223 [details] [diff] [review]
Query application reputation

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

::: toolkit/components/downloads/test/unit/test_app_rep.js
@@ +64,5 @@
> +/*
> +  applicationReputation.queryReputation(createURI("http://example.com"), "", null, 0, "", null);
> +*/
> +  applicationReputation.queryReputation(createURI("http://example.com"), "",
> +                                        null, 0, "", listener);

Do you have a mock server set up?  You might need to do that or the XHR might hit a weird state when it can't connect via HTTP.

What is the event being posted in nsHttpConnectionMgr::PostEvent() ?
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnectionMgr.cpp#173

Wild guess... it's closing the connection and can't uploadChannel->ExplicitSetUploadStream() on a closed connection in ApplicationReputation::QueryReputation()
Thanks, Sid. I do have a mock server set up with nsHttpServer on localhost. The XHR was just to see if I could connect to the localhost http server in the test, which I could, the actual query uses nsIChannel.AsyncOpen.

I need to figure out why the test on nsAppRep by itself is closing the connection and nsDownloadManager isn't (not that that hook is completely correct)
Attached patch Query application reputation (obsolete) — — Splinter Review 
Hmm, so if I write a listener in C it works fine. Clearly I don't know how to write javascript.

This patch needs cleanup and commenting.
Attachment #761223 - Attachment is obsolete: true
Attached patch Query application reputation (obsolete) — — Splinter Review 
This patch still needs some work but is ready for the first round of review. The csd.pb.* files were generated with the protocol buffer compiler from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe_browsing/csd.proto&q=clientdownloadrequest&sq=package:chromium 

Things to do:
- Figure out why the native implementation of nsIApplicationReputationListener works but the JS implementation crashes
- Add more tests, there's currently just a malware test
- Use the MIME info to fill in the download type for the request (this is defaulting to WIN_EXECUTABLE now)

https://tbpl.mozilla.org/?tree=Try&rev=a818d4e1cfd7
Attachment #763001 - Attachment is obsolete: true
Attachment #763133 - Flags: review?(paolo.mozmail)
Attached patch hook in to the download manager (obsolete) — — Splinter Review 
This is just to show how the application reputation could be hooked into the existing download manager.
And that build had compile errors on debug, but they are easy to fix. I'll do that but in the meantime the IDLs are ready for review.
Comment on attachment 763133 [details] [diff] [review]
Query application reputation

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

I did a first pass review on the IDL, and I suggest a different approach to the interface design. I didn't look at the code in detail as it still needs work, for example you'll need to use a second-level function with NS_ENSURE_SUCCESS instead of checking individual errors and invoking the callback every time.

It's strange that you weren't able to use a JavaScript callback object, it is a really common case (setAndFetchFaviconForPage and nsIFaviconDataCallback come to my mind, if you want to check them out).

::: toolkit/components/downloads/nsIApplicationReputation.idl
@@ +16,5 @@
> + * @remarks All public methods of the interface may only be called from the main
> + *          thread.
> + */
> +[scriptable, uuid(857da2c0-cfe5-11e2-8b8b-0800200c9a66)]
> +interface nsIApplicationReputation : nsISupports {

Let's call this nsIApplicationReputationQuery (this is creatable and single use, I thought this was a service initially).

@@ +22,5 @@
> +  const unsigned short UNKNOWN_VERDICT = 0;
> +  // The downloaded file is not malware.
> +  const unsigned short OK_VERDICT = 1;
> +  // The downloaded file is malware.
> +  const unsigned short MALWARE_VERDICT = 2;

Since we have basically a yes/no choice (plus unexpected errors), and we won't likely add another verdict type in a way that confuses callers, I think we don't need these constants.

I'd define a shouldBlock property that is populated on completion.

@@ +45,5 @@
> +   * @remarks aListener must not be null. aListener is not managed by
> +   * this interface and must outlive nsIApplicationReputation.
> +   * queryReputationDone is guaranteed to be called on aListener.
> +   */
> +  void queryReputation(in nsIURI aSource,

Since this object is creatable, maybe it's better to use properties rather than function arguments, so that we can easily add more when needed, or ignore some of them.

Let's make this a single-use method "void start(aCallback)".

@@ +46,5 @@
> +   * this interface and must outlive nsIApplicationReputation.
> +   * queryReputationDone is guaranteed to be called on aListener.
> +   */
> +  void queryReputation(in nsIURI aSource,
> +                       in AString aName,

aName is likely irrelevant for the reputation check, and it can be pretty much anything. Since this is prone to misuse (and in fact, the display name of the download isn't necessarily the file name or URL part), I'd simply remove it.

@@ +47,5 @@
> +   * queryReputationDone is guaranteed to be called on aListener.
> +   */
> +  void queryReputation(in nsIURI aSource,
> +                       in AString aName,
> +                       in nsIMIMEInfo aMIMEInfo,

Let's use a contentType string property, instead of nsIMIMEInfo, as there is no other relevant information in there. The new Downloads API will use a content type string internally.

@@ +58,5 @@
> + * Listens for the response for the query.
> + * @remarks All public methods of the interface may only be called from the main
> + *          thread.
> + */
> +[scriptable, uuid(9a228470-cfe5-11e2-8b8b-0800200c9a66)]

You can use the "function" annotation, so that you don't need to define an entire new object in JavaScript:

[scriptable, function, uuid(9a228470-cfe5-11e2-8b8b-0800200c9a66)]

@@ +59,5 @@
> + * @remarks All public methods of the interface may only be called from the main
> + *          thread.
> + */
> +[scriptable, uuid(9a228470-cfe5-11e2-8b8b-0800200c9a66)]
> +interface nsIApplicationReputationListener : nsISupports {

nsIApplicationReputationCallback (it has a single method that will be called exactly once, and is passed to a function as an argument)

@@ +66,5 @@
> +   * @param aVerdict
> +   *        One of OK_VERDICT, MALWARE_VERDICT, or UNKNOWN_VERDICT from
> +   *        nsIAPplicationReputation
> +   */
> +  void queryReputationDone(in unsigned short aVerdict);

To conform to other similar callback objects, please use this signature:

void onComplete(aQuery, aStatus);

For example, you can then do:

applicationReputationQuery.start(function onComplete(aQuery, aStatus) {
  // Note: applicationReputationQuery == aQuery
  if (!Components.isSuccessCode(aStatus)) {
    // Fail download with error...
  } else if (aQuery.shouldBlock) {
    // Block download because of malware...
  } else {
    // Continue...
  }
});
Attachment #763133 - Flags: review?(paolo.mozmail)
Attached patch Query application reputation (obsolete) — — Splinter Review 
Hi Paolo,

Thanks for the comments. Is this closer to what you meant for the IDL?

Re: the suggested file name, this is actually used in the Chrome implementation: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe_browsing/csd.proto&q=clientdownloadrequest&sq=package:chromium&l=203

Re: content type, great! Is that patch already in?

I'm still struggling with the JS version of the callback (it is NPEing mysteriously) and need to add more tests.

Thanks,
Monica
Attachment #763133 - Attachment is obsolete: true
Attachment #765695 - Flags: feedback?(paolo.mozmail)
Attachment #765695 - Attachment is patch: true
Comment on attachment 765695 [details] [diff] [review]
Query application reputation

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

(In reply to Monica Chew [:mmc] (please use ?needinfo) from comment #28)
> Thanks for the comments. Is this closer to what you meant for the IDL?

Yeah, just see a couple of nits below.

> Re: the suggested file name, this is actually used in the Chrome
> implementation:

In the current patch, we behave differently from the code you pointed to,
because we set this value to an arbitrary string that may be totally unrelated
to the chosen target file name.

That said, the target file name (that can be manually changed by the user) is,
and should be, irrelevant to determine whether the downloaded file is safe or
not. If we cannot leave the field empty (for example because the server-side
implementation cannot automatically determine the name from the URI), we
should extract this base name from the provided source URI (thus we have no
need for a separate property for the chosen target file name), or ensure we
use the server-provided "Content-Disposition" attachment file name.

> Re: content type, great! Is that patch already in?

No, we're working on getting the content type in bug 852482.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +41,5 @@
> +      mCallback->OnComplete(this, rv);                                    \
> +      NS_ENSURE_SUCCESS_BODY(res, ret)                                    \
> +      return ret;                                                         \
> +    }                                                                     \
> +  } while(0)

This can be unneeded if you use an approach like ProcessAttention / ProcessStateChange here:

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.cpp#307

This way, you also have code that is simpler to adapt, if you need to change the way the failure is notified (for example, posting an event instead of calling mCallback->OnComplete synchronously).

::: toolkit/components/downloads/ApplicationReputation.h
@@ +55,5 @@
> +  /**
> +   * The callback for the request. We don't own the callback. The callback MUST
> +   * outlive this instance.
> +   */
> +  nsIApplicationReputationCallback* mCallback;

I didn't notice this earlier: you should definitely use an nsCOMPtr here, to keep the reference count of the callback object greater than zero.

You could also use a weak reference if this is what you meant to do here, however I don't think it's useful, as the calling JavaScript code would need, as you noted, to store the callback reference somewhere else to ensure it's not garbage collected.

::: toolkit/components/downloads/nsIApplicationReputation.idl
@@ +24,5 @@
> +
> +  /*
> +   * The nsIURI spec from which the file was downloaded.
> +   */
> +  attribute ACString spec;

attribute nsIURI sourceURI;

@@ +40,5 @@
> +
> +  /*
> +   * The SHA256 hash of the downloaded file in raw bytes.
> +   */
> +  attribute ACString sha256;

attribute ACString sha256Hash; (for consistency)
Attachment #765695 - Flags: feedback?(paolo.mozmail) → feedback+
Thanks Paolo!

(In reply to Paolo Amadini [:paolo] from comment #29)
> That said, the target file name (that can be manually changed by the user)
> is,
> and should be, irrelevant to determine whether the downloaded file is safe or
> not. If we cannot leave the field empty (for example because the server-side
> implementation cannot automatically determine the name from the URI), we
> should extract this base name from the provided source URI (thus we have no
> need for a separate property for the chosen target file name), or ensure we
> use the server-provided "Content-Disposition" attachment file name.

I changed the comments for this to reflect your notes in the IDL. I think the reason they don't extract it from the URI on the server side is exactly as you say, sometimes it comes from the C-D header.
 
> > Re: content type, great! Is that patch already in?
> 
> No, we're working on getting the content type in bug 852482.

I omitted this for now since we can always add it later. For now it is defaulting to WIN_EXECUTABLE.

> ::: toolkit/components/downloads/ApplicationReputation.cpp
> @@ +41,5 @@
> > +      mCallback->OnComplete(this, rv);                                    \
> > +      NS_ENSURE_SUCCESS_BODY(res, ret)                                    \
> > +      return ret;                                                         \
> > +    }                                                                     \
> > +  } while(0)
> 
> This can be unneeded if you use an approach like ProcessAttention /
> ProcessStateChange here:
> 
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/
> BackgroundFileSaver.cpp#307

I wrapped as much of this in a function as I could, but am not sure if that is quite what you meant. Did you mean to add something like mCanceled? I couldn't figure out a way to do that without adding complexity and length. In any case, the references to mCallback are now only in EnsureCallback and at the end of OnStopRequest.

I think I fixed everything else, including the weird JS errors which were in part stemming from requiring aCallback to be non-null (the thing that marshals the scriptable JS function into an nsIApplicationReputationCallback seems to be doing it lazily and it was crashing on debug builds, https://tbpl.mozilla.org/?rev=492d94e27224&tree=Try)

In any case, as you mentioned it is possible for the querier to just emit an event there so I think it is OK for aCallback to be null.

All other comments fixed, and I added a bunch of tests. New try: https://tbpl.mozilla.org/?tree=Try&rev=0fadf956903a
Attachment #763134 - Attachment is obsolete: true
Attachment #765695 - Attachment is obsolete: true
Attachment #766748 - Flags: review?(paolo.mozmail)
qref
Attachment #766748 - Attachment is obsolete: true
Attachment #766748 - Flags: review?(paolo.mozmail)
Attachment #766751 - Flags: review?(paolo.mozmail)
Comment on attachment 766751 [details] [diff] [review]
Write interface to query application reputation

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

::: toolkit/components/downloads/test/unit/test_app_rep.js
@@ +100,5 @@
> +
> +  appRep.start(function onComplete(aQuery, aStatus) {
> +    // We should be getting the garbage response.
> +    do_check_eq(Cr.NS_ERROR_CANNOT_CONVERT_DATA, aStatus);
> +    // We should not block on parse errors.

This could throw NS_ERROR_NOT_AVAILABLE instead, if we want to say that shouldBlock is accessible if and only if onComplete gets NS_OK.
I had a quick look at the patch for now. In the meantime, can you post some
shell recipes, or manual steps, I could execute to generate the .cc files?

Also, I'd be interested in knowing the steps for calculating the lengths used
in the regression tests. This is all required so that we can react to updates
to the server-side interface.

(In reply to Monica Chew [:mmc] (please use ?needinfo) from comment #30)
> I wrapped as much of this in a function as I could, but am not sure if that
> is quite what you meant.

The idea is this pseudo-code:

nsresult MyFunction(ICallback *aCallback)
{
  NS_ENSURE_ARG_POINTER(aCallback);

  mCallback = aCallback;

  nsresult rv = DoMyFunction();
  if (NS_FAILED(rv)) {
    mCallback->Callback(rv);
    mCallback = null;
  }

  return NS_OK;
}

nsresult DoMyFunction(aCallback)
{
  nsresult rv;

  // Do things with the usual pattern.
  rv = DoSomething();
  NS_ENSURE_SUCCESS(rv, rv);

  rv = DoSomethingElse();
  NS_ENSURE_SUCCESS(rv, rv);

  // The last call is the one that, if it succeeds,
  // will cause the callback to be invoked later.
  rv = StartAsyncOperation();
  NS_ENSURE_SUCCESS(rv, rv);
}

> I think I fixed everything else, including the weird JS errors which were in
> part stemming from requiring aCallback to be non-null

I don't really understand how the aCallback parameter could be null if we
specify a callback object in JavaScript. Maybe I could test this directly
on my machine and see what's happening locally.
Thank you very much for the speedy first pass! I still have my fingers crossed that I can get this checked in before leaving for vacation on Wednesday, we'll see :)

(In reply to Paolo Amadini [:paolo] from comment #33)
> I had a quick look at the patch for now. In the meantime, can you post some
> shell recipes, or manual steps, I could execute to generate the .cc files?

I added toolkit/components/download/generate_csd.sh to this patch.

> Also, I'd be interested in knowing the steps for calculating the lengths used
> in the regression tests. This is all required so that we can react to updates
> to the server-side interface.

I added a do_print statement in the test, which I had originally to write the actual do_check tests. As far as changes server-side, the rules for protocol buffers are never to introduce new fields that are required, all new fields must be optional. So, old clients can never break parsing on new optional fields (they won't even know how to query for them), and new clients can never require new fields.

I realize that statement may not be very comforting :) If I had access to the protobuf API from JS I would have constructed new protobufs with SAFE and MALWARE verdicts with some optional fields set, but it was getting kind of long to try to do that by hand. I did manually test with obsolete patch https://bugzilla.mozilla.org/attachment.cgi?id=763134 and we are getting extra fields and parsing correctly.

> The idea is this pseudo-code:

D'oh! That is much cleaner, fixed.

> I don't really understand how the aCallback parameter could be null if we
> specify a callback object in JavaScript. Maybe I could test this directly
> on my machine and see what's happening locally.

philor mentioned my parent was ancient, I hope that was the problem. New try with the NS_ENSURE_ARG(mCallback) checks: https://tbpl.mozilla.org/?tree=Try&rev=87f3f82fc4ad
Attachment #766751 - Attachment is obsolete: true
Attachment #766751 - Flags: review?(paolo.mozmail)
Attachment #766981 - Flags: review?(paolo.mozmail)
Re: backwards compatibility, here's the relevant section from https://developers.google.com/protocol-buffers/docs/overview:

You can add new fields to your message formats without breaking backwards-compatibility; old binaries simply ignore the new field when parsing. So if you have a communications protocol that uses protocol buffers as its data format, you can extend your protocol without having to worry about breaking existing code.
Depends on: 812238
According to my irc logs, the b2g ARM bustage on try is due to http://hg.mozilla.org/integration/mozilla-inbound/rev/971b68a8f9cb.
Comment on attachment 766981 [details] [diff] [review]
Write interface to query application reputation

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

Looks good and clean, there are just a few things to check and maybe two follow-ups to be filed.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +1,2 @@
> +#include "ApplicationReputation.h"
> +#include "csd.pb.h"

License boilerplate and mode line please! (same for the header file)

@@ +24,5 @@
> +#include "nsXPCOMStrings.h"
> +
> +// Preferences that we need to initialize the query.
> +#define PREF_SB_APP_REP_URL "browser.safebrowsing.appRepURL"
> +#define PREF_SB_MALWARE_ENABLED "browser.safebrowsing.malware.enabled"

So, if I understand correctly, this preference maps to the "Block reported attack sites" checkbox. Whether the application reputation functionality can be bound to this preference should be discussed. While I think using this preference for this initial check-in is fine, please file a separate bug blocking 662819 to get a UX review and figure out if this is what we want (or if we need a separate preference or we should rename the label).

@@ +103,5 @@
> +ApplicationReputationQuery::Start(nsIApplicationReputationCallback* aCallback) {
> +  NS_ENSURE_ARG_POINTER(aCallback);
> +  mCallback = aCallback;
> +
> +  MOZ_ASSERT(!mCalledOnce, "Can't call Start() more than once");

This should be NS_ENSURE_STATE, as we are on an interface boundary and callers like add-ons may actually call this more than once.

Please move this after NS_ENSURE_ARG_POINTER and before "mCallback = aCallback;".

@@ +111,5 @@
> +  if (NS_FAILED(rv)) {
> +    mCallback->OnComplete(this, rv);
> +    mCallback = nullptr;
> +  }
> +  return rv;

You should return NS_OK here (and update the tests consequently). Callers are already notified of the failure by the callback.

@@ +125,5 @@
> +  bool malwareCheckEnabled = false;
> +  prefs->GetBoolPref(PREF_SB_MALWARE_ENABLED, &malwareCheckEnabled);
> +  if (!malwareCheckEnabled) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }

if (!Preferences::GetBool(...)) {
  return NS_ERROR_NOT_AVAILABLE;
}

You can use the same static helper functions below.

@@ +138,5 @@
> +  safe_browsing::ClientDownloadRequest req;
> +
> +  nsAutoCString spec;
> +  rv = mURI->GetSpec(spec);
> +  NS_ENSURE_SUCCESS(rv, rv);

I guess we should null-check mURI with NS_ENSURE_STATE? (and add a regression test for that)

@@ +149,5 @@
> +  nsCString locale;
> +  prefs->GetCharPref(PREF_GENERAL_LOCALE, getter_Copies(locale));
> +  req.set_locale(locale.get());
> +  req.mutable_digests()->set_sha256(mSha256Hash.Data());
> +  req.set_file_basename(NS_ConvertUTF16toUTF8(mName).get());

Both the hash and the name my not be set by the caller, is it correct to call the set_sha256 and set_file_basename methods with empty data in that case? (if so, indicate that in a comment)

@@ +154,5 @@
> +
> +  // Serialize the protocol buffer to a string
> +  std::string serialized;
> +  if (!req.SerializeToString(&serialized)) {
> +    return NS_ERROR_CANNOT_CONVERT_DATA;

May this occur in case we have invalid member variables? (this should just be NS_ERROR_UNEXPECTED in this case)

Or should this actually map to NS_ERROR_OUT_OF_MEMORY?

@@ +179,5 @@
> +
> +  rv = httpChannel->SetRequestHeader(
> +    NS_LITERAL_CSTRING("User-Agent"),
> +    NS_LITERAL_CSTRING("CsdTesting/Mozilla"), false);
> +  NS_ENSURE_SUCCESS(rv, rv);

Will this User-Agent string remain the same? If it needs to be changed later, a bug should be filed and the source code here annotated. We may make it a constant at the top of the file, or a preference.

@@ +232,5 @@
> +NS_IMETHODIMP
> +ApplicationReputationQuery::OnStopRequest(nsIRequest *aRequest,
> +                                          nsISupports *aContext,
> +                                          nsresult aResult) {
> +  NS_ENSURE_ARG_POINTER(mCallback);

nit: NS_ENSURE_STATE

@@ +250,5 @@
> +  uint32_t status = 0;
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = channel->GetResponseStatus(&status);
> +  NS_ENSURE_SUCCESS(rv, rv);

nit: "uint32_t status = 0;" just above "rv = channel->..."

@@ +253,5 @@
> +  rv = channel->GetResponseStatus(&status);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (status != 200) {
> +    return NS_ERROR_NOT_AVAILABLE;

We may document in the IDL that this is the error code we get when the network request was unsuccessful, or the service has been disabled.

@@ +259,5 @@
> +
> +  std::string buf(mResponse.Data(), mResponse.Length());
> +  safe_browsing::ClientDownloadResponse response;
> +  if (!response.ParseFromString(buf)) {
> +    return NS_ERROR_CANNOT_CONVERT_DATA;

nit: this looks like a good error name, but it's documented as "used by nsIVariant" only, so I'd just use NS_ERROR_FAILURE here (we may additionally issue an NS_WARNING with an error description).

::: toolkit/components/downloads/ApplicationReputation.h
@@ +44,5 @@
> +
> +  /**
> +   * True if Start() has already been called.
> +   */
> +  bool mCalledOnce;

nit: mRequestStarted?

@@ +49,5 @@
> +
> +  /**
> +   * True if reading mShouldBlock is valid (OnStopRequest has been called).
> +   */
> +  bool mShouldBlockIsValid;

nit: mRequestSucceeded?

@@ +62,5 @@
> +   * The response from the application reputation query. This is read in chunks
> +   * as part of our nsIStreamListener implementation and may contain embedded
> +   * NULLs.
> +   */
> +  nsAutoCString mResponse;

nit: It looks like the string guide suggests nsCString for member variables, not a big deal though.

::: toolkit/components/downloads/nsIApplicationReputation.idl
@@ +13,5 @@
> + * Asynchronously queries an application reputation service based on metadata
> + * of the downloaded file.
> + * @remarks This is a single-use interface. Start() may only be called once,
> + *          and shouldBlock can only be accessed after
> + *          nsIApplicationReputationCallback.onComplete has been called.

mini-nit: blank line before @remarks.

@@ +33,5 @@
> +  /*
> +   * The target filename for the downloaded file, as inferred from the source
> +   * URI or provided by the Content-Disposition attachment file name.
> +   */
> +  attribute AString name;

suggestedFileName?

@@ +50,5 @@
> +   * Start querying the application reputation service.
> +   *
> +   * @remarks aCallback may not be null. aCallback must outlive
> +   * nsIApplicationReputation.  onComplete is guaranteed to be called on
> +   * aCallback. This function may not be called more than once.

Since we have a strong reference, the "aCallback must outlive nsIApplicationReputation" part can be removed (also in the .h file).

nit: Indent @remarks like you did above.

::: toolkit/components/downloads/test/unit/test_app_rep.js
@@ +6,5 @@
> +
> +Cu.import('resource://gre/modules/NetUtil.jsm');
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const ApplicationReputation = Components.Constructor(

nit: const ApplicationReputationQuery = ...

@@ +34,5 @@
> +    return blob;
> +  }
> +
> +  gHttpServ.registerPathHandler("/download", function(request, response) {
> +    response.setStatusLine(request.httpVersion, 200, "OK");

nit: I don't think setStatusLine is required here.

@@ +54,5 @@
> +  });
> +
> +  gHttpServ.start(4444);
> +  do_register_cleanup(function teardown() {
> +    gHttpServ.stop(do_test_finished);

I don't think do_test_finished can be called in cleanup functions, that are expected to finish synchronously. In the jsdownloads tests, I had to use a tail file for shutting down the HTTP server.

@@ +69,5 @@
> +  } catch (ex if ex.result == Cr.NS_ERROR_NOT_AVAILABLE) { }
> +  run_next_test();
> +});
> +
> +add_test(function test_dirty() {

nit: test_shouldBlock_true

@@ +81,5 @@
> +    run_next_test();
> +  });
> +});
> +
> +add_test(function test_clean() {

nit: test_shouldBlock_false
Attachment #766981 - Flags: review?(paolo.mozmail) → feedback+
Paolo, thanks a million for the speedy review! It really makes a difference in saving time context switching, mentally and otherwise.

I was really hoping to get another iteration out before I left, but unfortunately I pulled from central while it was broken and my client is not in a good state, so this patch is totally untested and not ready for re-review.

This will be my highest priority when I get back from PTO on July 8.
Attachment #766981 - Attachment is obsolete: true
Re: followup bugs, I filed bug 887041 and bug 887044 for resolving the user agent and UX questions.
(In reply to Monica Chew [:mmc] (gone until July 8) from comment #40)
> Re: followup bugs, I filed bug 887041 and bug 887044 for resolving the user
> agent and UX questions.

Thanks!

(In reply to Monica Chew [:mmc] (gone until July 8) from comment #39)
> This will be my highest priority when I get back from PTO on July 8.

Enjoy your vacation :-)
Hi Paolo,

I think I addressed everything you mentioned.

(In reply to Paolo Amadini [:paolo] from comment #38)
> @@ +154,5 @@
> > +
> > +  // Serialize the protocol buffer to a string
> > +  std::string serialized;
> > +  if (!req.SerializeToString(&serialized)) {
> > +    return NS_ERROR_CANNOT_CONVERT_DATA;
> 
> May this occur in case we have invalid member variables? (this should just
> be NS_ERROR_UNEXPECTED in this case)

This can only happen if the protocol buffer is not initialized properly and some required fields are not set. Since we are setting all of the required fields in the code above this should actually never happen. I changed it to NS_ERROR_UNEXPECTED.

> @@ +62,5 @@
> > +   * The response from the application reputation query. This is read in chunks
> > +   * as part of our nsIStreamListener implementation and may contain embedded
> > +   * NULLs.
> > +   */
> > +  nsAutoCString mResponse;
> 
> nit: It looks like the string guide suggests nsCString for member variables,
> not a big deal though.

I don't think I can use this, nsCString breaks on embedded nulls.

I'm still waiting on try to be back up, but everything passes locally.
Attachment #767546 - Attachment is obsolete: true
Attachment #772373 - Flags: review?(paolo.mozmail)
Comment on attachment 772373 [details] [diff] [review]
Write interface to query application reputation

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

::: browser/app/profile/firefox.js
@@ +760,5 @@
>  
>  pref("browser.safebrowsing.warning.infoURL", "https://www.mozilla.org/%LOCALE%/firefox/phishing-protection/");
>  pref("browser.safebrowsing.malware.reportURL", "http://safebrowsing.clients.google.com/safebrowsing/diagnostic?client=%NAME%&hl=%LOCALE%&site=");
> +// TODO(mmc): Switch to blank before checkin. We don't want to launch without
> +// telemetry and whitelisting.

TODO: remove todo before checking in, this is not hooked in anywhere.

::: toolkit/components/downloads/ApplicationReputation.h
@@ +59,5 @@
> +  bool mRequestSucceeded;
> +
> +  /**
> +   * The callback for the request. We don't own the callback. The callback MUST
> +   * outlive this instance.

Will remove "MUST outlive" comment
Comment on attachment 772373 [details] [diff] [review]
Write interface to query application reputation

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

::: browser/app/profile/firefox.js
@@ +763,5 @@
> +// TODO(mmc): Switch to blank before checkin. We don't want to launch without
> +// telemetry and whitelisting.
> +#ifndef MOZILLA_OFFICIAL
> +pref("browser.safebrowsing.appRepURL", "https://sb-ssl.google.com/safebrowsing/clientreport/download");
> +#endif

Since you're not blanking this preference, can you clarify the "#ifndef"? It doesn't seem to be required, as other safebrowsing URLs are not under a similar condition.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +118,5 @@
> +}
> +
> +NS_IMETHODIMP
> +ApplicationReputationQuery::Start(nsIApplicationReputationCallback* aCallback) {
> +  NS_ENSURE_STATE(aCallback);

NS_ENSURE_ARG_POINTER(aCallback);

(I think we have too many different macros for checking null, but still...)

@@ +153,5 @@
> +
> +  safe_browsing::ClientDownloadRequest req;
> +
> +  nsAutoCString spec;
> +  rv = mURI->GetSpec(spec);

I'd move the NS_ENSURE_STATE(mURI) check here (it's easier to verify correctness at first glance, and it's robust in case the code is moved around or made asynchronous in the future).

@@ +166,5 @@
> +  NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_GENERAL_LOCALE, &locale),
> +                    NS_ERROR_NOT_AVAILABLE);
> +  req.set_locale(locale.get());
> +  req.mutable_digests()->set_sha256(mSha256Hash.Data());
> +  req.set_file_basename(NS_ConvertUTF16toUTF8(mSuggestedFileName).get());

Both the hash and the name my not be set by the caller, is it correct to call the set_sha256 and set_file_basename methods with empty data in that case? (if so, indicate that in a comment)

@@ +280,5 @@
> +  std::string buf(mResponse.Data(), mResponse.Length());
> +  safe_browsing::ClientDownloadResponse response;
> +  if (!response.ParseFromString(buf)) {
> +    return NS_ERROR_CANNOT_CONVERT_DATA;
> +  }

NS_ERROR_FAILURE + NS_WARNING

::: toolkit/components/downloads/ApplicationReputation.h
@@ +44,5 @@
> +  bool mShouldBlock;
> +  nsAutoString mSuggestedFileName;
> +  nsCOMPtr<nsIURI> mURI;
> +  uint32_t mFileSize;
> +  nsAutoCString mSha256Hash;

(In reply to Monica Chew [:mmc] from comment #42)
> I don't think I can use this, nsCString breaks on embedded nulls.

https://developer.mozilla.org/Mozilla_internal_string_guide#Introduction

At least on paper, nsString/nsCString preserve embedded nulls, and additionally guarantee the presence of a null terminator in the underlying buffer. They inherit from nsSubstring/nsCSubstring that also preserve embedded nulls but don't provide the terminator. Have you experienced a different behavior?

nsAutoString/nsAutoCString inherit from nsString/nsCString and additionally contain a 64 character buffer regardless of the size of the string. This is why its use in member variables is discouraged (in our case, we add 256 bytes of unneeded buffers, and we might even not be using them since we're referencing existing strings):

https://developer.mozilla.org/Mozilla_internal_string_guide#Member_variables

In particular mSha256Hash and mSuggestedFileName should definitely not be Auto strings. Having mResponse as an nsCString is also a good way to ensure that, if someone looks at the code and do a cut-and-paste, they don't end up using the wrong string type. If there is a good reason for using nsAutoCString instead, a comment should be added to explain the exception.
Attachment #772373 - Flags: review?(paolo.mozmail)
> Since you're not blanking this preference, can you clarify the "#ifndef"? It
> doesn't seem to be required, as other safebrowsing URLs are not under a
> similar condition.

I just wanted to be extra cautious. I changed the comment to reference the bug I thought should be required before turning it on.

> > +ApplicationReputationQuery::Start(nsIApplicationReputationCallback* aCallback) {
> > +  NS_ENSURE_STATE(aCallback);
> 
> NS_ENSURE_ARG_POINTER(aCallback);
> 
> (I think we have too many different macros for checking null, but still...)

Oops, I think this was flip flopped from comment 38. I have made all of the pointer checks NS_ENSURE_ARG_POINTER and the bool state check NS_ENSURE_STATE.
 
> @@ +153,5 @@
> > +
> > +  safe_browsing::ClientDownloadRequest req;
> > +
> > +  nsAutoCString spec;
> > +  rv = mURI->GetSpec(spec);
> 
> I'd move the NS_ENSURE_STATE(mURI) check here (it's easier to verify
> correctness at first glance, and it's robust in case the code is moved
> around or made asynchronous in the future).

I moved this and then realized that the test behavior is now different (Start returns NS_OK instead of NS_ERROR and calls OnComplete with NS_ERROR). I thought it was inconsistent to return NS_OK for null source URI but NS_ERROR for null callback so I reverted it.

> @@ +166,5 @@
> > +  NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_GENERAL_LOCALE, &locale),
> > +                    NS_ERROR_NOT_AVAILABLE);
> > +  req.set_locale(locale.get());
> > +  req.mutable_digests()->set_sha256(mSha256Hash.Data());
> > +  req.set_file_basename(NS_ConvertUTF16toUTF8(mSuggestedFileName).get());
> 
> Both the hash and the name my not be set by the caller, is it correct to
> call the set_sha256 and set_file_basename methods with empty data in that
> case? (if so, indicate that in a comment)

I changed the comment in the idl for these attributes to say that if the caller doesn't set the attributes, the query will succeed but not necessarily contain useful data. I didn't want to be too restrictive about which fields were required because the API still hasn't been officially released by Google.

> @@ +280,5 @@
> > +  std::string buf(mResponse.Data(), mResponse.Length());
> > +  safe_browsing::ClientDownloadResponse response;
> > +  if (!response.ParseFromString(buf)) {
> > +    return NS_ERROR_CANNOT_CONVERT_DATA;
> > +  }
> 
> NS_ERROR_FAILURE + NS_WARNING

Fixed.

> ::: toolkit/components/downloads/ApplicationReputation.h
> @@ +44,5 @@
> > +  bool mShouldBlock;
> > +  nsAutoString mSuggestedFileName;
> > +  nsCOMPtr<nsIURI> mURI;
> > +  uint32_t mFileSize;
> > +  nsAutoCString mSha256Hash;
> 
> (In reply to Monica Chew [:mmc] from comment #42)
> > I don't think I can use this, nsCString breaks on embedded nulls.
> 
> https://developer.mozilla.org/Mozilla_internal_string_guide#Introduction

I must have read this a million times -- I changed it to nsCString, sorry for the brain fart.
Attachment #772373 - Attachment is obsolete: true
Attachment #772718 - Flags: review?(paolo.mozmail)
Comment on attachment 772718 [details] [diff] [review]
Write interface to query application reputation

(In reply to Monica Chew [:mmc] from comment #45)
> Oops, I think this was flip flopped from comment 38. I have made all of the
> pointer checks NS_ENSURE_ARG_POINTER and the bool state check
> NS_ENSURE_STATE.

Uh, I think this isn't documented anywhere... I should have mentioned that
NS_ENSURE_ARG and NS_ENSURE_ARG_POINTER are used for the arguments of the
function (they return NS_ERROR_INVALID_ARG), while NS_ENSURE_STATE is used
for member variables (regardless of their type) or other pre-existing object
state (and returns NS_ERROR_UNEXPECTED).

> I moved this and then realized that the test behavior is now different
> (Start returns NS_OK instead of NS_ERROR and calls OnComplete with
> NS_ERROR). I thought it was inconsistent to return NS_OK for null source URI
> but NS_ERROR for null callback so I reverted it.

Actually, I think a null aCallback or an attempt to set a different callback
are the only cases where we're justified in failing early (we couldn't
reasonably fail later). All the other cases should be handled by invoking the
callback, so that we're more consistent in how we fail. This should be a
simple change even if it involves changing a test.

> I changed the comment in the idl

I see, this looks good. Didn't notice that as Bugzilla interdiff didn't work.

r+ with these changes, and now... you need super-review on the IDL. Should
hopefully be a quick one now that we've a clearly defined and thoroughly
documented interface.
Attachment #772718 - Flags: review?(paolo.mozmail) → review+
Thanks for explaining the meaning of NS_ENSURE_STATE for member variables, I totally didn't get that.

I have fixed these, plus the test for only throwing invalid on Start() if aCallback is null.

Gavin, Dave, do you have time for superreview of the idl? Paolo is the owner of downloads submodule and has reviewed already.

Dave, I found this thread from you about superreview from last December (https://groups.google.com/forum/#!searchin/mozilla.dev.planning/superreview/mozilla.dev.planning/fZV-DYnqQEc/KWJIDWJZ4ZMJ) but couldn't tell if it went anywhere.
Attachment #772718 - Attachment is obsolete: true
Attachment #772797 - Flags: superreview?(gavin.sharp)
Attachment #772797 - Flags: review?(dtownsend+bugmail)
Comment on attachment 772797 [details] [diff] [review]
Write interface to query application reputation

>diff --git a/toolkit/components/downloads/nsIApplicationReputation.idl b/toolkit/components/downloads/nsIApplicationReputation.idl

>+interface nsIApplicationReputationQuery : nsISupports {
>+  /**
>+   * True if the downloaded file should be blocked because it's malware. This
>+   * can only be accessed after nsIApplicationReputationCallback.onComplete has
>+   * been called.
>+   */
>+  readonly attribute bool shouldBlock;

This seems like a strange API. Why not pass the value to onComplete instead of requiring that it be retrieved from the object?

>+  attribute nsIURI sourceURI;
>+  attribute AString suggestedFileName;
>+  attribute unsigned long fileSize;
>+  attribute ACString sha256Hash;

Along those lines, why aren't these just arguments to start()? I guess this comes from Paolo's proposal in comment 27.

It's not clear to me why it's useful to have a publically exposed object that represents the request itself, but maybe I'm misunderstanding the intended use of this API. I would have expected to define a service with a method (accepting metadata) that kicks of a request and asynchronously returns a result (passing in the resulting data to a callback). Internally it would keep track of objects representing the requests, perhaps, but that's an implementation detail that I don't yet see a reason to expose. Am I misunderstanding?
Separate concern: I'm sure this is probably fine, but the code imported from Chromium should probably get explicit sign-off from someone on the licensing team (Gerv), to make sure we're complying with all license requirements.

(I'm also a bit confused about the presence of just toolkit/components/protobuf/Makefile.in in the patch, presumably there are other files there that weren't included in the patch?)
Hey Gavin,

Thanks for the comments. The license for the protocol buffer library was in bug 810404. The initial import was bug 812238 and I missed the Makefile change that would make it compile with m-c (because nothing was using it in the initial import).

> >+  /**
> >+   * True if the downloaded file should be blocked because it's malware. This
> >+   * can only be accessed after nsIApplicationReputationCallback.onComplete has
> >+   * been called.
> >+   */
> >+  readonly attribute bool shouldBlock;
> 
> This seems like a strange API. Why not pass the value to onComplete instead
> of requiring that it be retrieved from the object?

I guess one reason to support retrieving the verdict from the object is that the Google API actually supports multiple verdicts (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe_browsing/csd.proto&q=clientdownloadrequest&sq=package:chromium&type=cs&l=223) but we only care about one at the moment, so passing it back through the object possibly lets callers get away with fewer API changes.

I am happy to write it either way -- here are 2 existing APIs in toolkit that use the same idiom:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/mozIColorAnalyzer.idl#25
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsIFaviconService.idl#123
 
> >+  attribute nsIURI sourceURI;
> >+  attribute AString suggestedFileName;
> >+  attribute unsigned long fileSize;
> >+  attribute ACString sha256Hash;
> 
> Along those lines, why aren't these just arguments to start()? I guess this
> comes from Paolo's proposal in comment 27.

Yes. I guess the argument for keeping them separate is that it makes it easier to add or deprecate attributes later and keeps the interface cleaner, especially since the Google API has not been publically released yet.

Thinking about changing this to a service, it seems that would require extra bookkeeping for this interface to keep track of and throttle outstanding requests, when the nsIChannel->AsyncOpen is already nicely handling callbacks.

Do you think that writing this as a service is worth the extra overhead?

Thanks,
Monica
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #48)
> It's not clear to me why it's useful to have a publically exposed object
> that represents the request itself, but maybe I'm misunderstanding the
> intended use of this API. I would have expected to define a service with a
> method (accepting metadata) that kicks of a request and asynchronously
> returns a result (passing in the resulting data to a callback). Internally
> it would keep track of objects representing the requests, perhaps, but
> that's an implementation detail that I don't yet see a reason to expose. Am
> I misunderstanding?

I agree with gavin here, I'd expect this to be more along the lines of:

nsIApplicationReputationService
  queryReputation(sourceURI, filesize, hash, filename, callback)

If we're concerned about the set of parameters changing over time then declaring those parameters as an object might make sense.

Presumably the request can fail, is there any use in exposing that or do we just assume that means we shouldn't block?

I'd rather that Paolo or Marco did the code review here if they're ok with that.
Comment on attachment 772797 [details] [diff] [review]
Write interface to query application reputation

Probably best to get the API agreed upon here before proceeding with full review
Attachment #772797 - Flags: review?(dtownsend+bugmail)
(In reply to Dave Townsend (:Mossop) from comment #51)
> If we're concerned about the set of parameters changing over time then
> declaring those parameters as an object might make sense.

I'd like to take this as consensus to leave the ApplicationReputationQuery object as an object, since both Paolo and I agreed that it should be one.
 
> Presumably the request can fail, is there any use in exposing that or do we
> just assume that means we shouldn't block?

It means we shouldn't block. I think that exposing network failures to the caller is a layering violation, and the purpose of this class should be returning a yes/no decision even in the face of transient failures.

> I'd rather that Paolo or Marco did the code review here if they're ok with
> that.

Paolo has done the review and given an r+ in comment 46.

Given that Dave has said that it might make sense to keep the parameters as an object, I think the only remaining question in this review is whether or not Gavin is OK with keeping the idl as is instead of a service, which will incur additional overhead in keeping track of outstanding queries.

Gavin, what do you think?

Thanks,
Monica
(In reply to Monica Chew [:mmc] from comment #53)
> (In reply to Dave Townsend (:Mossop) from comment #51)
> Given that Dave has said that it might make sense to keep the parameters as
> an object, I think the only remaining question in this review is whether or
> not Gavin is OK with keeping the idl as is instead of a service, which will
> incur additional overhead in keeping track of outstanding queries.


What overhead are you concerned about here? Your patch already creates a separate query object for each request. That can still happen with a service regardless of whether they are exposed to callers.
(In reply to Dave Townsend (:Mossop) from comment #54)
> (In reply to Monica Chew [:mmc] from comment #53)
> > (In reply to Dave Townsend (:Mossop) from comment #51)
> > Given that Dave has said that it might make sense to keep the parameters as
> > an object, I think the only remaining question in this review is whether or
> > not Gavin is OK with keeping the idl as is instead of a service, which will
> > incur additional overhead in keeping track of outstanding queries.
> 
> 
> What overhead are you concerned about here? Your patch already creates a
> separate query object for each request. That can still happen with a service
> regardless of whether they are exposed to callers.

That is true, a separate query object is created for each request. In the current implementation no single class if responsible for keeping track of all outstanding requests, which I think would happen if we switched to a service, unless I misunderstand Gavin's comment 48. This is a bookkeeping overhead, not an additional memory overhead.
(In reply to Monica Chew [:mmc] from comment #55)
> (In reply to Dave Townsend (:Mossop) from comment #54)
> > (In reply to Monica Chew [:mmc] from comment #53)
> > > (In reply to Dave Townsend (:Mossop) from comment #51)
> > > Given that Dave has said that it might make sense to keep the parameters as
> > > an object, I think the only remaining question in this review is whether or
> > > not Gavin is OK with keeping the idl as is instead of a service, which will
> > > incur additional overhead in keeping track of outstanding queries.
> > 
> > 
> > What overhead are you concerned about here? Your patch already creates a
> > separate query object for each request. That can still happen with a service
> > regardless of whether they are exposed to callers.
> 
> That is true, a separate query object is created for each request. In the
> current implementation no single class if responsible for keeping track of
> all outstanding requests, which I think would happen if we switched to a
> service, unless I misunderstand Gavin's comment 48. This is a bookkeeping
> overhead, not an additional memory overhead.

He mentions it as a potential implementation detail. Whether it does or not is up to you. I don't think the service would, for example, allow callers to enumerate all the current queries, it would just be there as a means to kick off a new query.
OK. I think that we either have to choose between having the query object exposed through the IDL or running it as a service, otherwise we get an interface like

nsIApplicationReputationService::start(nsIApplicationReputationQuery, nsIApplicationReputationCallback) as the only method in the service interface

nsIApplicationReputationQuery with only attributes

which I think is strictly worse than the current implementation, since it requires callers to both instantiate the query and pass it to the service.

I still think that keeping the query object accessible through the idl is the right thing to do, given that the API is not officially released.

Is there any advantage to implementing this as a service, if the query itself is also accessible through the interface? Other than some constants for the URL to send the query to, there is no shared state between queries -- a new channel must be opened for each one.

Thanks,
Monica
Would it help to rename start to send, like XHR? nsIApplicationReputationQuery is much closer in spirit to XHR than any service, it is basically a simple wrapper around a single HTTP request.
My apologies for having requested super-review late in the process, we should
definitely have discussed this upfront. I'll do better next time.

(In reply to Dave Townsend (:Mossop) from comment #51)
> I agree with gavin here, I'd expect this to be more along the lines of:
> 
> nsIApplicationReputationService
>   queryReputation(sourceURI, filesize, hash, filename, callback)

The issue with this type of signatures is that they tend to want the callback
as the last argument, and if a new argument is needed, they tend to change the
position of the callback argument instead of just appending the new argument,
and this makes it needed for add-ons to version-check Gecko before calling the
method, instead of relying on feature detection. That said, I think very few
add-ons, if any, will need to execute the query reputation call themselves.

> If we're concerned about the set of parameters changing over time then
> declaring those parameters as an object might make sense.

So, I suggested something along the lines of nsIRequest/nsIChannel, where you
set members variables and then call a method (like asyncOpen) accepting a
callback as its argument. When the callback informs you, you can check other
properties of the same object for a response state. (We may also call this a
"listener".)

By having a request object with methods and not only attributes, in the future
we might, for example, easily add a cancel() method on the request, or progress
listeners if we need them. This wouldn't be possible with the other proposed
signatures.

The difference is that nsIChannel is created through a factory service, because
it may have multiple implementations, instead of direct instantiation. We don't
have multiple implementations here, but we may consider using a factory service
to instantiate the query object. (Whether or not we keep track of all the
created requests is an implementation detail that is independent of the
instantiation method.)
Comment on attachment 772797 [details] [diff] [review]
Write interface to query application reputation

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

I noticed that two test implementation fixes are needed in the latest patch.

::: toolkit/components/downloads/test/unit/test_app_rep.js
@@ +112,5 @@
> +  // No source URI
> +  appRep.start(function onComplete(aQuery, aStatus) {
> +    do_check_eq(Cr.NS_ERROR_UNEXPECTED, aStatus);
> +  });
> +  run_next_test();

run_next_test should be called inside the callback.

@@ +138,5 @@
> +      let shouldBlock = aQuery.shouldBlock;
> +      do_throw("Shouldn't be able to get shouldBlocked if not successful");
> +    } catch (ex if ex.result == Cr.NS_ERROR_NOT_AVAILABLE) { }
> +  });
> +  run_next_test();

Also here.
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #57)
> OK. I think that we either have to choose between having the query object
> exposed through the IDL or running it as a service, otherwise we get an
> interface like
> 
> nsIApplicationReputationService::start(nsIApplicationReputationQuery,
> nsIApplicationReputationCallback) as the only method in the service interface
> 
> nsIApplicationReputationQuery with only attributes
> 
> which I think is strictly worse than the current implementation, since it
> requires callers to both instantiate the query and pass it to the service.

Not when calling from JS, you can just pass a plain JS object from there.

> I still think that keeping the query object accessible through the idl is
> the right thing to do, given that the API is not officially released.

I don't really understand what you're saying here, can you explain?

> Is there any advantage to implementing this as a service, if the query
> itself is also accessible through the interface? Other than some constants
> for the URL to send the query to, there is no shared state between queries
> -- a new channel must be opened for each one.

The advantage to using a service over the API in the current patch is that it provides a clean separation between the data that is used to create the query (passed to the service's query method) and the information returned by the query (passed to the callback).

There's no confusion about when you can access what properties, because those available after the query completes simply aren't there to be accessed.

it also gives you more flexibility in how you implement the API. You can create one object per query as the current API forces, or you can just have the one service that handles everything internally which could be more memory efficient.
> > I still think that keeping the query object accessible through the idl is
> > the right thing to do, given that the API is not officially released.
> 
> I don't really understand what you're saying here, can you explain?

I just mean that if Google modifies the parameters of the request, it will be easier for callers to add attributes to the request object than trying to modify the parameters for start. And it's easier for Google to modify the API when the API hasn't been officially released.

Changes coming to make this a service.

Thanks,
Monica
Attached patch idl changes only (obsolete) — — Splinter Review 
Please have another look.
Attachment #772797 - Attachment is obsolete: true
Attachment #772797 - Flags: superreview?(gavin.sharp)
Attachment #774794 - Flags: superreview?(dtownsend+bugmail)
Attachment #774794 - Flags: review?(paolo.mozmail)
Comment on attachment 774794 [details] [diff] [review]
idl changes only

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

::: toolkit/components/downloads/nsIApplicationReputation.idl
@@ +26,5 @@
> +   * @param aCallback
> +   *        The callback for receiving the results of the query.
> +   *
> +   * @remarks aCallback may not be null.  onComplete is guaranteed to be called
> +   *          on aCallback. This function may not be called more than once. If

"more than once with the same query object"
Comment on attachment 774794 [details] [diff] [review]
idl changes only

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

Looks OK for a service-based interface. I'm indicating a couple of suggestions that came to my mind, though I'd wait for Dave's super-review to see if these are really necessary.

I wonder if we should allow reusing nsIApplicationReputationQuery objects?

If we'll want to add a progress listener or request control object for cancellation in the future, we'll need to add a third parameter or a return value to start().

::: toolkit/components/downloads/nsIApplicationReputation.idl
@@ +27,5 @@
> +   *        The callback for receiving the results of the query.
> +   *
> +   * @remarks aCallback may not be null.  onComplete is guaranteed to be called
> +   *          on aCallback. This function may not be called more than once. If
> +   *          any of the above attributes have not been set or have been set

nit: above attributes = attributes of aQuery?

@@ +33,5 @@
> +   *          request can still be constructed and will solicit a valid
> +   *          response, but won't produce any useful information.
> +   */
> +  void start(in nsIApplicationReputationQuery aQuery,
> +             in nsIApplicationReputationCallback aCallback);

Maybe we should call this queryReputation?
Attachment #774794 - Flags: review?(paolo.mozmail)
> I wonder if we should allow reusing nsIApplicationReputationQuery objects?
> 
> If we'll want to add a progress listener or request control object for
> cancellation in the future, we'll need to add a third parameter or a return
> value to start().

My vote would be against on both, on the principle of YAGNI (http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it)

I'm happy to add this functionality later should the onComplete callback be insufficient for some reason.

> > +   *
> > +   * @remarks aCallback may not be null.  onComplete is guaranteed to be called
> > +   *          on aCallback. This function may not be called more than once. If
> > +   *          any of the above attributes have not been set or have been set
> 
> nit: above attributes = attributes of aQuery?

Will fix, thanks for catching.

> @@ +33,5 @@
> > +   *          request can still be constructed and will solicit a valid
> > +   *          response, but won't produce any useful information.
> > +   */
> > +  void start(in nsIApplicationReputationQuery aQuery,
> > +             in nsIApplicationReputationCallback aCallback);
> 
> Maybe we should call this queryReputation?

Either way is fine with me.

Thanks,
Monica
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #66)
> > I wonder if we should allow reusing nsIApplicationReputationQuery objects?
> > 
> > If we'll want to add a progress listener or request control object for
> > cancellation in the future, we'll need to add a third parameter or a return
> > value to start().
> 
> My vote would be against on both, on the principle of YAGNI
> (http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it)
> 
> I'm happy to add this functionality later should the onComplete callback be
> insufficient for some reason.

Yeah, that's exactly what I was saying.
Comment on attachment 774794 [details] [diff] [review]
idl changes only

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

::: toolkit/components/downloads/nsIApplicationReputation.idl
@@ +26,5 @@
> +   * @param aCallback
> +   *        The callback for receiving the results of the query.
> +   *
> +   * @remarks aCallback may not be null.  onComplete is guaranteed to be called
> +   *          on aCallback. This function may not be called more than once. If

"more than once for the same query object" since we aren't passing it to the callback.

@@ +33,5 @@
> +   *          request can still be constructed and will solicit a valid
> +   *          response, but won't produce any useful information.
> +   */
> +  void start(in nsIApplicationReputationQuery aQuery,
> +             in nsIApplicationReputationCallback aCallback);

query or queryReputation sounds good to me

@@ +84,5 @@
> +   * @param aShouldBlock
> +   *        Whether or not the download should be blocked.
> +   */
> +  void onComplete(in nsresult aStatus,
> +                  in bool aShouldBlock);

It looks like most callers will only care about aShouldBlock. In which case it might be better to put that first then I can just declare function callback(shouldBlock) and ignore the second argument that I don't care about.
Attachment #774794 - Flags: superreview?(dtownsend+bugmail) → superreview+
sr=dtownsend in comment 68, r=paolo in comment 46, addressed changes in comment 60.

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

Added nsIApplicationReputationService. I did leave the callback attached to the query so the idl for nsIApplicationReputationQuery has one extra field.

Paolo, do you want to have one last look before checking in? If not, I'll push when try is green.

Thanks,
Monica
Attachment #774794 - Attachment is obsolete: true
Flags: needinfo?(paolo.mozmail)
Comment on attachment 780642 [details] [diff] [review]
Write interface to query application reputation

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

Looks good to me!

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +170,5 @@
> +    NS_LITERAL_CSTRING("application/octet-stream"), serialized.size(),
> +    NS_LITERAL_CSTRING("POST"), false);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIStreamListener> aListener = do_QueryInterface(aQuery, &rv);

nit: aListener -> listener
Attachment #780642 - Flags: review+
Flags: needinfo?(paolo.mozmail)
This was caused by https://bugzilla.mozilla.org/show_bug.cgi?id=891993, which was not in my try push. Sorry about that. I pinged dholbert to see if I could revert.
Chatted with dholbert, he's going to back out FAIL_ON_WARNINGS (and I will fix non protobuf related warnings before relanding)
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #74)
> (and I will
> fix non protobuf related warnings before relanding)

(In particular, please define ApplicationReputationQuery as "class ApplicationReputationQuery MOZ_FINAL" instead of "class ApplicationReputationQuery", and remove the unused field ApplicationReputation::mType.)
Thanks Daniel! Try again with local warnings fixed and FAIL_ON_WARNINGS enabled: https://tbpl.mozilla.org/?tree=Try&rev=5f8239555046
https://hg.mozilla.org/mozilla-central/rev/68062bad2d33
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 901198
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: