Closed Bug 829832 Opened 12 years ago Closed 11 years ago

BackgroundFileSaver should support hash computation

Categories

(Core :: Networking: File, defect)

Product:
Core
Component:
Networking: File
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: mmc, Assigned: mmc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 17 obsolete files)

Compute sha256 hashes when downloading files in BackgroundFileSaver.
28.20 KB, patch
Details | Diff | Splinter Review 
When a file is saved to disk, BackgroundFileSaver should be able to compute a hash on it. This is required for computing application reputation (bug 662819) without incurring unnecessary reads or copies.
Assignee: nobody → mmc
Including ScopedNSSTypes.h is throwing a compile error that cert.h can't be found.

4:10.94 In file included from ../../../dist/include/ScopedNSSTypes.h:16:0,
 4:10.94                  from /home/mchew/mozilla-central/netwerk/base/src/nsFileStreams.cpp:6:
 4:10.94 ../../../dist/system_wrappers/cert.h:3:23: fatal error: cert.h: No such file or directory
 4:10.94 compilation terminated.
 4:11.02 

Contents of system_wrappers/cert.h:
mchew@mchew-12604:~/mozilla-central$ cat ../clean/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/system_wrappers/cert.h
#pragma GCC system_header
#pragma GCC visibility push(default)
#include_next <cert.h>
#pragma GCC visibility pop

and there's an additional cert.h in security/nss/lib/certdb/cert.h, I probably need to fix the include path somehow.
Attachment #701370 - Flags: feedback?(sstamm)
So... I think you can get around the include problem by including NSS headers when you compile.  I'm not sure this is the right way to do it, but it got me past that error and on to some other ones.

Example:  (bsmith, mayhemer: there's gotta be a better way, what do you think?)

diff -r b286a676bed2 netwerk/base/src/Makefile.in
--- a/netwerk/base/src/Makefile.in	Fri Jan 11 16:26:27 2013 -0800
+++ b/netwerk/base/src/Makefile.in	Fri Jan 11 17:31:29 2013 -0800
@@ -78,16 +78,29 @@ CPPSRCS		= \
 		nsPreloadedStream.cpp \
 		nsStreamListenerWrapper.cpp \
 		ProxyAutoConfig.cpp \
 		Dashboard.cpp \
 		NetworkActivityMonitor.cpp \
 		$(NULL)
 
 LOCAL_INCLUDES	+= -I$(topsrcdir)/dom/base
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/nss
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/util
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/softoken
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/pk11wrap
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/pkcs7
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/certdb
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/cryptohi
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/dev
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/base
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/pki
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/smime
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/freebl/
+LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/ssl
 
 ifeq ($(MOZ_WIDGET_TOOLKIT),os2)
 	CPPSRCS += nsURLHelperOS2.cpp
 else
 ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
 	CPPSRCS += nsURLHelperWin.cpp
 	CPPSRCS += nsNativeConnectionHelper.cpp
 	CPPSRCS += nsAutodialWin.cpp
Flags: needinfo?(honzab.moz)
Attachment #701370 - Flags: feedback?(sstamm)
(In reply to Sid Stamm [:geekboy] from comment #2)
> +LOCAL_INCLUDES += -I$(topsrcdir)/security/nss/lib/nss
[...]

No, this is not a good way of doing things. The *public* header files for NSS get published to dist/include/nss during the build and that path is in the include path for all of Gecko as of a couple of months ago.

try |make -C security/build| and see if that causes your subsequent build to succeed. If it does, it probably means that you have to move security/build in front of netwerk in toolkit/toolkit-tiers.mk and/or toolkit/toolkit-makefiles.sh.
(In reply to Brian Smith (:bsmith) from comment #3)
> try |make -C security/build|

that is, |make -C $OBJDIR/security/build|, where $OBJDIR is your objdir.
FWIW, there is an already-r+d patch for computing **MD5** signatures on an HTTP response in bug 232030. It should be simple to modify it to computer SHA256 hashes instead.

Computing the hash in the HTTP Channel seems like a much better place than computing it in nsIFileOutputStream because computing it in the HTTP channel is more general-purpose as it could be used for things that aren't downloads to disk. We have other bugs asking for such functionality for other reasons.

I recommend working with the Necko peer that you're planning to have review your patch to make sure they agree with the approach, if it requires modifying Necko.

> nsRefPtr<Digest> mDigest;

Digest mDigest; (Digest is not a reference counted type.)

> PK11Context *mDigestContext;

ScopedPK11Context mDigestContext;

ScopedPK11Context is the smart pointer type for PK11Context objects.
(In reply to Brian Smith (:bsmith) from comment #5)
> Computing the hash in the HTTP Channel seems like a much better place than
> computing it in nsIFileOutputStream because computing it in the HTTP channel
> is more general-purpose as it could be used for things that aren't downloads
> to disk. We have other bugs asking for such functionality for other reasons.

Never mind. Obviously, it doesn't help ftp:// to have the hash computation in nsHttpChannel.
Since BackgroundFileSaver may open several file output streams (the target file
may be closed, copied, then reopened for append), there should be the ability to
preserve the hash computation parameters across the different output streams.

If you already considered that, then never mind - but next time feel free to post
patches without the "don't look" notice ;-)
Hi Paolo,

I would like to be able to preserve hash state across different output streams -- but it seems that PK11Context is not XPCOM-ified and therefore can't be touched in BackgroundFileSaver, and has to live solely in nsFileStreams.cpp. If there's a workaround, please let me know.

bsmith, make -C obj/security/build fails with linking errors on mozsqlite, and according to gps's blog mach should be treated as a black-box replacement, with make -C objdir increasingly guaranteed not to work.

I haven't found the right incantation of include orders in .mk files to make this work yet, but perhaps this is a sign that copying headers wholesale into a global includes dir, with no namespacing and little to no sign to where headers are coming from, is not a great idea.

Monica
This patch makes including "ScopedNssTypes" in BackgroundFileSaver compile. It moves security/build (NSS) and sqlite3 (needed for nss) above netwerk.

Local build on linux works, tbpl:

https://tbpl.mozilla.org/?tree=Try&rev=a9088515c6c9
Attachment #701370 - Attachment is obsolete: true
Attachment #701983 - Flags: review?(mh+mozilla)
Attachment #701983 - Flags: feedback?(bsmith)
Attachment #701983 - Attachment is patch: true
Comment on attachment 701983 [details] [diff] [review]
Move nss and sqlite3 above network in toolkit-tiers

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

From a build system perspective, this is good. From a platform perspective, I'm not sure we want to use NSS for hash computation. We have SHA1 in MFBT for a reason.
Attachment #701983 - Flags: review?(mh+mozilla) → review+
Seeing as how SHA 1 has been broken for 8 years, it's probably a good thing to have other hashes available. Thanks, Mike.
Personally, when you are using this just to compare content has been altered, you don't need any strong hashing function.  Offline App Cache update check is using md5 to save resources.  This is not security-sensitive check.  I don't see any valid attack here.
Flags: needinfo?(honzab.moz)
Fix patch message to include bug number and reviewer.
Attachment #701983 - Attachment is obsolete: true
Attachment #701983 - Flags: feedback?(bsmith)
Fair enough -- the reason BackgroundFileSaver needs the hash is not as a checksum but to pass it along to DownloadManager so it can query whether or not a downloaded binary is malware (bug 662819)
(In reply to Monica Chew from comment #12)
> Seeing as how SHA 1 has been broken for 8 years, it's probably a good thing
> to have other hashes available. Thanks, Mike.

Let me rephrase my comment: we added SHA1 in MFBT for a reason. If we need other hashes (which, as Honza mentions, is in itself questionable), we /may/ want to add them to MFBT instead of using NSS.
(In reply to Mike Hommey [:glandium] from comment #16)
> Let me rephrase my comment: we added SHA1 in MFBT for a reason. If we need
> other hashes (which, as Honza mentions, is in itself questionable), we /may/
> want to add them to MFBT instead of using NSS.

Instead we should remove the crypto stuff from mfbt. See bug 830880.
Hrm, seems like that build config fix should have been landed in a separate bug - it's generally a bad idea to land multiple not-directly-related patches in the same bug.
Whiteboard: [leave open]
Thanks Gavin, I'll try to be more fastidious in the future.
Whiteboard: [leave open]
Files:
- BackgroundFileSaver: In ProcessStateChange before the call to NS_AsyncCopy, (optionally) read the existing file from disk if it's been cancelled and resumed, then wrap the output stream in DigestStream
- DigestStreams: Add DigestStream for consuming an input stream and computing the hash on that, and DigestOutputStream for wrapping nsIOutputStream
- ScopedNSSTypes: Fix typo in comment, make Digest sub-classable
- test_backgroundfilesaver.js: Update the verifyContents check to also check that the hash was computed correctly.

TBPL: https://hg.mozilla.org/try/rev/d7b7977e910b
Attachment #707366 - Flags: review?(bsmith)
Comment on attachment 707366 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

::: security/manager/ssl/src/DigestStreams.cpp
@@ +1,1 @@
> +#include <stdio.h>

oops, sloppy include, will fix.

::: security/manager/ssl/src/Makefile.in
@@ +20,5 @@
>  LIBXUL_LIBRARY	= 1
>  
>  CPPSRCS = 				\
>  	CryptoTask.cpp \
> +	DigestStreams.cpp \

Not sure where these belong, I just took a best guess.
Comment on attachment 707366 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

I'm making a drive-by review on this code, I hope to be helpful here. This is a very good work already, I'm looking into ways to improve it even more!

My main question is if there is a specific reason why we don't simply make nsICryptoHash thread-safe, instead of copy-pasting its implementation. We're eventually going to use the hashing functions from JavaScript, and having nsIBackgroundFileSaver interact with nsICryptoHash seems simpler than reimplementing its methods on it (for example, to allow selection of the hash type). Maybe you already took this into consideration, I'm asking because I didn't see this alternative (or the reasons to avoid it) discussed on the bug.

The nsIOutputStream wrapper is a good idea, but it doesn't look reusable as currently implemented, since it handles only the specific calls used today by BackgroundFileSaver, and fails on others. So, I'm not sure it's worth sharing this wrapper. I'd just move it into BackgroundFileSaver.cpp and have it call into nsICryptoHash.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +424,5 @@
> +  if (NS_FAILED(rv)) {
> +    // The file can't be found.
> +    PR_LOG(fslog, PR_LOG_DEBUG, ("BackgroundFileSaver: Nothing to update"));
> +  } else {
> +    rv = istream->Available(&available);

Per documentation, Available is not guaranteed to return the total size, even for local files.

In any case, BackgroundFileSaver currently can only start with a blank output file (see creationIoFlags), unless we are renaming in the middle of the operation, in which case we should just continue the existing hash computation with the same digest context (we explicitly don't want to re-read the file that we just moved using the optimized operating system calls).

So, for now, we can just avoid this code and move support for resuming to a follow-up bug.

@@ +725,5 @@
>                                                    nsISupports *aContext)
>  {
> +  // This function is never called, because
> +  // nsExternalHelperAppService::OnStartRequest doesn't call it. This is
> +  // possibly a bug.

Yeah, nsExternalHelperAppService makes assumptions here. It doesn't hurt to update it to invoke these methods, please file a follow-up bug to have this fixed. The comment here should instead be removed from the patch.

::: netwerk/base/src/BackgroundFileSaver.h
@@ +246,5 @@
>  };
>  
>  ////////////////////////////////////////////////////////////////////////////////
> +//// BackgroundFileSaverStreamListener. This class is instantiated by
> +// nsExternalHelperAppService.

This is not the only consumer, actually. And hopefully downloads won't follow that code path anymore in the near future :-) This is why we don't put such comments in the code, in general, as they tend to become outdated quickly.

::: netwerk/test/unit/test_backgroundfilesaver.js
@@ +16,5 @@
>  //// Globals
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +// Dummy call to initalize psm
> +Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

Can you explain why this call is needed? In any case, we generally put initialization inside an add_task test at the top of the tests section.

@@ +118,5 @@
>   * @return {Promise}
>   * @resolves When the operation completes.
>   * @rejects Never.
>   */
> +function promiseVerifyContents(aFile, aExpectedContents, aSaver) {

I'd keep the promiseVerifyContents function generic so that it can be used for arbitrary data (the existing "TEST_DATA_SHORT.length" test is just for logging, might as well be a hard-coded constant).

You can just do_check_eq(saver.hash, <appropriate constant>) in the test functions themselves. I'd recommend adding a specific test case for the hash, and also checking it in test_combinations, but leaving the hash test out of the other test cases (so it can more easily be determined what's exactly broken in case of regressions, by seeing which tests continue to work and which ones fail).
Hi Paolo,

Thank you again for your thoughtful comments. I'll respond in depth later but wanted to address the nsICryptoHash question now in case there needs to be more discussion.

After chatting with bsmith, he strongly advised that nsICryptoHash is deprecated, should only be used from JS, and to use ScopedNSSTypes instead. I am not sure of the work involved in making nsICryptoHash threadsafe, and am probably not the best person to do that work in any case, especially since NSS already seems to be threadsafe and I am no crypto expert.

Given that information, is simply adding a streaming interface to ScopedNSSTypes acceptable? bsmith's requirements were that ScopedNSSTypes.h must never know anything about xpcom.

So far as choosing the hash type from the caller -- the relevant caller in this case is nsExternalHelperAppService. I could add an additional attribute hashType to nsIBackgroundSaver.idl, but it did not seem to add any benefits to have nsExternalHelperAppService set the hash type rather than BackgroundFileSaver. If there's a use case I'm missing, please let me know.

Thanks,
Monica
(In reply to Monica Chew from comment #25)
> After chatting with bsmith, he strongly advised that nsICryptoHash is
> deprecated, should only be used from JS, and to use ScopedNSSTypes instead.

That's fine, if the goal is to deprecate the interface. I'm curious to hear
from bsmith about what are the reason for deprecation, that is if there are
design flaws or if it's just located in the wrong part of the code base.

> Given that information, is simply adding a streaming interface to
> ScopedNSSTypes acceptable? bsmith's requirements were that ScopedNSSTypes.h
> must never know anything about xpcom.

It would still make sense to have the streaming wrapper local to
BackgroundFileSaver, if it only implements a minimal subset of
nsIOutputStream.

> So far as choosing the hash type from the caller -- the relevant caller in
> this case is nsExternalHelperAppService. I could add an additional attribute
> hashType to nsIBackgroundSaver.idl, but it did not seem to add any benefits
> to have nsExternalHelperAppService set the hash type rather than
> BackgroundFileSaver. If there's a use case I'm missing, please let me know.

It's OK to support more hash types in a follow-up (even more than one), when
we need them, for example to compare the final file with hashes provided by the
server or indicated in a download link (that could just be MD5).

However, even in the first version, hash computation should only be done if
explicitly required by the caller. We might as well use an appropriately named
attribute to select the hash algorithm (that has only one valid value for now,
like "sha256"), it may be a string or even a bit mask (like file I/O flags).
(In reply to Paolo Amadini [:paolo] from comment #26)
> > Given that information, is simply adding a streaming interface to
> > ScopedNSSTypes acceptable? bsmith's requirements were that ScopedNSSTypes.h
> > must never know anything about xpcom.
> 
> It would still make sense to have the streaming wrapper local to
> BackgroundFileSaver, if it only implements a minimal subset of
> nsIOutputStream.

There is one more future caller of DigestStream: security/manager/ssl/src/JARSignatureVerification::FindAndLoadOneEntry, which is manually reading chunks of a file and feeding it to DigestOp, so I don't think DigestStream and DigestOutputStream should belong BackgroundFileSaver. I'm happy to move it somewhere more stream-y, so long as it doesn't require reviewers other than you and bsmith.
Recent-ish green try:

https://tbpl.mozilla.org/?tree=Try&rev=6ffafc7a0373
Attachment #702405 - Attachment is obsolete: true
Attachment #707366 - Attachment is obsolete: true
Attachment #707366 - Flags: review?(bsmith)
Attachment #708391 - Flags: review?(paolo.mozmail)
Attachment #708391 - Flags: review?(bsmith)
Ready for review.

> ::: netwerk/base/src/BackgroundFileSaver.cpp
> @@ +424,5 @@
> > +  if (NS_FAILED(rv)) {
> > +    // The file can't be found.
> > +    PR_LOG(fslog, PR_LOG_DEBUG, ("BackgroundFileSaver: Nothing to update"));
> > +  } else {
> > +    rv = istream->Available(&available);
> 
> Per documentation, Available is not guaranteed to return the total size,
> even for local files.

Removed and added UpdateHashFromExisting.
 
> In any case, BackgroundFileSaver currently can only start with a blank
> output file (see creationIoFlags), unless we are renaming in the middle of
> the operation, in which case we should just continue the existing hash
> computation with the same digest context (we explicitly don't want to
> re-read the file that we just moved using the optimized operating system
> calls).
> 
> So, for now, we can just avoid this code and move support for resuming to a
> follow-up bug.

I think this code is functioning correctly in this patch. If you agree, I'd like to avoid the makework of filing another bug and going through another review process. bsmith's recommendation was that the cancel/resume case was important enough that we should try to get it right the first iteration -- however, I'd like to make progress on this, so any way is acceptable to the reviewers, I'll go ahead and do.

> @@ +725,5 @@
> >                                                    nsISupports *aContext)
> >  {
> > +  // This function is never called, because
> > +  // nsExternalHelperAppService::OnStartRequest doesn't call it. This is
> > +  // possibly a bug.
> 
> Yeah, nsExternalHelperAppService makes assumptions here. It doesn't hurt to
> update it to invoke these methods, please file a follow-up bug to have this
> fixed. The comment here should instead be removed from the patch.

Comments removed.

> ::: netwerk/base/src/BackgroundFileSaver.h
> @@ +246,5 @@
> >  };
> >  
> >  ////////////////////////////////////////////////////////////////////////////////
> > +//// BackgroundFileSaverStreamListener. This class is instantiated by
> > +// nsExternalHelperAppService.
> 
> This is not the only consumer, actually. And hopefully downloads won't
> follow that code path anymore in the near future :-) This is why we don't
> put such comments in the code, in general, as they tend to become outdated
> quickly.

This comment I extended and left. There's probably something wrong with my workflow here, but I've had a hard time identifying what parts of the tree are relevant (for this bug and in general). My approach to figuring things out has been to search mxr (which usually breaks because I'm searching for more than 29 chars, the glimpse limit), search for identifiers (which in this case breaks because creation is happening through contract id), and finally turning on NSPR logging for all modules hoping that the caller I need is outputting something sufficiently unique to figure out where the relevant code is. So I'd tend to support more comments even if they may be deprecated, over no comments.

That being said, if you really hate this comment, I will remove it.
 
> ::: netwerk/test/unit/test_backgroundfilesaver.js
> @@ +16,5 @@
> >  //// Globals
> >  
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +// Dummy call to initalize psm
> > +Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);
> 
> Can you explain why this call is needed? In any case, we generally put
> initialization inside an add_task test at the top of the tests section.

This is some black magic recommended by bsmith. I put it in test_setup and marked it as such.

> @@ +118,5 @@
> >   * @return {Promise}
> >   * @resolves When the operation completes.
> >   * @rejects Never.
> >   */
> > +function promiseVerifyContents(aFile, aExpectedContents, aSaver) {
> 
> I'd keep the promiseVerifyContents function generic so that it can be used
> for arbitrary data (the existing "TEST_DATA_SHORT.length" test is just for
> logging, might as well be a hard-coded constant).
> 
> You can just do_check_eq(saver.hash, <appropriate constant>) in the test
> functions themselves. I'd recommend adding a specific test case for the
> hash, and also checking it in test_combinations, but leaving the hash test
> out of the other test cases (so it can more easily be determined what's
> exactly broken in case of regressions, by seeing which tests continue to
> work and which ones fail).

I limited hash computation to test_combinations and setTarget_after_close (which had exposed bug in an earlier try run), but adding another test case seemed superfluous given that it's already checking 32 combinations.

Thanks,
Monica
Comment on attachment 708391 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +451,5 @@
>  
>    // Create the target file, or append to it if we already started writing it.
>    nsCOMPtr<nsIOutputStream> outputStream;
>    rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream),
> +                                   target,

gratuitous change, will revert

@@ +770,5 @@
>  NS_IMETHODIMP
>  BackgroundFileSaverStreamListener::OnStopRequest(nsIRequest *aRequest,
>                                                   nsISupports *aContext,
>                                                   nsresult aStatusCode)
>  {

spurious comment removal, will fix
(In reply to Monica Chew from comment #29)
> bsmith's recommendation was that the cancel/resume case was
> important enough that we should try to get it right the first iteration

The misunderstanding here is that BackgroundFileSaver doesn't currently support
resuming at all, and this patch is not really adding support for resuming yet.

BackgroundFileSaver starts saving data to disk when the first call to setTarget
is made, and when this happens, the file indicated to setTarget is deleted if
it exists (as you can see documented in the IDL file). Subsequent calls to
setTarget will move this partially saved file to another location, before the
writing resumes. But in this case, we are always appending data to the file
that we were already saving using the same instance of BackgroundFileSaver.
Form the point of view of the consumer, it is as if we are still writing to
the same file as before.

If a download is canceled, and then resumed form the user interface, we go
through a totally different code path that does not involve BackgroundFileSaver.

Fixing that is a much bigger work, this is why I suggest you don't do this in
this bug, since it will probably just slow you down on this one, since that
involves touching other code areas.

Actually, I've tested that your patch works exactly in the same way if you
remove the call to UpdateHashFromExisting, and this also saves a couple of
seconds of execution time on my machine. I recommend removing that code
entirely to move faster on this patch.

> This comment I extended and left. There's probably something wrong with my
> workflow here, but I've had a hard time identifying what parts of the tree
> are relevant (for this bug and in general). [...] So I'd tend to support
> more comments even if they may be deprecated, over no comments.
> 
> That being said, if you really hate this comment, I will remove it.

Finding one's way around our huge code base is hard for everyone, and requires
a lot of time. I've really no issue at all about keeping the comment if you
think it can help, it's just that in my experience an outdated comment might
be as misleading as no comment at all.

> > Can you explain why this call is needed? In any case, we generally put
> > initialization inside an add_task test at the top of the tests section.
> 
> This is some black magic recommended by bsmith. I put it in test_setup and
> marked it as such.

I'm afraid this is not a satisfying answer :-( If this call is needed only in
tests, it should be indicated in the comment, along with the reason why it's
needed (we don't really want to tell people to ask Brian!).

If this call is needed in production code, then this might be hiding a real
bug (possible race condition on browser startup), and we should have
BackgroundFileSaver initialize the service as needed.

> I limited hash computation to test_combinations and setTarget_after_close
> (which had exposed bug in an earlier try run), but adding another test case
> seemed superfluous given that it's already checking 32 combinations.

Test cases also work as short working code snippets explaining how to use the
component. One dedicated to explain "how do I activate hashing" could be useful,
but don't worry, we can also add it another time.
I've also put some more thought on the support for multiple hashing algorithms
in downloads. The general idea is that we might have a link in a web site that,
for the purpose of download integrity verification only (not for checking if
the download is malware), also includes a checksum (MD5 may be common). Example:

<a href="firefox.exe" md5="ABCDABCD...">

To support this, we'll need to compute two hashes at the same time on the same
download (because we'll not disable the malware check when the checksum is also
specified by the website).

I think we could have an interface for this in BackgroundFileSaver that allows
enabling a small set of common hashing algorithms individually. This would look
a bit different from the bitmask I previously suggested, sorry for the possibly
duplicate work in the past iteration. Please use these attributes:

attribute bool sha256enabled;
readonly attribute ACString sha256hash;

In the future, we'll add md5enabled/md5hash and possibly other hash types. This
is much easier to access from JavaScript than arrays of objects to read the
result of arbitrary hashes.
Comment on attachment 708391 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

For the rest, the patch looks very near to completion (and the remaining changes are really easy to do now).

::: netwerk/base/public/nsIBackgroundFileSaver.idl
@@ +49,5 @@
>     */
>    attribute nsIBackgroundFileSaverObserver observer;
>  
>    /**
> +   * The hash associated with the file that was downloaded.

Please specify how the hash is encoded. If I understand correctly, these are plain octets, not the ASCII hexadecimal representation, right?

Also, specify at which time in the lifetime of the object the value should be read (that is, after onSaveComplete is called). Look at the other comments for an idea about the style to use.

@@ +57,5 @@
> +  /**
> +   * The hash algorithm used to compute the hash, from secoidt.h.
> +   * SEC_OID_UNKNOWN (0) means the hash will not be computed.
> +   */
> +  attribute long hashType;

On the corresponding sha256enabled attribute, please specify at which time in the lifetime of the object the value should be set (that is, before setTarget is called for the first time).

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +207,5 @@
> +
> +nsresult
> +BackgroundFileSaver::GetHash(nsACString& aHash)
> +{
> +  return NS_CStringCopy(aHash, mHash);

Mutex protection is needed on mHash, it's accessed by two threads.

Also, can aHash be null? In that case, NS_ENSURE_ARG is needed.

@@ +314,5 @@
> +  if (mHashType != SEC_OID_UNKNOWN && !mDigestContext) {
> +    mDigestContext =
> +      PK11_CreateDigestContext(static_cast<SECOidTag>(mHashType));
> +    NS_ENSURE_TRUE(mDigestContext, NS_ERROR_OUT_OF_MEMORY);
> +  }

This block should be moved inside ProcessStateChange for proper error handling.

@@ +445,5 @@
> +    // cancel/resume), update the hash.
> +    if (mDigestContext) {
> +      rv = UpdateHashFromExisting(mDigestContext, target);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }

Just remove.

@@ +456,2 @@
>                                     PR_WRONLY | creationIoFlags, 0600);
>    NS_ENSURE_SUCCESS(rv, rv);

nit: newline accidentally deleted.

@@ +467,5 @@
>  
>    // Start copying our input to the target file.  No errors can be raised past
>    // this point if the copy starts, since they should be handled by the thread.
>    {
>      MutexAutoLock lock(mLock);

nit: newline accidentally deleted.

@@ +542,5 @@
> +  // Finish computing the hash
> +  if (!failed && mDigestContext) {
> +    Digest d;
> +    rv = d.End(SEC_OID_SHA256, mDigestContext);
> +    NS_CStringCopy(mHash, nsDependentCSubstring(char_ptr_cast(d.get().data), d.get().len));

Mutex protection (ideally, only around the NS_CStringCopy call).

@@ +596,5 @@
>  
> +nsresult
> +BackgroundFileSaver::UpdateHashFromExisting(PK11Context* aContext,
> +                                            nsIFile* aFile)
> +{

Just remove the function.

::: netwerk/base/src/BackgroundFileSaver.h
@@ +161,5 @@
>  
> +  /**
> +   * Used to calculate the file hash.
> +   */
> +  ScopedPK11Context mDigestContext;

Please describe in the comment when this is initialized, and that this keeps state across renames.

@@ +162,5 @@
> +  /**
> +   * Used to calculate the file hash.
> +   */
> +  ScopedPK11Context mDigestContext;
> +  nsAutoCString mHash;

Please move the mHash declaration to the section dedicated to variables that are shared between the threads.

@@ +166,5 @@
> +  nsAutoCString mHash;
> +  /**
> +   * An enum corresponding to SEC_OID_<hashType> from secoidt.h.
> +   */
> +  int32_t mHashType;

Same mutex protection and move is required for sha256enabled.

@@ +223,5 @@
> +  /**
> +   * Called in the cancel/resume case when there is an existing file to append.
> +   * The caller owns aContext and aFile.
> +   */
> +  nsresult UpdateHashFromExisting(PK11Context* aContext, nsIFile* aFile);

Just remove.

::: security/manager/ssl/src/DigestStreams.cpp
@@ +5,5 @@
> +#include "nsIInputStream.h"
> +
> +namespace mozilla {
> +
> +DigestStream::DigestStream(PK11Context* digestContext) :

Just remove DigestStream.
Attachment #708391 - Flags: review?(paolo.mozmail)
Attachment #708391 - Flags: review?(bsmith)
(In reply to Paolo Amadini [:paolo] from comment #33)
> Please specify how the hash is encoded. If I understand correctly, these are
> plain octets, not the ASCII hexadecimal representation, right?
> 
> Also, specify at which time in the lifetime of the object the value should
> be read (that is, after onSaveComplete is called). Look at the other
> comments for an idea about the style to use.

Improved comments.
 
> @@ +57,5 @@
> > +  /**
> > +   * The hash algorithm used to compute the hash, from secoidt.h.
> > +   * SEC_OID_UNKNOWN (0) means the hash will not be computed.
> > +   */
> > +  attribute long hashType;
> 
> On the corresponding sha256enabled attribute, please specify at which time
> in the lifetime of the object the value should be set (that is, before
> setTarget is called for the first time).

Here too.

> ::: netwerk/base/src/BackgroundFileSaver.cpp
> @@ +207,5 @@
> > +
> > +nsresult
> > +BackgroundFileSaver::GetHash(nsACString& aHash)
> > +{
> > +  return NS_CStringCopy(aHash, mHash);
> 
> Mutex protection is needed on mHash, it's accessed by two threads.

Wrapped MutexAutoLock around accesses to mHash and mHashType.
 
> Also, can aHash be null? In that case, NS_ENSURE_ARG is needed.

It is not a pointer type so I think we're ok.

> @@ +314,5 @@
> > +  if (mHashType != SEC_OID_UNKNOWN && !mDigestContext) {
> > +    mDigestContext =
> > +      PK11_CreateDigestContext(static_cast<SECOidTag>(mHashType));
> > +    NS_ENSURE_TRUE(mDigestContext, NS_ERROR_OUT_OF_MEMORY);
> > +  }
> 
> This block should be moved inside ProcessStateChange for proper error
> handling.

I think I moved it to the right place.
 
> @@ +445,5 @@
> > +    // cancel/resume), update the hash.
> > +    if (mDigestContext) {
> > +      rv = UpdateHashFromExisting(mDigestContext, target);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +    }
> 
> Just remove.

Removed.

> @@ +456,2 @@
> >                                     PR_WRONLY | creationIoFlags, 0600);
> >    NS_ENSURE_SUCCESS(rv, rv);
> 
> nit: newline accidentally deleted.

Fixed.

> 
> @@ +467,5 @@
> >  
> >    // Start copying our input to the target file.  No errors can be raised past
> >    // this point if the copy starts, since they should be handled by the thread.
> >    {
> >      MutexAutoLock lock(mLock);
> 
> nit: newline accidentally deleted.

Fixed.
 
> @@ +542,5 @@
> > +  // Finish computing the hash
> > +  if (!failed && mDigestContext) {
> > +    Digest d;
> > +    rv = d.End(SEC_OID_SHA256, mDigestContext);
> > +    NS_CStringCopy(mHash, nsDependentCSubstring(char_ptr_cast(d.get().data), d.get().len));
> 
> Mutex protection (ideally, only around the NS_CStringCopy call).

Fixed.
 
> @@ +596,5 @@
> >  
> > +nsresult
> > +BackgroundFileSaver::UpdateHashFromExisting(PK11Context* aContext,
> > +                                            nsIFile* aFile)
> > +{
> 
> Just remove the function.

Deleted.
 
> ::: netwerk/base/src/BackgroundFileSaver.h
> @@ +161,5 @@
> >  
> > +  /**
> > +   * Used to calculate the file hash.
> > +   */
> > +  ScopedPK11Context mDigestContext;
> 
> Please describe in the comment when this is initialized, and that this keeps
> state across renames.

Commented.
 
> @@ +162,5 @@
> > +  /**
> > +   * Used to calculate the file hash.
> > +   */
> > +  ScopedPK11Context mDigestContext;
> > +  nsAutoCString mHash;
> 
> Please move the mHash declaration to the section dedicated to variables that
> are shared between the threads.

Moved.
 
> @@ +166,5 @@
> > +  nsAutoCString mHash;
> > +  /**
> > +   * An enum corresponding to SEC_OID_<hashType> from secoidt.h.
> > +   */
> > +  int32_t mHashType;
> 
> Same mutex protection and move is required for sha256enabled.

Locked.
 
> @@ +223,5 @@
> > +  /**
> > +   * Called in the cancel/resume case when there is an existing file to append.
> > +   * The caller owns aContext and aFile.
> > +   */
> > +  nsresult UpdateHashFromExisting(PK11Context* aContext, nsIFile* aFile);
> 
> Just remove.

Removed.
 
> ::: security/manager/ssl/src/DigestStreams.cpp
> @@ +5,5 @@
> > +#include "nsIInputStream.h"
> > +
> > +namespace mozilla {
> > +
> > +DigestStream::DigestStream(PK11Context* digestContext) :
> 
> Just remove DigestStream.

Removed.
Hi Paolo,

(In reply to Paolo Amadini [:paolo] from comment #31)
> (In reply to Monica Chew from comment #29)
> > bsmith's recommendation was that the cancel/resume case was
> > important enough that we should try to get it right the first iteration
> 
> The misunderstanding here is that BackgroundFileSaver doesn't currently
> support
> resuming at all, and this patch is not really adding support for resuming
> yet.

Thanks for explaining, I will file a bug on myself.

> > > Can you explain why this call is needed? In any case, we generally put
> > > initialization inside an add_task test at the top of the tests section.
> > 
> > This is some black magic recommended by bsmith. I put it in test_setup and
> > marked it as such.
> 
> I'm afraid this is not a satisfying answer :-( If this call is needed only in
> tests, it should be indicated in the comment, along with the reason why it's
> needed (we don't really want to tell people to ask Brian!).

I know what you mean, I try to avoid asking Brian stuff as well :)

> If this call is needed in production code, then this might be hiding a real
> bug (possible race condition on browser startup), and we should have
> BackgroundFileSaver initialize the service as needed.

Sorry, I didn't understand your initial question. This was indeed a bug in the c code, I've added a call to initialize NSS there as well.
 
Monica
(In reply to Paolo Amadini [:paolo] from comment #32)
> I've also put some more thought on the support for multiple hashing
> algorithms
> in downloads. The general idea is that we might have a link in a web site
> that,
> for the purpose of download integrity verification only (not for checking if
> the download is malware), also includes a checksum (MD5 may be common).
> Example:
> 
> <a href="firefox.exe" md5="ABCDABCD...">
> 
> To support this, we'll need to compute two hashes at the same time on the
> same
> download (because we'll not disable the malware check when the checksum is
> also
> specified by the website).
> 
> I think we could have an interface for this in BackgroundFileSaver that
> allows
> enabling a small set of common hashing algorithms individually. This would
> look
> a bit different from the bitmask I previously suggested, sorry for the
> possibly
> duplicate work in the past iteration. Please use these attributes:
> 
> attribute bool sha256enabled;
> readonly attribute ACString sha256hash;
> 
> In the future, we'll add md5enabled/md5hash and possibly other hash types.
> This
> is much easier to access from JavaScript than arrays of objects to read the
> result of arbitrary hashes.

I changed these in the IDL but not the in the BackgroundFileSaver.h -- to properly support this in DigestOutputStream we need to pass in a vector of digest contexts, which seems like overkill for our current use case.

        Attachment #708391 -
        Attachment is obsolete: true

        Attachment #709137 -
        Flags: review?(paolo.mozmail)

        Attachment #709137 -
        Flags: review?(bsmith)

    
    

    
  


  
Whoops, stale patch.

        Attachment #709137 -
        Attachment is obsolete: true

        Attachment #709137 -
        Flags: review?(paolo.mozmail)

        Attachment #709137 -
        Flags: review?(bsmith)

        Attachment #709138 -
        Flags: review?(paolo.mozmail)

        Attachment #709138 -
        Flags: review?(bsmith)

    
    

    
  
Comment on attachment 709138 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

::: netwerk/test/unit/test_backgroundfilesaver.js
@@ +262,5 @@
>  
>  add_task(function test_setup()
>  {
> +  // Dummy call to initalize psm, so hash computations work correctly.
> +  Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

maybe we don't need this now?

    
    

    
  
Comment on attachment 709138 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

This is ready to go with just a few changes, thanks!

Before landing you'll need final review from bsmith also, in particular on DigestOutputStream, but it should be fast.

(In reply to Monica Chew from comment #36)
> I changed these in the IDL but not the in the BackgroundFileSaver.h -- to
> properly support this in DigestOutputStream we need to pass in a vector of
> digest contexts, which seems like overkill for our current use case.

That's the way to go!

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +351,5 @@
>      mWorkerThreadAttentionRequested = false;
> +
> +    // Lazily initialize mDigestContext, in case mHashType is valid
> +    if (mHashType != SEC_OID_UNKNOWN && !mDigestContext) {
> +      // Dummy to initialize psm

Change comment to something like "Ensure that the Personal Security Manager service is initialized.  This is required for PK11_CreateDigestContext to operate correctly."

@@ +359,5 @@
> +      mHash.Truncate();
> +      mDigestContext =
> +        PK11_CreateDigestContext(static_cast<SECOidTag>(mHashType));
> +      NS_ENSURE_TRUE(mDigestContext, NS_ERROR_OUT_OF_MEMORY);
> +    }

mHash.Truncate() is probably unneeded here, and you should read mHashType to a local variable so you can move the code block outside of the mutex lock. We don't want to keep the lock while invoking getService, PK11_CreateDigestContext, or other operations that we don't know how long could take, potentially blocking the main thread on the lock.

@@ +540,5 @@
>  
> +  // Finish computing the hash
> +  if (!failed && mDigestContext) {
> +    Digest d;
> +    rv = d.End(SEC_OID_SHA256, mDigestContext);

If this fails, we should at least do NS_WARNING instead of copying the string. (It's not the best possible solution since the consumer would get a successful onSaveComplete but a missing hash, but it should be acceptable since it's quite uncommon to fail here.)

@@ +746,5 @@
>  BackgroundFileSaverStreamListener::OnStopRequest(nsIRequest *aRequest,
>                                                   nsISupports *aContext,
>                                                   nsresult aStatusCode)
>  {
> +

nit: unneded new line.

        Attachment #709138 -
        Flags: review?(paolo.mozmail) → review+

    
    

    
  
Comment on attachment 709138 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +351,5 @@
>      mWorkerThreadAttentionRequested = false;
> +
> +    // Lazily initialize mDigestContext, in case mHashType is valid
> +    if (mHashType != SEC_OID_UNKNOWN && !mDigestContext) {
> +      // Dummy to initialize psm

MOZ_ASSERT(NS_IsMainThread());

::: netwerk/base/src/BackgroundFileSaver.h
@@ +17,5 @@
>  #include "nsIAsyncOutputStream.h"
>  #include "nsIBackgroundFileSaver.h"
>  #include "nsIStreamListener.h"
>  #include "nsStreamUtils.h"
> +// Can't declare mozilla::ScopedPK11Context here because of templatization

This comment is unneeded.

@@ +153,5 @@
> +
> +  /**
> +   * An enum corresponding to SEC_OID_<hashType> from secoidt.h.
> +   */
> +  int32_t mHashType;

Isn't this unnecssary? It's always going to be SHA256.

::: netwerk/test/unit/test_backgroundfilesaver.js
@@ +262,5 @@
>  
>  add_task(function test_setup()
>  {
> +  // Dummy call to initalize psm, so hash computations work correctly.
> +  Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

It seems you're already doing it in BackgroundFileSaver so it should be unnecessary here.

::: security/manager/ssl/src/DigestStreams.cpp
@@ +35,5 @@
> +}
> +
> +NS_IMETHODIMP
> +DigestOutputStream::Write(const char* aBuf, uint32_t aCount, uint32_t* retval)
> +{

Must check if NSS is shut down here, which means you must implement nsNSSShutDownObject, so you can write:

nsNSSShutDownPreventionLock lock;
if (isAlreadyShutDown()) {
  return NS_ERROR_NOT_AVAILABLE;
}

::: security/manager/ssl/src/DigestStreams.h
@@ +9,5 @@
> +
> +namespace mozilla {
> +
> +// A wrapper around Digest that can consume from an nsIInputStream.
> +class DigestStream : public Digest

DigestStream shouldn't inherit from Digest; it should contain a digest.

::: security/manager/ssl/src/ScopedNSSTypes.h
@@ +143,5 @@
>   *   for (...) {
>   *      rv = MapSECStatus(PK11_DigestOp(digestContext, ...));
>   *      NS_ENSURE_SUCCESS(rv, rv);
>   *   }
> + *   rv = digest.End(SEC_OID_SHA1, digestContext);

Must ensure NSS isn't shut down before calling this function.

@@ +156,5 @@
>      item.data = buf;
>      item.len = 0;
>    }
>  
> +  virtual ~Digest()

Let's not do this.

        Attachment #709138 -
        Flags: review?(bsmith) → review-

    
    

    
  
(In reply to Brian Smith (:bsmith) from comment #41)
> > +      // Dummy to initialize psm
> 
> MOZ_ASSERT(NS_IsMainThread());

I get this means that the service must be initialized in the main thread, so
we probably could do this lazily in the sha256Enabled setter, when set to true.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Broken patch (obsolete)
        — 
          — Splinter Review
      

    


  
Subclassing nsNSSShutdownObject causes xpcshell test to break.

        Attachment #709138 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        xpcshell logs (obsolete)
        — 
      

    


  

    
    

    
  
From the NSPR logs when running xpcshell test, something is not being called on the main thread when it should be, and it's not the call to initialize nss.

-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737ef300
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737c2700
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737c2800
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737edc00
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737c1700
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737c2600
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737edb00
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737c2500
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737ef600
-2143299136[7fde73714040]: Destroying nsStandardURL @7fde737c1600
-2143299136[7fde73714040]: DocLoader:7fde73714e10: Stop() called
-2143299136[7fde73714040]: DocLoader:7fde73714e10: deleted.
-2143299136[7fde73714040]: LOADGROUP [7371aae0]: Destroyed.
-2143299136[7fde73714040]: nsNSSComponent::dtor
-2143299136[7fde73714040]: WARNING: NS_ENSURE_TRUE(mMainThread) failed: file /home/mchew/mozilla-central/xpcom/threads/nsThreadManager.cpp, line 259
-2143299136[7fde73714040]: WARNING: NS_ENSURE_TRUE(thread) failed: file /home/mchew/mozilla-central/obj-x86_64-unknown-linux-gnu/xpcom/build/nsThreadUtils.cpp, line 174
1806038784[7fde73716680]: WARNING: NS_ENSURE_TRUE(mMainThread) failed: file /home/mchew/mozilla-central/xpcom/threads/nsThreadManager.cpp, line 250
1806038784[7fde73716680]: WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /home/mchew/mozilla-central/obj-x86_64-unknown-linux-gnu/xpcom/build/nsThreadUtils.cpp, line 161
-2143299136[7fde73714040]: nsNSSComponent::ShutdownNSS
-2143299136[7fde73714040]: Unloaded library /home/mchew/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin/libnssckbi.so
-2143299136[7fde73714040]: evaporating psm resources
-2143299136[7fde73714040]: now evaporating NSS resources

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Broken patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch currently works, but uncommenting nssShutDownObject stuff makes xpcshell-test break. I can't make it break manually, and I didn't get any other usable traces with minidump_stackwalk.

I moved NSS initialization to SetTarget.
Flags: needinfo?(bsmith)

        Attachment #709262 -
        Attachment is obsolete: true

    
    

    
  
OK, got it. Steps to get trace: make SOLO_FILE=<test_backgroundfilesaver.js> -C <path> check-interactive, then attach gdb to xpcshell, then type _execute_test(); in the interactive xpcshell, then continue in gdb, then exit in xpcshell.

261	        virtualDestroyNSSReference();
(gdb) bt
#0  0x00002b2cb576d62c in shutdown (calledFrom=nsNSSShutDownObject::calledFromList, this=0x2b2cc2f3d140) at /home/mchew/mozilla-central/security/manager/ssl/src/nsNSSShutDown.h:261
#1  nsNSSShutDownList::evaporateAllNSSResourcesHelper (table=<optimized out>, hdr=<optimized out>, number=<optimized out>, arg=<optimized out>)
    at /home/mchew/mozilla-central/security/manager/ssl/src/nsNSSShutDown.cpp:219
#2  0x00002b2cb5bc44c0 in PL_DHashTableEnumerate (table=0x2b2cc2f3aec8, etor=0x2b2cb576d5d4 <nsNSSShutDownList::evaporateAllNSSResourcesHelper(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0x0)
    at /home/mchew/mozilla-central/obj-x86_64-unknown-linux-gnu/xpcom/build/pldhash.cpp:717
#3  0x00002b2cb576dcdf in nsNSSShutDownList::evaporateAllNSSResources (this=0x2b2cc2f3aeb0) at /home/mchew/mozilla-central/security/manager/ssl/src/nsNSSShutDown.cpp:205
#4  0x00002b2cb573b72f in nsNSSComponent::ShutdownNSS (this=0x2b2cc2fc9400) at /home/mchew/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1867
#5  0x00002b2cb573b910 in nsNSSComponent::~nsNSSComponent (this=0x2b2cc2fc9400, __in_chrg=<optimized out>) at /home/mchew/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:404
#6  0x00002b2cb573ba26 in nsNSSComponent::~nsNSSComponent (this=0x2b2cc2fc9400, __in_chrg=<optimized out>) at /home/mchew/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:415
#7  0x00002b2cb5738389 in Release (this=0x2b2cc2fc9400) at /home/mchew/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1969
#8  nsNSSComponent::Release (this=0x2b2cc2fc9400) at /home/mchew/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1969
#9  0x00002b2cb5bff960 in operator= (rhs=0x0, this=0x2b2cc07cd318) at ../../dist/include/nsCOMPtr.h:947
#10 FreeFactoryEntries (aCID=..., aEntry=0x2b2cc07cd300, arg=<optimized out>) at /home/mchew/mozilla-central/xpcom/components/nsComponentManager.cpp:1058
#11 0x00002b2cb5bfd436 in nsBaseHashtable<nsIDHashKey, nsFactoryEntry*, nsFactoryEntry*>::s_EnumReadStub (table=<optimized out>, hdr=<optimized out>, number=<optimized out>, arg=<optimized out>)
    at ../../dist/include/nsBaseHashtable.h:400
#12 0x00002b2cb5bc44c0 in PL_DHashTableEnumerate (table=0x2b2cc076d3a0, 
    etor=0x2b2cb5bfd41e <nsBaseHashtable<nsIDHashKey, nsFactoryEntry*, nsFactoryEntry*>::s_EnumReadStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0x7fff203cfa10)
    at /home/mchew/mozilla-central/obj-x86_64-unknown-linux-gnu/xpcom/build/pldhash.cpp:717
#13 0x00002b2cb5bfec2e in nsBaseHashtable<nsIDHashKey, nsFactoryEntry*, nsFactoryEntry*>::EnumerateRead (this=0x2b2cc076d3a0, enumFunc=0x2b2cb5bff940 <FreeFactoryEntries(nsID const&, nsFactoryEntry*, void*)>, 
    userArg=0x0) at ../../dist/include/nsBaseHashtable.h:190
#14 0x00002b2cb5bfec91 in nsComponentManagerImpl::FreeServices (this=0x2b2cc076d330) at /home/mchew/mozilla-central/xpcom/components/nsComponentManager.cpp:1070
#15 0x00002b2cb5bc943b in mozilla::ShutdownXPCOM (servMgr=<optimized out>) at /home/mchew/mozilla-central/xpcom/build/nsXPComInit.cpp:613
#16 0x000000000040657e in main (argc=24, argv=0x7fff203cfde0, envp=0x7fff203d1086) at /home/mchew/mozilla-central/js/xpconnect/shell/xpcshell.cpp:1980
(gdb)

    
    

    
  
So DigestOutputStream doesn't own the digest context, it's owned by the caller since the original intention was to re-use the context in the cancel-resume case. Maybe this is confusing NSS Shutdown object, or does everything that owns an NSS object need to implement NSSShutDownObject?

If so, perhaps BackgroundFileSaver should implement NSSShutdownObject instead?

    
    

    
  
Not sure why it is crashing.

The main purposes of nsNSSShutDownObject are:

1. to Destroy/free/delete all your NSS objects (in this case, the digest context) in destructorSafeDestroyNSSReference()/virtualDestroyNSSReference(),
2. to ensure you never call any NSS function after NSS has shut down.
Flags: needinfo?(bsmith)

    
    

    
  
Is that necessary, then? The digest context is already scoped and will be deallocated when BackgroundFileSaver is destroyed.

    
    

    
  
I guess I should transfer ownership of the digest context to DigestOutputStream.

    
    

    
  
(In reply to Monica Chew from comment #50)
> Is that necessary, then? The digest context is already scoped and will be
> deallocated when BackgroundFileSaver is destroyed.

But, the BackgroundFileSaver may get destroyed *after* NSS has been shut down. And, in that case, you cannot call SEC_DestroyHashContext (or whatever it's called) in the destructor, because it is too late.

    
    

    
  
OK. How can BackgroundFileSaver and DigestOutputStream share the digest context? BackgroundFileSaver needs ownership of mDigestContext for operating on NotifySaveComplete, and DigestOutputStream needs to be able to check isAlreadyShutDown() etc. when writing to the stream.

There doesn't seem to be even a way to release ownership of a scoped ptr once it's been created.

    
    

    
  
(In reply to Monica Chew from comment #53)
> OK. How can BackgroundFileSaver and DigestOutputStream share the digest
> context? BackgroundFileSaver needs ownership of mDigestContext for operating
> on NotifySaveComplete, and DigestOutputStream needs to be able to check
> isAlreadyShutDown() etc. when writing to the stream.

Up to you. It makes the most sense to me, based on what I know, to do all the PK11_* stuff without DigestOutputStream and have DigestOutputStream implement nsNSSShutDownObject.

> There doesn't seem to be even a way to release ownership of a scoped ptr
> once it's been created.

ScopedX newOwner(oldOwner.forget());

    
    

    
  
I get we probably need two pieces:

1. An object that has the same lifetime of BackgroundFileSaver, and wraps the
   digest context and all the NSS digest calls. This is created lazily only if
   hashing is requested, but then it is destroyed only with BackgroundFileSaver.
2. An object that is created when the output stream changes, and forwards
   writes to a method on the first object.

Object (1) would be very similar to nsCryptoHash, inheriting from
nsNSSShutdownObject, with the difference that it would do PK11_ calls
instead of HASH_ calls:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#2512

The only thing we should be aware of is that, while all calls into object (1)
are normally from the worker thread, the service destruction notification
comes from the main thread, so we may need to add some additional mutex
protection around all the PK11_ calls (for example in
destructorSafeDestroyNSSReference), compared to nsICryptoHash.

Object (2) would look similar to the current stream wrapper.

    
    

    
  


  
I wasn't calling shutdown correctly. Addressed all comments, please have another look.

        Attachment #709264 -
        Attachment is obsolete: true

        Attachment #709280 -
        Attachment is obsolete: true

        Attachment #709485 -
        Flags: review?(bsmith)

    
    

    
  
Comment on attachment 709485 [details] [diff] [review]
Compute SHA256 in BackgroundFileSaver.

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

Just a few notes before Brian's final review. The first one is actually just a suggestion, mainly for code maintainability, you'd do a favor to people who'll work on the same code in the future if you do a last effort to address it now.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +170,5 @@
> +
> +  // Ensure Personal Security Manager is initialized. This is required for
> +  // PK11_CreateDigestContext to work.
> +  if (initializeNSS) {
> +    MOZ_ASSERT(NS_IsMainThread(), "Must initialize NSS on main thread");

I know there's technically no race condition with the worker thread, because of how GetWorkerThreadAttention works today (the only other place where it's called is the Finish method), but it took me some time to figure that out.

To make this more future-proof and easier to validate, I'd make the following changes:

- Initialize NSS _before_ setting mAssignedTarget.
- Add a comment to mAssignedTarget saying it is always set only after NSS has been initialized. The worker thread will be able to rely on this assumption.

and...

@@ +388,5 @@
>    // The target can only be null if it has never been assigned.  In this case,
>    // there is nothing to do since we never created any output file.
>    if (!target) {
>      return NS_OK;
>    }

...move the mDigestContext initialization after this check that exits when mAssignedTarget is null.

In fact, you could even move initalization all the way down before mDigestContext is first used.

> NS_WARNING("BackgroundFileSaver: created context");

I guess this was left here for debugging only, in the final patch it should be a log line instead since there's no error condition here.

@@ +478,5 @@
>    outputStream = NS_BufferOutputStream(outputStream, BUFFERED_OUTPUT_SIZE);
>    if (!outputStream) {
>      return NS_ERROR_FAILURE;
>    }
> +  if (mDigestContext && !isAlreadyShutDown()) {

This should be encapsulated within an nsNSSShutDownPreventionLock.

@@ +554,5 @@
>  
>      mComplete = true;
>    }
>  
> +  // Ensure we notify completion now that the operation failed.

Spurious comment? The operation isn't necessarily failed here...

@@ +562,5 @@
>    }
>  
> +  // Finish computing the hash
> +  if (!failed && mDigestContext && !isAlreadyShutDown()) {
> +    nsNSSShutDownPreventionLock lock;

isAlreadyShutDown() should always be called within nsNSSShutDownPreventionLock, to prevent a race condition.

    
    

    
  
Hi Paolo,

I'm happy to move the NSS initialization before SetTarget is called, but that would mean that we always have to initialize it whether or not hashing is enabled, because SetSha256Enabled can be called from any thread.

The other option is saying that Sha256Enabled must be set on the main thread, but having it also initialize NSS seems like an undesirable side effect, so I'd rather go with always initializing NSS.

Is that all right? I will address the other cleanups.

Thanks,
Monica

    
    

    
  
(In reply to Monica Chew from comment #59)
> I'm happy to move the NSS initialization before SetTarget is called, but
> that would mean that we always have to initialize it whether or not hashing
> is enabled, because SetSha256Enabled can be called from any thread.

I mean, we should still initialize NSS within SetTarget, but swap the order
of the calls inside the function (you'll need to get two separate locks, the
first just to read sha256Enabled, released immediately, and the second time
to set mAssignedTarget, after NSS initialization is completed).

Initialization will only be done if sha256Enabled is true, and we already
require to set that property before calling setTarget.

> The other option is saying that Sha256Enabled must be set on the main
> thread, but having it also initialize NSS seems like an undesirable side
> effect, so I'd rather go with always initializing NSS.

sha256Enabled, as well as all the other public methods of the interface,
should all be called from the main thread, in any case. This isn't mentioned
in the interface documentation, but feel free to add a comment since I realize
the IDL is where most people will be looking for that kind of information.

    
    

    
  
Brian's recommendations:
- Move NSS initialization to setSha256Enabled and force it to be called on the main thread
- Add MOZ_ASSERT(NS_IsMainThread) to everything that we expect to be called on the main thread
- Make GetSha256Hash throw NS_ERROR_NOT_AVAILABLE if either 1) sha256 has never been enabled or 2) the caller calls it before NotifySaveComplete
- Once these are done, remove locks for mSha256Enabled and mSha256Hash, since by contract they can't be read or written unsafely.

    
    

    
  


  
Addressed Paolo's cleanups, added MOZ_ASSERTs IsMainThread where appropriate and updated idl comments, moved NSS initialization to setSha256enabled, changed GetSha256Hash to throw if the hash is invalid and added a test case for that.

Please have another look.

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

        Attachment #709485 -
        Attachment is obsolete: true

        Attachment #709485 -
        Flags: review?(bsmith)

        Attachment #710872 -
        Flags: review?(bsmith)

    
    

    
  
Comment on attachment 710872 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

Looks good! I noticed just one last thing to fix functionally.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +472,5 @@
> +  if (sha256Enabled && !mDigestContext) {
> +    nsNSSShutDownPreventionLock lock;
> +    mDigestContext =
> +      PK11_CreateDigestContext(static_cast<SECOidTag>(SEC_OID_SHA256));
> +    NS_ENSURE_TRUE(mDigestContext, NS_ERROR_OUT_OF_MEMORY);

Inside this first nsNSSShutDownPreventionLock, you should also verify isAlreadyShutDown.

I'd more simply rewrite this part under a single lock as:

if (sha256Enabled) {
  nsNSSShutDownPreventionLock lock;
  NS_ENSURE_FALSE(isAlreadyShutDown(), NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
  if (!mDigestContext) {
    mDigestContext = ...
    NS_ENSURE_TRUE(mDigestContext, NS_ERROR_OUT_OF_MEMORY);
  }
  nsRefPtr<DigestOutputStream> digestOutputStream = ...
  outputStream = digestOutputStream;
}

::: netwerk/base/src/BackgroundFileSaver.h
@@ +165,5 @@
> +  /**
> +   * Whether or not to compute the hash. Must be set on the main thread but can
> +   * be read on any thread.
> +   */
> +  bool mSha256Enabled;

The comment should be updated with the new logic - mSha256enabled can be set on the main thread only before setTarget is called, then it should be never changed again and thus can be read on any thread without locking.

        Attachment #710872 -
        Flags: review+

    
    

    
  
Comment on attachment 710872 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

::: netwerk/test/unit/test_backgroundfilesaver.js
@@ +478,5 @@
> +  } catch (ex if ex.result == Cr.NS_ERROR_NOT_AVAILABLE) { }
> +  // Enable hashing, but don't feed any data to saver
> +  let destFile = getTempFile(TEST_FILE_NAME_1);
> +  saver.setTarget(destFile, false);
> +  saver.sha256Enabled = true;

Oh, I almost forgot, this order of the calls became a "Just Don't Do That" in the last iteration, so they should be swapped in the test also - otherwise we incur in the same startup race condition we've been trying to avoid.

@@ +483,5 @@
> +  saver.finish(Cr.NS_ERROR_FAILURE);
> +  try {
> +    let hash = saver.sha256Hash;
> +    throw "Shouldn't be able to get hash if save did not complete";
> +  } catch (ex if ex.result == Cr.NS_ERROR_NOT_AVAILABLE) { }

Please add a comment saying that, since you're not waiting on promiseSaverComplete, this getter can run either before or after onSaveComplete is actually called. The expected behavior is the same in both cases, since we're canceling the operation.

    
    

    
  
Thanks, Paolo.

(In reply to Paolo Amadini [:paolo] from comment #64)
> > +  if (sha256Enabled && !mDigestContext) {
> > +    nsNSSShutDownPreventionLock lock;
> > +    mDigestContext =
> > +      PK11_CreateDigestContext(static_cast<SECOidTag>(SEC_OID_SHA256));
> > +    NS_ENSURE_TRUE(mDigestContext, NS_ERROR_OUT_OF_MEMORY);
> 
> Inside this first nsNSSShutDownPreventionLock, you should also verify
> isAlreadyShutDown.
> 
> I'd more simply rewrite this part under a single lock as:
> 
> if (sha256Enabled) {
>   nsNSSShutDownPreventionLock lock;
>   NS_ENSURE_FALSE(isAlreadyShutDown(), NS_ERROR_ILLEGAL_DURING_SHUTDOWN);
<snip>

bsmith sez: you must always take the lock before doing any NSS ops. However, the lock can be acquired when NSS has already been shut down (it only prevents NSS from changing state within the block where it's acquired), so I don't think that the NS_ENSURE_FALSE(isAlreadyShutDown()) statement is correct.

Fixing your other cleanups.

    
    

    
  


  
Address Paolo's requests for more comments, fix race condition in test.

bsmith, do you have time to review this? If not, I need to come up with a plan.

        Attachment #710872 -
        Attachment is obsolete: true

        Attachment #710872 -
        Flags: review?(bsmith)

        Attachment #711993 -
        Flags: review?(bsmith)

    
    

    
  
(In reply to Monica Chew from comment #66)
> bsmith sez: you must always take the lock before doing any NSS ops. However,
> the lock can be acquired when NSS has already been shut down (it only
> prevents NSS from changing state within the block where it's acquired)

True!

> so I don't think that the NS_ENSURE_FALSE(isAlreadyShutDown()) statement is
> correct.

It expands to "if (isAlreadyShutDown()) return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;"
plus a warning, so it's correct, but the alternate implementation in your latest
patch is also correct.

Thank you a lot for working on this feature! It will be used in the new downloads
API also, and all this experience with NSS that we had together will be surely
useful to both of us. And adding more hash types when needed will be a breeze :-)

    
    

    
  
Paolo, thanks for all your help with this. It really makes a world of difference to have such a responsive reviewer. I look forward to bugging you later about the other parts of this bug :)

    
    

    
  
Comment on attachment 711993 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

Drive by review.

Looks good, but I think that the use of nsCstrings is incorrect. For sure the copy operation will truncate 1/16 on average (1/256*16) in the BackgroundFileSaver::GetSha256Hash func.

::: netwerk/base/public/nsIBackgroundFileSaver.idl
@@ +56,5 @@
> +   *
> +   * @remarks Reading this will throw NS_ERROR_NOT_AVAILABLE unless
> +   *          sha256enabled is true and onSaveComplete has been called.
> +   */
> +  readonly attribute ACString sha256Hash;

It was my impression that *Cstrings where NULL terminated. would this work(a fp with a null char) ? or would it be more correct to use hex or base64 encoding?

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +228,5 @@
> +  MutexAutoLock lock(mLock);
> +  if (mHash.EqualsLiteral("")) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +  return NS_CStringCopy(aHash, mHash);

CStrings are null terminated. This could be a problem

::: netwerk/base/src/BackgroundFileSaver.h
@@ +13,5 @@
>  #define BackgroundFileSaver_h__
>  
>  #include "mozilla/Mutex.h"
>  #include "nsCOMPtr.h"
> +#include "nsNSSShutDown.h"

you need this one here? I think only the cpp needs it.

@@ +159,5 @@
> +  /**
> +   * The SHA 256 hash in raw bytes of the downloaded file. This is written
> +   * by the worker thread but can be read on the main thread.
> +   */
> +  nsAutoCString mHash;

I would rename it mSHA256 or something to indicate the hash type. You might also want to change the type or representation so that this field will be filled correctly

    
    

    
  
Hi Camilo,

From https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide:

nsSubstring/nsCSubstring (also known as nsAString/nsACString): the abstract base class for all strings. It provides an API for assignment, individual character access, basic manipulation of characters in the string, and string comparison. This class corresponds to the XPIDL AString parameter type. nsAString is not necessarily null-terminated.

The C just means narrow, so I don't think that anything with an A (nsAString, nsACString) in it requires null termination.

NS_CStringCopy takes nsACString as params, perhaps it's a bad name, but I think the call is correct.

Because BackgroundFileSaver inherits nsNSSShutDown, its .h should include nsNSSShutDown so it's a complete class declaration, and anything using it doesn't need to necessarily know about nsNSSShutDown.

Renaming mHash to mSha256Hash.

Thanks,
Monica

    
    

    
  


  
Rename mHash to mSha256

        Attachment #711993 -
        Attachment is obsolete: true

        Attachment #711993 -
        Flags: review?(bsmith)

        Attachment #713602 -
        Flags: review?(cviecco)

    
    

    
  
Comment on attachment 713602 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

Great job! (and how fast you can make me change my mind).

        Attachment #713602 -
        Flags: review?(cviecco) → review+

    
    

    
  


  
Rebase, updated patch reviewers in patch description.
Try: https://tbpl.mozilla.org/?tree=Try&rev=55e9563048b1

I don't think that bsmith, who is an official NSS peer, has time for review, although he was very helpful giving in-person advice, and I took all of his implementation advice on how to do the streaming digest and other aspects of NSS handling. However, I don't think that any reasonable person reading through the review comments from Paolo, who wrote the module, and Camilo, who has worked on NSS, would say that this patch is insufficiently reviewed.

Therefore in the interests of not wasting anyone's time, as soon as try is green, I'm going to mark checkin-needed. If bug watchers agree, please give me no sign :)

        Attachment #713602 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 713642 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

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

Chillax. I'm not big on process either but there are reasons we require module peers to review code. In particular, because this code is going to be put in the modules we are responsible for, we have to make sure we can maintain it. And other reasons.

::: netwerk/base/public/nsIBackgroundFileSaver.idl
@@ +64,5 @@
> +   *
> +   * @remarks This must be set on the main thread before the first call to
> +   *          setTarget.
> +   */
> +  attribute bool sha256Enabled;

Given the restrictions on the use of this documented in the implementation, it should be just "void enableSHA256();" Then we don't have to worry about the unimportant getter or the unimportant case of someone setting "sha256Enabled = false";

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +105,5 @@
> +    return;
> +  }
> +  if (mDigestContext) {
> +    mozilla::psm::PK11_DestroyContext_true(mDigestContext.forget());
> +  }

mDigestContext = nullptr;

But, the bigger issue: Why is BackgroundFileSaver implementing nsNSSShutDownObject in the first place? DigestStream already implements the interface, and so if we just made DigestStream own the digest context, then we'd only need one implementation of nsNSSShutDownObject. That would make the threading/ownership model much clearer.

I suspect that the reason is that we need the digest context to live longer than an output stream. Is that so?

I am concerned that DigestOutputStream considers its digest context as always available unless NSS has been shut down. That means that the digest context must always be destroyed After the last possible write to the output stream. How is this enforced? Especially, mDigestContext is declared after the output stream, so mDigestContext will get destroyed first when the BackgroundFileSaver is destroyed. Also, something else could hold a reference to the output stream that lasts longer than the lifetime of BackgroundFileSaver, AFAICT; it isn't obvious whether that happens or not in this case.

IMO, this would be clearer if BackgroundFileSaver created the digest context, and then gave ownership of it to its output stream, and then took back ownership of it when it closed the output stream. Then there would only be a single object referencing the digest context at once and the concurrency and ownership would clearly be correct.

@@ +228,5 @@
> +  MutexAutoLock lock(mLock);
> +  if (mSha256.EqualsLiteral("")) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +  return NS_CStringCopy(aHash, mSha256);

aHash = mSha256;?

@@ +480,5 @@
> +      NS_ENSURE_TRUE(mDigestContext, NS_ERROR_OUT_OF_MEMORY);
> +    }
> +  }
> +
> +  if (mDigestContext) {

if mSha256Enabled is true then we MUST return an error code unless we wrap the output stream with the hash-computing output stream. We shouldn't just silently NOT compute the hash in this case.

@@ +485,5 @@
> +    // No need to acquire the NSS lock here, DigestOutputStream must acquire it
> +    // in any case before each asynchronous write.
> +    nsRefPtr<DigestOutputStream> digestOutputStream =
> +      new DigestOutputStream(outputStream, mDigestContext);
> +    outputStream = digestOutputStream;

outputStream = new DigestOutputStream(...)

@@ +574,5 @@
> +      if (NS_SUCCEEDED(rv)) {
> +        MutexAutoLock lock(mLock);
> +        NS_CStringCopy(mSha256,
> +                       nsDependentCSubstring(char_ptr_cast(d.get().data),
> +                                             d.get().len));

mSha256 = nsDependentCSubstring(...) or
mSha256.Assign(char_ptr_cast(d.get().data, d.get().len))?

::: netwerk/base/src/BackgroundFileSaver.h
@@ +164,5 @@
> +
> +  /**
> +   * Whether or not to compute the hash. Must be set on the main thread before
> +   * setTarget is called. It should only be set once, never changed again, and
> +   * thus can be read on any thread without locking.

This "set once, never changed again" can be enforced by changing the IDL in the way I described. Also, these requirements on callers should be in the IDL, not in the implementation.

::: security/manager/ssl/src/DigestStreams.cpp
@@ +1,1 @@
> +#include "DigestStreams.h"

Add the license + modelines comments to new files as decribed in the coding style guide.

@@ +8,5 @@
> +NS_IMPL_THREADSAFE_ISUPPORTS1(DigestOutputStream,
> +                              nsIOutputStream)
> +
> +DigestOutputStream::DigestOutputStream(nsIOutputStream* aStream,
> +                                       PK11Context* aContext) :

: mOutputStream(aStream)
  , mDigestContext(aContext)

like other code in these modules.

@@ +43,5 @@
> +  }
> +
> +  nsresult rv = MapSECStatus(PK11_DigestOp(mDigestContext,
> +                                           uint8_t_ptr_cast(aBuf), aCount));
> +  NS_ENSURE_SUCCESS(rv, rv);

blank line here.

@@ +53,5 @@
> +                              uint32_t aCount, uint32_t* retval)
> +{
> +  // Not supported. We could read the stream to a buf, call DigestOp on the
> +  // result, seek back and pass the stream on, but it's not worth it since our
> +  // application (NS_AsyncCopy) doesn't invoke this on the sink.

MOZ_NOT_REACHED(...);

@@ +61,5 @@
> +NS_IMETHODIMP
> +DigestOutputStream::WriteSegments(nsReadSegmentFun aReader,
> +                                  void *aClosure, uint32_t aCount,
> +                                  uint32_t *retval)
> +{

MOZ_NOT_REACHED(...);

        Attachment #713642 -
        Flags: review-

    
    

    
  
> Chillax. I'm not big on process either but there are reasons we require
> module peers to review code. In particular, because this code is going to be
> put in the modules we are responsible for, we have to make sure we can
> maintain it. And other reasons.

OK. In the future, it would be good to at know an estimate of when you have time for a review. Having a 2 week review cycle is not conducive to completing work in a quarter.

> ::: netwerk/base/public/nsIBackgroundFileSaver.idl
> @@ +64,5 @@
> > +   *
> > +   * @remarks This must be set on the main thread before the first call to
> > +   *          setTarget.
> > +   */
> > +  attribute bool sha256Enabled;
> 
> Given the restrictions on the use of this documented in the implementation,
> it should be just "void enableSHA256();" Then we don't have to worry about
> the unimportant getter or the unimportant case of someone setting
> "sha256Enabled = false";

I will do this.

> ::: netwerk/base/src/BackgroundFileSaver.cpp
> @@ +105,5 @@
> > +    return;
> > +  }
> > +  if (mDigestContext) {
> > +    mozilla::psm::PK11_DestroyContext_true(mDigestContext.forget());
> > +  }
> 
> mDigestContext = nullptr;
> 
> But, the bigger issue: Why is BackgroundFileSaver implementing
> nsNSSShutDownObject in the first place? DigestStream already implements the
> interface, and so if we just made DigestStream own the digest context, then
> we'd only need one implementation of nsNSSShutDownObject. That would make
> the threading/ownership model much clearer.
> 
> I suspect that the reason is that we need the digest context to live longer
> than an output stream. Is that so?

Yes.

> I am concerned that DigestOutputStream considers its digest context as
> always available unless NSS has been shut down. That means that the digest
> context must always be destroyed After the last possible write to the output
> stream. How is this enforced? 

Why is this true? What if another digester needs to write to it after, for example if it works out that cancelling and resuming requires another streaming digest?

> IMO, this would be clearer if BackgroundFileSaver created the digest
> context, and then gave ownership of it to its output stream, and then took
> back ownership of it when it closed the output stream. Then there would only
> be a single object referencing the digest context at once and the
> concurrency and ownership would clearly be correct.

I will do this via some incantation of scoped_ptr.forget() when the digest context is passed in to DigestOutputStream. However, BackgroundFileSaver still needs to implement NSSShutDown object since it's the creator of the digest context, so that part will not change. Do you disagree with this plan?
 
> @@ +228,5 @@
> > +  MutexAutoLock lock(mLock);
> > +  if (mSha256.EqualsLiteral("")) {
> > +    return NS_ERROR_NOT_AVAILABLE;
> > +  }
> > +  return NS_CStringCopy(aHash, mSha256);
> 
> aHash = mSha256;?

Do you have a reason for asking for this change, or is this some XPCOM idiom that's recommended somewhere? The reason I am using the utility function is that presumably invoking <string class>.assign() is lower overhead than the copy constructor, and using the utility is a good way not to have to think about it, assuming that the people maintaining the utility function have already thought about it.

> @@ +480,5 @@
> > +      NS_ENSURE_TRUE(mDigestContext, NS_ERROR_OUT_OF_MEMORY);
> > +    }
> > +  }
> > +
> > +  if (mDigestContext) {
> 
> if mSha256Enabled is true then we MUST return an error code unless we wrap
> the output stream with the hash-computing output stream. We shouldn't just
> silently NOT compute the hash in this case.

This code and code referred to by your other comments below existed in the patch as you reviewed it on 2/1 in comment 41, although you did not make those comments then. Is there any other change that you require, before I make another round of changes and ask for re-review?

Thanks,
Monica

    
    

    
  
After chatting with bsmith, I am not going to implement anything substantially different about the ownership of digest context. However, I will improve the comments to make sure readers know that BackgroundFileSaver always outlives DigestOutputStream.

mmc_: looking at the digest stream, i don't see a good place for digestoutputstream to transfer ownership back to background file saver
bsmith: I agree. Maybe my suggestion doesn't work.
bsmith: or, at least, maybe it isn't necessary to do it that way.
mmc_: would you be satisfied if i updated the comments to say that backgroundfilesaver always outlives digestoutputstream?
bsmith: Yes, the important point is that mDigestContext is created before the NS_AsyncCopy and is never destroyed before AsyncCallback is called.
bsmith: if you can explain how we ensure that, then I think it is good.
mmc_: ok, great

    
    

    
  
I have just glanced at this patch. biesi as netwerk owner asks the following of all checkins to that module:

* either a review by a peer or someone a peer designates. I'm a network peer and am happy to delegate the network review of this patch to paolo assuming bsmith is involved in the security side as paolo is the author of the code being changed.

* an sr for idl changes. I'm not an sr'r so I can't help you there - bz or biesi are your best bets.

I also think our review system needs to be made much more responsive but lets not loose faith in what we do have. You can always escalate through owners etc..

    
    

    
  
Comment on attachment 713642 [details] [diff] [review]
Compute sha256 hashes when downloading files in BackgroundFileSaver.

sr=biesi on the IDL change with the change from the attribute to the function.

(btw, you should replace the EqualsLiteral("") call with IsEmpty())

        Attachment #713642 -
        Flags: superreview+

    
    

    
  
Try is looking green with changes to fix biesi and bsmith's suggestions (modulo the memory management suggestion discussed in comment 77).

It's too late to check in now, so I'll wait until Tuesday to rebase and submit.

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

    
    

    
  


  
sr=biesi in comment 79
r=paolo in comment 64, as delegated by mcmanus in comment 78
r=cviecco in comment 73

        Attachment #713642 -
        Attachment is obsolete: true

    
    

    
  


  
Move DigestStream to BackgroundFileSaver as suggested by paolo in comment 26, to avoid touching anything in the NSS directory.

        Attachment #715616 -
        Attachment is obsolete: true

    
  
Status: NEW → ASSIGNED

    
    

    
  
Looks orange on Mac, unlike last Friday when Try was green (https://tbpl.mozilla.org/?tree=Try&rev=f21274b5b4d8). I'm going to ask for a rollback, try to repro locally, and try on Try one more time.

    
    

    
  
Try is still looking green, with zero changes to the submitted/reverted patch.

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

    
    

    
  
Second try also came in green: https://tbpl.mozilla.org/?tree=Try&rev=526944732440

I also couldn't repro locally when synced to mozilla-inbound, so I think it's reasonable to try again, with the same patch.
Keywords: checkin-needed

    
    

    
  
Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/49a583310573 (while you were typing comment 88) with a clobber and a touch of the /CLOBBER file - usually "it's fine on try, but not on inbound" (and especially the way inbound kept having xpcshell failures after the backout) means that for some reason it requires a clobber. It'd be extra nice if someone figured out why, and fix the bustedness if it's just busted dependencies.
Keywords: checkin-needed

    
    

    
  
Maybe because the IID of nsIBackgroundFileSaver needs to be updated?

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/49a583310573
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

    
  
Depends on: 845627

    
  
Depends on: 844392

    
  
Depends on: 853037

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: