1205228 - Make PackagedAppVerifier work asynchronously

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Henry Chang [:hchang]]

I was hoping put the task to Bug 1178518 but it turns out way too complicated and easy to have bugs. So, I file a specific bug to deal with this and will add a couple of test cases to ensure all the edge cases are well handled.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug1205228.patch

Comment 2

[Henry Chang [:hchang]]

The patch changes the verification to work asynchronously by

1) Use a list to keep track of the resources that have been downloaded but not verified.
   (i.e. PackagedAppVerifier::mPendingResourceCacheInfoList)

2) When the verifier receives a downloaded resource, append to the list and do the asynchronous verification. When we got the async verification callback, the head of list is the the resource that has just been verified. Then we callback to PackagedAppService with related information.

3) Regarding the resource hash computation, since the verifier may receive the resource before the manifest verification is done, we need to store the computed hashes and look it up later. That's what PackagedAppResource::mResourceHashStore is for.

4) When running test case 'test_bad_package' in test_packaged_app_service.js, I observed that if the last part of the package is broken (in the test case no "Content-Location" is speicified), the PackagedAppDownloader will receive a OnStopRequest without a OnStartRequest. In the original design, it will immediately call |ClearCallbacks|. This leads some issue when we serving content asynchronously. The reason is at the time we call |ClearCallbacks|, some resources may still be in the queue, which means the resource we request may exist but |ClearCallbacks| will just respond the request with NS_ERROR_FILE_NOT_FOUND. To solve this, the simplest way is to append a "special" entry to PackagedAppVerifier::mPendingResourceCacheInfoList as a "broken last part". The "broken last part" is a ResouceCacheInfo with null URI. 

5) After failing to download a package due to whatever reason, we call FinalizeDownload and delete the downloader. We used to explicitly set PackageAppDownloader::mVerifier to null to break the downloader-verifier
cross reference. It works for sync-version verifier. The verifier will be just deleted after the statement "mVerifier = nullptr;" [1]. For async-version verifier, however, the verifier will be ref-counted by the the async callbacker (while calling NS_DispatchToMainThread with NS_NewRunnableMethodWithArgs) so we have to  remove the downloader from the verifier instead to delete the downloader ASAP.

[1] http://hg.mozilla.org/mozilla-central/file/e7d613b3bcfe/netwerk/protocol/http/PackagedAppService.cpp#l588

Comment 3

[Henry Chang [:hchang]]

Comment on attachment 8662261 [details] [diff] [review]
Bug1205228.patch

Hi Valentin,

Could you please have a review for this patch? You can refer to the bug description and comment 2 for the purpose of this bug and the explanation of the patch. I also add a bunch of test cases to ensure every thing wouldn't be broken after changing the working model to async.

Thanks!

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8662261 [details] [diff] [review]
Bug1205228.patch

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

This seems OK. I'm thinking we could probably bypass the PackagedAppVerifier completely if the package isn't signed, so we can avoid the performance overhead. I'll file another bug to analyze this possibility.

::: netwerk/protocol/http/PackagedAppVerifier.cpp
@@ +49,5 @@
>  }
>  
> +PackagedAppVerifier::~PackagedAppVerifier()
> +{
> +  while (auto i = mPendingResourceCacheInfoList.popFirst()) {

Assert main thread.

@@ +134,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  LOG(("Hash of %s is %s", mHashingResourceURI.get(), hash.get()));
> +
> +  // Store the computated hash associated with the resource URI.

MOZ_RELEASE_ASSERT(NS_IsMainThread())

@@ +299,5 @@
>        mPackageCacheEntry = nullptr; // the cache entry is no longer needed.
>      }
>    }
>  
> +  RefPtr<ResourceCacheInfo> info(mPendingResourceCacheInfoList.popFirst());

Assert main thread.

@@ +341,5 @@
>  
> +void
> +PackagedAppVerifier::SetHasBrokenLastPart(nsresult aStatusCode)
> +{
> +  // Append a record with null URI as a broken last part.

Wouldn't a boolean flag on the verifier work too? This doesn't look very clean.

@@ +346,5 @@
> +
> +  ResourceCacheInfo* info
> +    = new ResourceCacheInfo(nullptr, nullptr, aStatusCode, true);
> +
> +  mPendingResourceCacheInfoList.insertBack(info);

Assert main thread since mPendingResourceCacheInfoList isn't thread safe.

::: netwerk/protocol/http/PackagedAppVerifier.h
@@ +76,5 @@
> +    }
> +
> +    // A ResourceCacheInfo must have a URI. If mURI is null, this
> +    // resource is broken.
> +    bool IsBroken() const { return !mURI.get(); }

!mURI should be enough.

Comment 5

[Henry Chang [:hchang]]

Thanks for the review :)

(In reply to Valentin Gosu [:valentin] from comment #4)
> Comment on attachment 8662261 [details] [diff] [review]
> Bug1205228.patch
> 
> Review of attachment 8662261 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +341,5 @@
> >  
> > +void
> > +PackagedAppVerifier::SetHasBrokenLastPart(nsresult aStatusCode)
> > +{
> > +  // Append a record with null URI as a broken last part.
> 
> Wouldn't a boolean flag on the verifier work too? This doesn't look very
> clean.
> 

I had the same idea in the first place but it turned out not making the code cleaner. The reason is we have to identify the point where the ResourceCacheInfo list is empty and fire the equivalent "broken last part" event. Since we have some "early return" in VerifyResource/VerifyManifest, we might wind up adding more lines and doing some refactor.

Comment 6

[Henry Chang [:hchang]]

Attachment: Bug1205228_r2.patch

Comment 7

[Henry Chang [:hchang]]

Attachment: Bug1205228_r3.patch (fixed assertion failure in debug build)

Comment 8

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e6bda02e5f

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c944a70fbb

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e3c944a70fbb