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

RESOLVED DUPLICATE of bug 1373267

Status

()

Toolkit
Application Update
RESOLVED DUPLICATE of bug 1373267
5 years ago
7 months ago

People

(Reporter: dhylands, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
Created attachment 707687 [details] [diff] [review]
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.
Assignee: nobody → amarchesini
Created attachment 708562 [details] [diff] [review]
patch

This patch implements: |nsIDOMDOMRequest updateFromStreamRequest(in nsIInputStream aStream, in unsigned long aLen)|.
This new method execs |updateFromStream| in a separated thread.
Attachment #708562 - Flags: review?(dhylands)
<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.
Flags: needinfo?(bsmith)
(Reporter)

Comment 5

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #708562 - Flags: review?(dhylands)
Created attachment 709647 [details] [diff] [review]
patch for the nsUpdateService.js
Attachment #708562 - Attachment is obsolete: true
Attachment #709647 - Flags: review?(amberstong)
Created attachment 709648 [details] [diff] [review]
patch for the async hash updateFromStream
Attachment #709648 - Flags: review?(bsmith)
Attachment #709647 - Flags: review?(amberstong) → review?(robert.bugzilla)
(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 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
Attachment #709647 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 709647 [details] [diff] [review]
patch for the nsUpdateService.js

Looks like this leaks :(
Attachment #709647 - Flags: review+ → review-
Created attachment 711240 [details] [diff] [review]
patch for the async hash updateFromStream (b)

https://tbpl.mozilla.org/?tree=Try&rev=b7a87ceae7bc waiting for green
Attachment #711240 - Flags: review?(robert.bugzilla)
wait... wrong set of patches. I'm going to push it to try again.
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.
Attachment #711240 - Flags: review?(robert.bugzilla)
Attachment #711240 - Flags: review?(bsmith)
Attachment #709648 - Attachment is obsolete: true
Attachment #709648 - Flags: review?(bsmith)
Comment on attachment 709647 [details] [diff] [review]
patch for the nsUpdateService.js

same patch. The list was in the other one.
Attachment #709647 - Flags: review- → review?(robert.bugzilla)
Attachment #709647 - Flags: review?(robert.bugzilla) → review+
any news here?
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.
Attachment #711240 - Flags: review?(brian) → review-
clearing needinfo for real
Flags: needinfo?(brian)
> 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?
Do we still need this?
Flags: needinfo?(dhylands)
(Reporter)

Comment 22

3 years ago
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.
Flags: needinfo?(dhylands)
I'm not actually working on this.
Assignee: amarchesini → nobody
App update no longer has code for verifying mar hashes
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1373267
You need to log in before you can comment on or make changes to this bug.