Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12951014:
Fix bug in MemEntryImpl, which caused browser crash during clearing (Closed)
|
Created:
7 years, 9 months ago by Slava Chigrin Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionFix bug in MemEntryImpl, which caused browser crash during clearing
memory cache. It could appear if memory cache had sparse entries.
TEST=Run DiskCacheBackendTest.MemoryOnlyDoom* tests from net_unittests
binary.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194891
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/12951014/diff/1/net/disk_cache/backend_unitte... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12951014/diff/1/net/disk_cache/backend_unitte... net/disk_cache/backend_unittest.cc:1396: // third_part1, fourth_part1, third_part2, fourth_part2 The second time stamp should be somewhere in the middle, to make sure that deleting from time1 to time2 is not the same as deleting from time1. It would be interesting to see how that works in practice given the mix between parent and children in this part of the test. https://codereview.chromium.org/12951014/diff/1/net/disk_cache/backend_unitte... net/disk_cache/backend_unittest.cc:1415: TEST_F(DiskCacheBackendTest, MemoryOnlyDoomEntriesSinceSparse) { It would be nice to also add tests for the regular backend (just the simple one, not AppCache or the others). Copy/paste sounds good. https://codereview.chromium.org/12951014/diff/1/net/disk_cache/mem_backend_im... File net/disk_cache/mem_backend_impl.cc (right): https://codereview.chromium.org/12951014/diff/1/net/disk_cache/mem_backend_im... net/disk_cache/mem_backend_impl.cc:258: // InternalReadSparseData and InternalWriteSparseData implementations). nit: Remove "implementations"
https://codereview.chromium.org/12951014/diff/1/net/disk_cache/backend_unitte... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12951014/diff/1/net/disk_cache/backend_unitte... net/disk_cache/backend_unittest.cc:1396: // third_part1, fourth_part1, third_part2, fourth_part2 On 2013/03/21 22:01:57, rvargas wrote: > The second time stamp should be somewhere in the middle, to make sure that > deleting from time1 to time2 is not the same as deleting from time1. > > It would be interesting to see how that works in practice given the mix between > parent and children in this part of the test. Done. https://codereview.chromium.org/12951014/diff/1/net/disk_cache/backend_unitte... net/disk_cache/backend_unittest.cc:1415: TEST_F(DiskCacheBackendTest, MemoryOnlyDoomEntriesSinceSparse) { I have added tests for BackendImpl here. I noticed that GetEntryCount() for it counts also child entries, whereas MemBackendImpl - only parent. Is it expected behavior? I multiplied expected results by 3 in tests for BackendImpl to pass the tests. These new test fail sometimes. I investigated this issue - that is because of last used time of sparse entry updated when Close() operation completed on EntryImpl (since it destroys SparseControl, which calls SparseControl::CloseChild, which performs Write operation). Since Close() operation is asynchronous for disk backend entries, there are seems no way to guarantee that sparse entry will have last used time in particular time range, which we need for tests. I added FLAKY_ prefix for these tests, is this Ok? I can increase possibility that test will pass by adding more AddDelay() calls, but still we'll not have 100% guarantee that test will pass. I can not find any ways to fix this without adding Callback parameter to Close() operation, but seems it will cause huge refactoring. On 2013/03/21 22:01:57, rvargas wrote: > It would be nice to also add tests for the regular backend (just the simple one, > not AppCache or the others). Copy/paste sounds good. https://codereview.chromium.org/12951014/diff/1/net/disk_cache/mem_backend_im... File net/disk_cache/mem_backend_impl.cc (right): https://codereview.chromium.org/12951014/diff/1/net/disk_cache/mem_backend_im... net/disk_cache/mem_backend_impl.cc:258: // InternalReadSparseData and InternalWriteSparseData implementations). On 2013/03/21 22:01:57, rvargas wrote: > nit: Remove "implementations" Done.
https://codereview.chromium.org/12951014/diff/1/net/disk_cache/backend_unitte... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12951014/diff/1/net/disk_cache/backend_unitte... net/disk_cache/backend_unittest.cc:1415: TEST_F(DiskCacheBackendTest, MemoryOnlyDoomEntriesSinceSparse) { On 2013/03/22 13:45:02, vchigrin wrote: > I have added tests for BackendImpl here. I noticed that GetEntryCount() for it > counts also child entries, whereas MemBackendImpl - only parent. Is it expected > behavior? I multiplied expected results by 3 in tests for BackendImpl to pass > the tests. yes... the disk backend allows enumeration (and inspection) of sparse children so it reports the total number of entries. The memory backend doesn't enumerate sparse children so it doesn't count them either when reporting the number of entries. > > These new test fail sometimes. I investigated this issue - that is because of > last used time of sparse entry updated when Close() operation completed on > EntryImpl (since it destroys SparseControl, which calls > SparseControl::CloseChild, which performs Write operation). > > Since Close() operation is asynchronous for disk backend entries, there are > seems no way to guarantee that sparse entry will have last used time in > particular time range, which we need for tests. Take a look at DiskCacheBackendTest::BackendDoomBetween(). We use FlushQueueForTest() whenever we have to make sure that Close() does something before moving on with the test. I should have mentioned that before. > > I added FLAKY_ prefix for these tests, is this Ok? I can increase possibility > that test will pass by adding more AddDelay() calls, but still we'll not have > 100% guarantee that test will pass. > > I can not find any ways to fix this without adding Callback parameter to Close() > operation, but seems it will cause huge refactoring. > > On 2013/03/21 22:01:57, rvargas wrote: > > It would be nice to also add tests for the regular backend (just the simple > one, > > not AppCache or the others). Copy/paste sounds good. >
Thank you so much, FlushQueueForTest resolves problem with Close(). New patch set is attached to this review.
Hi! We sent Yandex CLA signed on the previous week. Could you please confirm you've received it?
Looks basically good. Just very minor nits. I still don't see the CLA. I'm waiting for an internal answer to see if the signature is stuck somewhere. https://codereview.chromium.org/12951014/diff/12001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12951014/diff/12001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:1416: AddDelay(); nit: we shouldn't need an extra delay here. https://codereview.chromium.org/12951014/diff/12001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:1431: // NOTE: BackendImpl counts child entries in it GetEntryCount(), while type: it -> its https://codereview.chromium.org/12951014/diff/12001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:1493: TEST_F(DiskCacheBackendTest, MemoryOnlyDoomBetween) { nit: don't move this test. (avoid not needed changes... or I'm missing the reason for moving it :) ) https://codereview.chromium.org/12951014/diff/12001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:1515: end = base::Time::Now(); ah, I see... OK then for the extra delay.
Thank you for review, I have fixed these style problems. New patch is attached to the review. https://codereview.chromium.org/12951014/diff/12001/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/12951014/diff/12001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:1431: // NOTE: BackendImpl counts child entries in it GetEntryCount(), while On 2013/03/28 00:31:59, rvargas wrote: > type: it -> its Done. https://codereview.chromium.org/12951014/diff/12001/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:1493: TEST_F(DiskCacheBackendTest, MemoryOnlyDoomBetween) { On 2013/03/28 00:31:59, rvargas wrote: > nit: don't move this test. (avoid not needed changes... or I'm missing the > reason for moving it :) ) Done.
The Yandex contributor's license agreement has been received and is on file. However, it has an explicit list of authorized contributors, so please do not add *@yandex-team.ru to the AUTHORS file. Instead add your name and email address explicitly (like folks from @intel.com). Slava Chigrin is on the authorized contributors list.
if we want to add *@yandex-team.ru to the AUTHORS file, should we send an update of this list with *@yandex-team.ru in it? On 2013/04/01 19:24:00, mal wrote: > The Yandex contributor's license agreement has been received and is on file. > > However, it has an explicit list of authorized contributors, so please do not > add mailto:*@yandex-team.ru to the AUTHORS file. > > Instead add your name and email address explicitly (like folks from @intel.com). > > Slava Chigrin is on the authorized contributors list.
We've updated the list. Now all Yandex folks are authorized to contribute.
On 2013/04/02 11:20:00, iroubin wrote: > We've updated the list. Now all Yandex folks are authorized to contribute. LGTM (for AUTHORS) I've checked with Google's open source programs office, and a new CLA has been received from Yandex that allows <*@yandex-team.ru>
LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vchigrin@yandex-team.ru/12951014/19001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vchigrin@yandex-team.ru/12951014/40002
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vchigrin@yandex-team.ru/12951014/40002
Failed to apply patch for AUTHORS: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 233. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS =================================================================== --- AUTHORS (revision 193900) +++ AUTHORS (working copy) @@ -233,3 +233,4 @@ Jun Jiang <jun.a.jiang@intel.com> Bobby Powers <bobbypowers@gmail.com> Patrick Riordan <patrickriordan177@gmail.com> +Yandex LLC <*@yandex-team.ru>
On 2013/04/18 07:37:46, I haz the power (commit-bot) wrote: > Failed to apply patch for AUTHORS: > While running patch -p0 --forward --force --no-backup-if-mismatch; > patching file AUTHORS > Hunk #1 FAILED at 233. > 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej > > Patch: AUTHORS > Index: AUTHORS > =================================================================== > --- AUTHORS (revision 193900) > +++ AUTHORS (working copy) > @@ -233,3 +233,4 @@ > Jun Jiang <mailto:jun.a.jiang@intel.com> > Bobby Powers <mailto:bobbypowers@gmail.com> > Patrick Riordan <mailto:patrickriordan177@gmail.com> > +Yandex LLC <mailto:*@yandex-team.ru> It looks like the patch isn't applying to the AUTHORS file because it's a bit out of date. Sync up to trunk, upload another patch, and it should go through the CQ without a problem.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vchigrin@yandex-team.ru/12951014/47001
Failed to apply patch for AUTHORS: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 234. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS =================================================================== --- AUTHORS (revision 194825) +++ AUTHORS (working copy) @@ -234,3 +234,4 @@ Bobby Powers <bobbypowers@gmail.com> Patrick Riordan <patrickriordan177@gmail.com> Kenneth Rohde Christiansen <kenneth.r.christiansen@intel.com> +Yandex LLC <*@yandex-team.ru>
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vchigrin@yandex-team.ru/12951014/59001
Message was sent while issue was closed.
Change committed as 194891 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
