Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1463463003:
AppCache: fix a browser crashing bug that can happen during updates. (Closed)
|
Created:
5 years, 1 month ago by michaeln Modified:
5 years, 1 month ago Reviewers:
gzobqq CC:
chromium-reviews, michaeln, darin-cc_chromium.org, jam, Will Harris Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAppCache: fix a browser crashing bug that can happen during updates.
BUG=558589
Committed: https://crrev.com/e5c298b780737c53fa9aae44d6fef522931d88b0
Cr-Commit-Position: refs/heads/master@{#360967}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Messages
Total messages: 13 (4 generated)
Description was changed from ========== appcache BUG=558589 ========== to ========== appcache BUG=558589 ==========
michaeln@chromium.org changed reviewers: + gzobqq@gmail.com
not tested yet, but here's what i have in mind as a fix
Thanks, I tested that this fix stops the PoC. https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_update_job.cc (right): https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_update_job.cc:431: return; Do we care about lack of host notification? https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_update_job.cc:887: --master_entries_completed_; Another possible fix would be to do this erase() and -- outside the if. This way any failed fetches won't be counted in master_entries_completed_, so failed urls can't be counted multiple times. On the positive side, any re-fetches will also get notified. But I'm not quite sure this change doesn't have side effects. https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_update_job.cc:1696: void AppCacheUpdateJob::DeleteSoon() { In any case, perhaps double check here or in the destructor that master_entry_fetches_ and pending_url_fetches_ are empty. It's difficult to verify the correctness of the check at MaybeCompleteUpdate().
thnx for looking and verifying it fixes the crash https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_update_job.cc (right): https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_update_job.cc:431: return; On 2015/11/20 06:10:54, gzobqq wrote: > Do we care about lack of host notification? we care more about the crash for this to happen, the master resource load must succeed a moment earlier and then fail to load a moment later. this seems like a small corner case. i think trading minor correctness errors (visible events) in that case for robustness is a reasonable trade. https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_update_job.cc:887: --master_entries_completed_; On 2015/11/20 06:10:54, gzobqq wrote: > Another possible fix would be to do this erase() and -- outside the if. This way > any failed fetches won't be counted in master_entries_completed_, so failed urls > can't be counted multiple times. On the positive side, any re-fetches will also > get notified. > > *But I'm not quite sure this change doesn't have side effects.* This is very tricky code, the concern for breaking something else is what led me to add the new collection instead of trying to alter the existing book keeping. The simple addition is a lot easier to reason about. https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_update_job.cc:1696: void AppCacheUpdateJob::DeleteSoon() { On 2015/11/20 06:10:54, gzobqq wrote: > In any case, perhaps double check here or in the destructor that > master_entry_fetches_ and pending_url_fetches_ are empty. It's difficult to > verify the correctness of the check at MaybeCompleteUpdate(). I've converted a few DCHECKs in the dtor to CHECKs given the severity of getting it wrong.
On 2015/11/20 20:38:56, michaeln wrote: > thnx for looking and verifying it fixes the crash > > https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... > File content/browser/appcache/appcache_update_job.cc (right): > > https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... > content/browser/appcache/appcache_update_job.cc:431: return; > On 2015/11/20 06:10:54, gzobqq wrote: > > Do we care about lack of host notification? > > we care more about the crash > > for this to happen, the master resource load must succeed a moment earlier and > then fail to load a moment later. this seems like a small corner case. i think > trading minor correctness errors (visible events) in that case for robustness is > a reasonable trade. > > https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... > content/browser/appcache/appcache_update_job.cc:887: > --master_entries_completed_; > On 2015/11/20 06:10:54, gzobqq wrote: > > Another possible fix would be to do this erase() and -- outside the if. This > way > > any failed fetches won't be counted in master_entries_completed_, so failed > urls > > can't be counted multiple times. On the positive side, any re-fetches will > also > > get notified. > > > > *But I'm not quite sure this change doesn't have side effects.* > > This is very tricky code, the concern for breaking something else is what led me > to add the new collection instead of trying to alter the existing book keeping. > The simple addition is a lot easier to reason about. I agree. > > https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... > content/browser/appcache/appcache_update_job.cc:1696: void > AppCacheUpdateJob::DeleteSoon() { > On 2015/11/20 06:10:54, gzobqq wrote: > > In any case, perhaps double check here or in the destructor that > > master_entry_fetches_ and pending_url_fetches_ are empty. It's difficult to > > verify the correctness of the check at MaybeCompleteUpdate(). > > I've converted a few DCHECKs in the dtor to CHECKs given the severity of getting > it wrong. LGTM
https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... File content/browser/appcache/appcache_update_job.cc (right): https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... content/browser/appcache/appcache_update_job.cc:431: return; On 2015/11/20 20:38:55, michaeln wrote: > On 2015/11/20 06:10:54, gzobqq wrote: > > Do we care about lack of host notification? > > we care more about the crash > > for this to happen, the master resource load must succeed a moment earlier and > then fail to load a moment later. this seems like a small corner case. i think > trading minor correctness errors (visible events) in that case for robustness is > a reasonable trade. If we were to care... looks like the sequence of expected events should be: CHECKING, DOWNLOADING, ERROR and AssociateNoCache(GURL())?
Description was changed from ========== appcache BUG=558589 ========== to ========== AppCache: fix a browser crashing bug that can happen during updates. BUG=558589 ==========
The CQ bit was checked by michaeln@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1463463003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1463463003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e5c298b780737c53fa9aae44d6fef522931d88b0 Cr-Commit-Position: refs/heads/master@{#360967}
Message was sent while issue was closed.
On 2015/11/20 21:19:15, michaeln wrote: > https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... > File content/browser/appcache/appcache_update_job.cc (right): > > https://codereview.chromium.org/1463463003/diff/1/content/browser/appcache/ap... > content/browser/appcache/appcache_update_job.cc:431: return; > On 2015/11/20 20:38:55, michaeln wrote: > > On 2015/11/20 06:10:54, gzobqq wrote: > > > Do we care about lack of host notification? > > > > we care more about the crash > > > > for this to happen, the master resource load must succeed a moment earlier and > > then fail to load a moment later. this seems like a small corner case. i think > > trading minor correctness errors (visible events) in that case for robustness > is > > a reasonable trade. > > If we were to care... looks like the sequence of expected events should be: > CHECKING, DOWNLOADING, ERROR and AssociateNoCache(GURL())? If I'm reading this correctly, it would be: CHECKING DOWNLOADING if group_->update_status() == DOWNLOADING AssociateIncompleteCache(inprogress_cache_.get(), manifest_url_) if inprogress_cache_ AssociateNoCache(GURL()) if inprogress_cache_ ERROR The error notification has the difficulty that AppCacheErrorDetails takes the (possibly redirected?) request->url() and response code. These come from the first failed request and are not known later. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
