Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2970133002:
DoomPartialEntry should not attempt to doom an already doomed entry. (Closed)
|
Created:
3 years, 5 months ago by shivanisha Modified:
3 years, 5 months ago Reviewers:
jkarlin CC:
cbentzel+watch_chromium.org, chromium-reviews, gavinp+disk_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDoomPartialEntry should not attempt to doom an already doomed entry.
I was not able to reproduce the crash reported but here is a speculative fix.
This is a scenario where a partial transaction needs to create a new entry and doom
the existing one since it cannot validate itself against the new entry, due to,
say, there are no strong validators.
The crash requires a specific race with another transaction for the same entry
with LOAD_BYPASS_CACHE set due to which the second transaction straight away goes
and dooms the entry (DoDoomEntry*) while the first one had opened the entry but
not yet reached DoomPartialEntry.
This race is much more likely after the parallel validation CL
(https://codereview.chromium.org/2721933002) because add to entry is always an
async operation now.
This CL adds a check that the entry is not already doomed before calling
cache_->DoomEntry in DoomPartialEntry and in one other calling location in
DoSuccessfulSendRequest as it may happen there as well.
BUG=736993
Review-Url: https://codereview.chromium.org/2970133002
Cr-Commit-Position: refs/heads/master@{#486869}
Committed: https://chromium.googlesource.com/chromium/src/+/2b6e7ee275a56e425ce1396a7288a92756872e1d
Patch Set 1 : Initial patch #Patch Set 2 : Test and test framework changes added. #
Total comments: 12
Patch Set 3 : Added tests and changes to test framework. #Patch Set 4 : Rebased with refs/heads/master@{#485330} #
Total comments: 5
Patch Set 5 : Feedback addressed #Patch Set 6 : GetDiskEntryRef added and doomed_entries_ removed #
Total comments: 4
Patch Set 7 : Feedback addressed #
Total comments: 4
Patch Set 8 : Feedback addressed #
Messages
Total messages: 71 (54 generated)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== DoomPartialEntry should not attempt to doom an already doomed entry. BUG=736993 ========== to ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. Before parallel validation, the assumption was that since this is the only active transaction associated with the entry, the entry has to be non-doomed at this point. After parallel validation, its possible that there is a writer and a headers transaction both associated with this entry and the writer transaction may have doomed this entry for some reason. BUG=736993 ==========
Description was changed from ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. Before parallel validation, the assumption was that since this is the only active transaction associated with the entry, the entry has to be non-doomed at this point. After parallel validation, its possible that there is a writer and a headers transaction both associated with this entry and the writer transaction may have doomed this entry for some reason. BUG=736993 ========== to ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. Before parallel validation, the assumption was that since this is the only active transaction associated with the entry, the entry has to be non-doomed at this point. After parallel validation, its possible that there is a writer and a headers transaction both associated with this entry and the writer transaction may have doomed this entry for some reason. BUG=736993 ==========
Description was changed from ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. Before parallel validation, the assumption was that since this is the only active transaction associated with the entry, the entry has to be non-doomed at this point. After parallel validation, its possible that there is a writer and a headers transaction both associated with this entry and the writer transaction may have doomed this entry for some reason. BUG=736993 ========== to ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. Before parallel validation, the assumption was that since this is the only active transaction associated with the entry, the entry has to be non-doomed at this point. After parallel validation, its possible that there is a writer and a headers transaction both associated with this entry and the writer transaction may have doomed this entry for some reason. BUG=736993 ==========
Description was changed from ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. Before parallel validation, the assumption was that since this is the only active transaction associated with the entry, the entry has to be non-doomed at this point. After parallel validation, its possible that there is a writer and a headers transaction both associated with this entry and the writer transaction may have doomed this entry for some reason. BUG=736993 ========== to ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. Before parallel validation, the assumption was that since this is the only active transaction associated with the entry, the entry has to be non-doomed at this point. After parallel validation, its possible that there is a writer and a headers transaction both associated with this entry and the writer transaction may have doomed this entry for some reason. This CL adds a check that the entry is not already doomed before calling cache_->DoomEntry in DoomPartialEntry and in one other calling location in DoSuccessfulSendRequest as it may happen there as well. BUG=736993 ==========
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
shivanisha@chromium.org changed reviewers: + jkarlin@chromium.org
Josh, PTAL, Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. Before parallel validation, the assumption was that since this is the only active transaction associated with the entry, the entry has to be non-doomed at this point. After parallel validation, its possible that there is a writer and a headers transaction both associated with this entry and the writer transaction may have doomed this entry for some reason. This CL adds a check that the entry is not already doomed before calling cache_->DoomEntry in DoomPartialEntry and in one other calling location in DoSuccessfulSendRequest as it may happen there as well. BUG=736993 ========== to ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. The crash requires a specific race with another transaction for the same entry with LOAD_BYPASS_CACHE set due to which the second transaction straight away goes and dooms the entry (DoDoomEntry*) while the first one had opened the entry but not yet reached DoomPartialEntry. This CL adds a check that the entry is not already doomed before calling cache_->DoomEntry in DoomPartialEntry and in one other calling location in DoSuccessfulSendRequest as it may happen there as well. BUG=736993 ==========
Description was changed from ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. The crash requires a specific race with another transaction for the same entry with LOAD_BYPASS_CACHE set due to which the second transaction straight away goes and dooms the entry (DoDoomEntry*) while the first one had opened the entry but not yet reached DoomPartialEntry. This CL adds a check that the entry is not already doomed before calling cache_->DoomEntry in DoomPartialEntry and in one other calling location in DoSuccessfulSendRequest as it may happen there as well. BUG=736993 ========== to ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. The crash requires a specific race with another transaction for the same entry with LOAD_BYPASS_CACHE set due to which the second transaction straight away goes and dooms the entry (DoDoomEntry*) while the first one had opened the entry but not yet reached DoomPartialEntry. This race is much more likely after the parallel validation CL (https://codereview.chromium.org/2721933002) because add to entry is always an async operation now. This CL adds a check that the entry is not already doomed before calling cache_->DoomEntry in DoomPartialEntry and in one other calling location in DoSuccessfulSendRequest as it may happen there as well. BUG=736993 ==========
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! The latest patch has a test added and test framework additions to pause and restart a cache operation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.cc:138: } This seems overly complex for our use case. Why not just let the consumer set a bool, "e.g., SetDeferBeforeNextRead(true);"? No need for a callback. https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cache.h File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.h:32: typedef base::Callback<void(bool* defer)> BeforeCacheOperationCallback; Using instead of typedef https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.h:32: typedef base::Callback<void(bool* defer)> BeforeCacheOperationCallback; Instead of passing back a bool* defer (we always set it to true, so what's the point?) why not have it pass back a continuation callback that the consumer can run when it wants the Read to continue. That way MockHttpCache doesn't need to keep track of doomed_entries_ and doesn't need the resume functions. WDYT? https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.h:82: void SetBeforeCacheOperationCallback( Name is too general, specifically this is for Reads. SetBeforeReadCallback? https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.h:87: int ResumeCacheOperation(); BeforeCacheOperationCallback could be passed a callback to this method.
https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.cc:138: } On 2017/07/07 19:12:39, jkarlin_slow wrote: > This seems overly complex for our use case. Why not just let the consumer set a > bool, "e.g., SetDeferBeforeNextRead(true);"? No need for a callback. Note that this comment came before I added the suggestion for passing a callback to the consumer.
https://codereview.chromium.org/2970133002/diff/80001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2970133002/diff/80001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:1483: // assertion hit. (crbug.com/736993) After F2F we discussed that this test doesn't actually reproduce the assertion from the bug. There are two related bug behaviors and we should test both. Scenario 1: Dooming the wrong entry 1. Transaction one is about to read headers 2. Transaction two dooms and creates a new entry 3. Transaction one dooms (the HC looks up and deletes the new entry!) Scenario 2: Nothing to doom 1. Transaction one is about to read headers 2. Transaction two dooms and hasn't created its new entry yet 3. Transaction one dooms, the HC doesn't know what to do because it can't find the entry and DCHECKS We should test both of these.
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Patchset #3 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! https://codereview.chromium.org/2970133002/diff/80001/net/http/http_cache_uni... File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/2970133002/diff/80001/net/http/http_cache_uni... net/http/http_cache_unittest.cc:1483: // assertion hit. (crbug.com/736993) On 2017/07/07 at 20:03:15, jkarlin_slow wrote: > After F2F we discussed that this test doesn't actually reproduce the assertion from the bug. There are two related bug behaviors and we should test both. > > Scenario 1: Dooming the wrong entry > > 1. Transaction one is about to read headers > 2. Transaction two dooms and creates a new entry > 3. Transaction one dooms (the HC looks up and deletes the new entry!) > > > Scenario 2: Nothing to doom > > 1. Transaction one is about to read headers > 2. Transaction two dooms and hasn't created its new entry yet > 3. Transaction one dooms, the HC doesn't know what to do because it can't find the entry and DCHECKS > > We should test both of these. Added 2 tests. Without the fix, the first dooms the wrong entry and second crashes with the same stack trace as reported. https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.cc:138: } On 2017/07/07 at 19:14:12, jkarlin_slow wrote: > On 2017/07/07 19:12:39, jkarlin_slow wrote: > > This seems overly complex for our use case. Why not just let the consumer set a > > bool, "e.g., SetDeferBeforeNextRead(true);"? No need for a callback. > > Note that this comment came before I added the suggestion for passing a callback to the consumer. Simplified by using an enum DeferOp. https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cache.h File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.h:32: typedef base::Callback<void(bool* defer)> BeforeCacheOperationCallback; On 2017/07/07 at 19:12:39, jkarlin_slow wrote: > Instead of passing back a bool* defer (we always set it to true, so what's the point?) why not have it pass back a continuation callback that the consumer can run when it wants the Read to continue. That way MockHttpCache doesn't need to keep track of doomed_entries_ and doesn't need the resume functions. WDYT? MockHttpCache needs to keep track of doomed entries because MockDiskEntry is the layer whose operation we are deferring so on resume, we should be able to retrieve it and invoke Resume on it. Keeping track of doomed entries also help in making IsDiskEntryDoomed return a better answer. https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.h:82: void SetBeforeCacheOperationCallback( On 2017/07/07 at 19:12:39, jkarlin_slow wrote: > Name is too general, specifically this is for Reads. SetBeforeReadCallback? Removed this for simplicity and using a DeferOp enum instead. https://codereview.chromium.org/2970133002/diff/80001/net/http/mock_http_cach... net/http/mock_http_cache.h:87: int ResumeCacheOperation(); On 2017/07/07 at 19:12:39, jkarlin_slow wrote: > BeforeCacheOperationCallback could be passed a callback to this method. The latest changes probably make this not applicable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Patchset #4 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... net/http/mock_http_cache.cc:425: // It's possible that the entry was doomed directly by invoking There is a lot of new code here to track doomed entries so that they can later be resumed by the test. This is why I suggested that when setting the defer, the test passes a callback. The entry can then run the callback and pass the test a closure to run when the test wants the entry to resume. The closure would have a ref to the entry and run the resume function. That way the MockDiskCache doesn't need to keep track of doomed entries since the test calls the entry directly. https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... net/http/mock_http_cache.h:24: enum DeferOp { I don't think we want this enum in the net:: namespace. Putting it in the class makes more sense to me.
https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... net/http/mock_http_cache.cc:425: // It's possible that the entry was doomed directly by invoking On 2017/07/12 at 16:15:54, jkarlin wrote: > There is a lot of new code here to track doomed entries so that they can later be resumed by the test. > > This is why I suggested that when setting the defer, the test passes a callback. The entry can then run the callback and pass the test a closure to run when the test wants the entry to resume. The closure would have a ref to the entry and run the resume function. > > That way the MockDiskCache doesn't need to keep track of doomed entries since the test calls the entry directly. Keeping track of doomed_entries seems kind of good to have. The passing of entry to the test seems to require more work in the test for every defer-resume and less consistent in terms of layering since it passes an entry reference back to the test. WDYT?
PTAL, Thanks! https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... net/http/mock_http_cache.cc:425: // It's possible that the entry was doomed directly by invoking On 2017/07/12 at 17:56:31, shivanisha wrote: > On 2017/07/12 at 16:15:54, jkarlin wrote: > > There is a lot of new code here to track doomed entries so that they can later be resumed by the test. > > > > This is why I suggested that when setting the defer, the test passes a callback. The entry can then run the callback and pass the test a closure to run when the test wants the entry to resume. The closure would have a ref to the entry and run the resume function. > > > > That way the MockDiskCache doesn't need to keep track of doomed entries since the test calls the entry directly. > > Keeping track of doomed_entries seems kind of good to have. The passing of entry to the test seems to require more work in the test for every defer-resume and less consistent in terms of layering since it passes an entry reference back to the test. WDYT? Let me know if you prefer to have the closure approach or the doomed_entries_ approach is fine. https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/180001/net/http/mock_http_cac... net/http/mock_http_cache.h:24: enum DeferOp { On 2017/07/12 at 16:15:55, jkarlin wrote: > I don't think we want this enum in the net:: namespace. Putting it in the class makes more sense to me. Moved it to MockDiskEntry
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, Thanks! As discussed f2f, removed doomed_entry_ map and the associated functions. Added a GetDiskEntryRef() in MockDiskCache.
https://codereview.chromium.org/2970133002/diff/220001/net/http/mock_http_cac... File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/220001/net/http/mock_http_cac... net/http/mock_http_cache.h:190: int ResumeCacheOperation(); It's unclear when you should get the entry and call resume on that and when you should call ResumeCacheOperation. Perhaps we should rename this to ResumeReadOperation? And then have a comment in SetDefer on how to resume the various operations. https://codereview.chromium.org/2970133002/diff/220001/net/http/mock_http_cac... net/http/mock_http_cache.h:195: MockDiskEntry* GetDiskEntryRef(const std::string& key); scoped_refptr<MockDiskEntry> GetDiskEntryRef(const std::string& key). Then you don't need to worry about Close() and AddRef.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2970133002/diff/220001/net/http/mock_http_cac... File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/220001/net/http/mock_http_cac... net/http/mock_http_cache.h:190: int ResumeCacheOperation(); On 2017/07/13 at 17:27:24, jkarlin wrote: > It's unclear when you should get the entry and call resume on that and when you should call ResumeCacheOperation. > > Perhaps we should rename this to ResumeReadOperation? And then have a comment in SetDefer on how to resume the various operations. Renaming this ResumeDiskEntryOperation to let it be extendable to other operations like Write in the future.
https://codereview.chromium.org/2970133002/diff/220001/net/http/mock_http_cac... File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/220001/net/http/mock_http_cac... net/http/mock_http_cache.h:195: MockDiskEntry* GetDiskEntryRef(const std::string& key); On 2017/07/13 at 17:27:25, jkarlin wrote: > scoped_refptr<MockDiskEntry> GetDiskEntryRef(const std::string& key). > > Then you don't need to worry about Close() and AddRef. done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with a couple of comments https://codereview.chromium.org/2970133002/diff/240001/net/http/mock_http_cac... File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/2970133002/diff/240001/net/http/mock_http_cac... net/http/mock_http_cache.cc:142: int MockDiskEntry::ResumeDiskEntryOperation() { Why does this return anything? https://codereview.chromium.org/2970133002/diff/240001/net/http/mock_http_cac... File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/240001/net/http/mock_http_cac... net/http/mock_http_cache.h:185: // Defers invoking the callback for the given operation. Please add a comment here on when you should call ResumeCacheOperation vs ResumeDiskEntryOperation
The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Josh! https://codereview.chromium.org/2970133002/diff/240001/net/http/mock_http_cac... File net/http/mock_http_cache.cc (right): https://codereview.chromium.org/2970133002/diff/240001/net/http/mock_http_cac... net/http/mock_http_cache.cc:142: int MockDiskEntry::ResumeDiskEntryOperation() { On 2017/07/14 at 16:45:42, jkarlin wrote: > Why does this return anything? Removed the return from both resume functions. https://codereview.chromium.org/2970133002/diff/240001/net/http/mock_http_cac... File net/http/mock_http_cache.h (right): https://codereview.chromium.org/2970133002/diff/240001/net/http/mock_http_cac... net/http/mock_http_cache.h:185: // Defers invoking the callback for the given operation. On 2017/07/14 at 16:45:42, jkarlin wrote: > Please add a comment here on when you should call ResumeCacheOperation vs ResumeDiskEntryOperation Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2970133002/#ps260001 (title: "Feedback addressed")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1500063997767040,
"parent_rev": "fcc9f6614289d1e710fb49bd4e116fbdc08cb966", "commit_rev":
"a211190f3118850f276a29f9dab242b4b1caabb2"}
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1500063997767040,
"parent_rev": "6c976758dc0eca9aee1ea291ec959aedd2a8cb6b", "commit_rev":
"2b6e7ee275a56e425ce1396a7288a92756872e1d"}
Message was sent while issue was closed.
Description was changed from ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. The crash requires a specific race with another transaction for the same entry with LOAD_BYPASS_CACHE set due to which the second transaction straight away goes and dooms the entry (DoDoomEntry*) while the first one had opened the entry but not yet reached DoomPartialEntry. This race is much more likely after the parallel validation CL (https://codereview.chromium.org/2721933002) because add to entry is always an async operation now. This CL adds a check that the entry is not already doomed before calling cache_->DoomEntry in DoomPartialEntry and in one other calling location in DoSuccessfulSendRequest as it may happen there as well. BUG=736993 ========== to ========== DoomPartialEntry should not attempt to doom an already doomed entry. I was not able to reproduce the crash reported but here is a speculative fix. This is a scenario where a partial transaction needs to create a new entry and doom the existing one since it cannot validate itself against the new entry, due to, say, there are no strong validators. The crash requires a specific race with another transaction for the same entry with LOAD_BYPASS_CACHE set due to which the second transaction straight away goes and dooms the entry (DoDoomEntry*) while the first one had opened the entry but not yet reached DoomPartialEntry. This race is much more likely after the parallel validation CL (https://codereview.chromium.org/2721933002) because add to entry is always an async operation now. This CL adds a check that the entry is not already doomed before calling cache_->DoomEntry in DoomPartialEntry and in one other calling location in DoSuccessfulSendRequest as it may happen there as well. BUG=736993 Review-Url: https://codereview.chromium.org/2970133002 Cr-Commit-Position: refs/heads/master@{#486869} Committed: https://chromium.googlesource.com/chromium/src/+/2b6e7ee275a56e425ce1396a7288... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:260001) as https://chromium.googlesource.com/chromium/src/+/2b6e7ee275a56e425ce1396a7288... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
