Closed
Bug 833063
Opened 11 years ago
Closed 7 years ago
update verification should be split into a separate process (at least for B2G)
Categories
(Toolkit :: Application Update, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 1373267
People
(Reporter: dhylands, Unassigned)
References
Details
Attachments
(3 files, 2 obsolete files)
2.17 KB,
patch
|
Details | Diff | Splinter Review | |
11.93 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
6.77 KB,
patch
|
briansmith
:
review-
|
Details | Diff | Splinter Review |
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•11 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•11 years ago
|
||
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.
Updated•11 years ago
|
Assignee: nobody → amarchesini
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
<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•11 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•11 years ago
|
Attachment #708562 -
Flags: review?(dhylands)
Comment 6•11 years ago
|
||
Attachment #708562 -
Attachment is obsolete: true
Attachment #709647 -
Flags: review?(amberstong)
Comment 7•11 years ago
|
||
Attachment #709648 -
Flags: review?(bsmith)
Updated•11 years ago
|
Attachment #709647 -
Flags: review?(amberstong) → review?(robert.bugzilla)
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
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 10•11 years ago
|
||
Comment on attachment 709647 [details] [diff] [review] patch for the nsUpdateService.js Looks like this leaks :(
Attachment #709647 -
Flags: review+ → review-
Comment 11•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b7a87ceae7bc waiting for green
Attachment #711240 -
Flags: review?(robert.bugzilla)
Comment 12•11 years ago
|
||
wait... wrong set of patches. I'm going to push it to try again.
Comment 13•11 years ago
|
||
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)
Comment 14•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b38680e0e76f green on try.
Updated•11 years ago
|
Attachment #711240 -
Flags: review?(bsmith)
Updated•11 years ago
|
Attachment #709648 -
Attachment is obsolete: true
Attachment #709648 -
Flags: review?(bsmith)
Comment 15•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #709647 -
Flags: review?(robert.bugzilla) → review+
Comment 16•11 years ago
|
||
any news here?
Comment 17•11 years ago
|
||
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-
Comment 18•11 years ago
|
||
clearing needinfo.
Comment 20•11 years ago
|
||
> 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?
Reporter | ||
Comment 22•10 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)
Comment 24•7 years ago
|
||
App update no longer has code for verifying mar hashes
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•