833063 - update verification should be split into a separate process (at least for B2G)

Status: RESOLVED DUPLICATE of bug 1373267

(Toolkit :: Application Update, defect)

Description

[Dave Hylands [:dhylands]]

With the improvements from bug 802423, applying an update no longer interferes with the UI.

However, after downloading the update.mar file, the UI still freezes while the updater code is performing the verification of the update.mar file.

The verification phase only takes about 10 seconds on a 50 Mb update.mar file, so this isn't anywhere near as bad as applying the update.

I took a quick look at the code, and _verifyDownload uses an nsIFileInputStream to read the file and nsICrytoHash.updateFromStream to compute the hash.

It appears that both of these occur on the MainThread.

Moving to the IOThread would be a start, but having the verification in a completely separate process would be best. This way the CPU priority and IO Priority can both be lowered.

Comment 1

[Dave Hylands [:dhylands]]

STR:

Create an update:
./build.sh gecko-update-full

This should create about a 50 Mb update file.

- Flash the phone with a fresh build
- Apply the update using:
tools/update-tools/test-update.py ${GECKO_OBJDIR}/dist/b2g-update/b2g-gecko-update.mar

I like to use the fast-update.patch (attached) to speed up the update process. You may still need to go into settings and do a Check Now to kick things off.

By watching logcat, you should see messages about the _verify being called (right after the 50 Mb .mar is downloaded) and the UI will be totally unresponsive for about 10 seconds while the update is verified.

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Patch to reduce timings for testing the update cycle (DO NOT DELIVER)

I also like to modify gaia/apps/system/js/updater_manager.js and change

  NOTIFICATION_BUFFERING_TIMEOUT: 30 * 1000,

to be

  NOTIFICATION_BUFFERING_TIMEOUT: 3 * 1000,

This reduces the time betweem when AUS reports an update is available and gaia reports that its available.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

This patch implements: |nsIDOMDOMRequest updateFromStreamRequest(in nsIInputStream aStream, in unsigned long aLen)|.
This new method execs |updateFromStream| in a separated thread.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

<bsmith> dhylands: what is the purpose of this sha1 hash in the first place?
<dhylands> Its to verify the integrity of a downloaded update
<bsmith> i.e. what is the reason we are verifying this hash
<bsmith> dhylands: so, if I can get the signature verification done, we may not even need to check that hash at all?
<dhylands> bsmith: Does the signature verification include the integrity of the contents?
<bsmith> Yes, the entire contents of the MAR are signed
<dhylands> bsmith: So yeah then it would seem like you wouldn't need both
<dhylands> bsmith: Although we'd also like the signature verification not to block MainThread
<bsmith> I am tryting to get the signature verification finished (late) today
<bsmith> So, I will revisit that patch if there are more problems with it
<bsmith> I agree that we should avoid having the signature verification block the main thread.
<bsmith> On the other hand, not sure about the priority. It seems like a "nice to have" more than a must-have.
<bsmith> The other thing to keep in mind is that the hash verification step mostly only helps if things are done over HTTPS
<bsmith> But, the HTTPS protocol itself uses HMAC to verify the contents of everything sent and received.
<dhylands> bsmith: Yeah 833063 is in the nice-to-have category

So, I will look at the nsICryptoHash changes once I know for sure whether the signature verification will be done.

One thing we must not do is add a new dependency on anything in DOM to PSM. We're trying to remove all the DOM dependencies now. But, let's not worry about that until we know whether we can just remove this hash check.

Comment 5

[Dave Hylands [:dhylands]]

That's a definite improvement (in UI experience while verifying).

However, I tried running it in a debug build and I see both of your assertions firing:

*** AUS:SVC Downloader:_verifyDownload called
AUS:SVC Downloader:_verifyDownload called
*** AUS:SVC Downloader:_verifyDownload downloaded size == expected size.
AUS:SVC Downloader:_verifyDownload downloaded size == expected size.
[Parent 488] ###!!! ASSERTION: Wrong thread!: 'NS_IsMainThread()', file /home/work/B2G-unagi/mozilla-inbound/security/manager/ssl/src/nsNSSComponent.cpp, line 277
[Parent 488] ###!!! ASSERTION: Wrong thread!: '!NS_IsMainThread()', file /home/work/B2G-unagi/mozilla-inbound/security/manager/ssl/src/nsNSSComponent.cpp, line 291
*** AUS:SVC Downloader:_verifyDownload hashes match.
AUS:SVC Downloader:_verifyDownload hashes match.

I think we need to split the patch into 2 and get the nsUpdateService.js portion reviewed by rstong, and the other parts reviewed by bsmith.

Without touching the UI, it takes the same 10 seconds to verify the hash.
If I scroll up and down continuously while it does the verification, it takes 20 seconds (which is fine - it just demonstrates that the scrolling is fairly CPU intensive).

If we decide to continue with this (see comment 4) then we'll also need to do a try run and make sure that the xpcshell tests all pass (there are a bunch of update xpcshell tests)

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch for the nsUpdateService.js

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch for the async hash updateFromStream

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to Andrea Marchesini (:baku) from comment #6)
> Created attachment 709647 [details] [diff] [review]
> patch for the nsUpdateService.js
I should be able to get to this on Tuesday 2/4

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 709647 [details] [diff] [review]
patch for the nsUpdateService.js

Looks good but I went ahead and threw this at try (with the other patch) for the app update tests just in case.
https://tbpl.mozilla.org/?tree=Try&rev=589bdee85048

r=me as long as the tests pass

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 709647 [details] [diff] [review]
patch for the nsUpdateService.js

Looks like this leaks :(

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch for the async hash updateFromStream (b)

https://tbpl.mozilla.org/?tree=Try&rev=b7a87ceae7bc waiting for green

Comment 12

[Andrea Marchesini [:baku]]

wait... wrong set of patches. I'm going to push it to try again.

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 711240 [details] [diff] [review]
patch for the async hash updateFromStream (b)

Looks like a bsmith review. After you get try results go ahead and flag me for review on the existing nsUpdateService.js patch if this is fixed by the other patch or a new nsUpdateService.js patch.

Comment 14

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=b38680e0e76f
green on try.

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 709647 [details] [diff] [review]
patch for the nsUpdateService.js

same patch. The list was in the other one.

Comment 16

[Andrea Marchesini [:baku]]

any news here?

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 711240 [details] [diff] [review]
patch for the async hash updateFromStream (b)

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO me if you want a response) from comment #4)
> One thing we must not do is add a new dependency on anything in DOM to PSM.
> We're trying to remove all the DOM dependencies now. But, let's not worry
> about that until we know whether we can just remove this hash check.

There should not be any new dependencies on DOM in netwerk/ or security/manager. r- for that. I will review a new patch much more expediently. Sorry for letting this sit.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

clearing needinfo.

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

clearing needinfo for real

Comment 20

[Andrea Marchesini [:baku]]

> There should not be any new dependencies on DOM in netwerk/ or
> security/manager. r- for that. I will review a new patch much more
> expediently. Sorry for letting this sit.

Can you suggest a different approach? Do we have an alternative to DOMRequest that is not part of DOM?

Comment 21

[Andrea Marchesini [:baku]]

Do we still need this?

Comment 22

[Dave Hylands [:dhylands]]

yeah. Right now, because update verification runs in the b2g process, it also runs with the same priority.

And since it's largely I/O bound, it winds up making the entire b2g app feel quite sluggish while its performing the verification.

When we launch the "apply update" process, its launched as a separate process, and we give it much lower priority, etc and this makes that part of the update not make the UI feel sluggish.

Comment 23

[Andrea Marchesini [:baku]]

I'm not actually working on this.

Comment 24

[Robert Strong (they/them - no direct email)]

App update no longer has code for verifying mar hashes