Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2742193002:
Harden allocator for file-backed memory. (Closed)
|
Created:
3 years, 9 months ago by bcwhite Modified:
3 years, 9 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org, manzagop (departed) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHarden allocator for file-backed memory.
Data persisted to disk can end up with some oddities should a hard
shutdown of the machine (such as from a power failure) cause an
incomplete flush of the memory.
It's possible that the actual data gets flushed but the "free_ptr"
that indicates the end of used memory does not. The data is still
iterable but might try to go beyond what is believed to be the end.
The GetBlockData() checks against free_ptr have been removed, relying
on the fixed mem_size_ boundary and the required existence of the
header cookie to know if the block is valid.
A "Flush()" call has been added to allow for signalling when
essential changes have been made. It's an empty call except for
file-backed memory. A call to it is done after initialization to
ensure the file on disk appears initialized.
A "memory state" field has been added to the shared header to
indicate the overall state of the memory (initalized, deleted, etc.)
and accessor methods added. This allows the caller to ensure that
major changes to the memory segment get propogated. This is used
to indicate if it has been "deleted" because actually deleting the
file is prone to failure.
Unfortunately, this requires a change to the SharedMetada structure
that makes this code incompatible with previous versions. The first
version with this change will be unable to read persistent data
written by the previous version. It will find invalid data in the
header and be unable to find the written data blocks. This is
unfortunate but safe due to all the precautions against memory
corruption already in the PMA.
The PersistentHistogramAllocator has been updated to use this when
deleting a file and the FileMetricsProvider now reads it and
discards the data if it detects this.
Still to do is make use of these features in the Breadcrumbs
project.
BUG=546019, 620813
Review-Url: https://codereview.chromium.org/2742193002
Cr-Commit-Position: refs/heads/master@{#457501}
Committed: https://chromium.googlesource.com/chromium/src/+/42561dc4b39708c610a0149fb61b6f17c0e21f8e
Patch Set 1 #Patch Set 2 : fix some build problems #
Total comments: 22
Patch Set 3 : addressed review comments by asvitkine #Patch Set 4 : addressed review comments by asvitkine; added tests #
Total comments: 2
Patch Set 5 : addressed final review comments #
Messages
Total messages: 35 (26 generated)
The CQ bit was checked by bcwhite@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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 bcwhite@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: This issue passed the CQ dry run.
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
Description was changed from ========== Harden allocator for file-backed memory. Data persisted to disk can end up with some oddities should a hard shutdown of the machine (such as from a power failure) cause an incomplete flush of the memory. It's possible that the actual data gets flushed but the "free_ptr" that indicates the end of used memory does not. The data is still iterable but might try to go beyond what is believed to be the end. The GetBlockData() checks against free_ptr have been removed, relying on the fixed mem_size_ boundary and the required existence of the header cookie to know if the block is valid. A "Flush()" call has been added to allow for signalling when essential changes have been made. It's an empty call except for file-backed memory. A call to it is done after initialization to ensure the file on disk appears initialized. A "memory state" field has been added to the shared header to indicate the overall state of the memory (initalized, deleted, etc.) and accessor methods added. This allows the caller to ensure that major changes to the memory segment get propogated. This is used to indicate if it has been "deleted" because actually deleting the file is prone to failure. Unfortunately, this requires a change to the SharedMetada structure that makes this code incompatible with previous versions. The first version with this change will be unable to read persistent data written by the previous version. It will find invalid data in the header and be unable to find the written data blocks. This is unfortunate but safe due to all the precautions against memory corruption already in the PMA. The PersistentHistogramAllocator has been updated to use this when deleting a file and the FileMetricsProvider now reads it and discards the data if it detects this. Still to do is make use of these features in the Breadcrumbs project. BUG=546019,620813 ========== to ========== Harden allocator for file-backed memory. Data persisted to disk can end up with some oddities should a hard shutdown of the machine (such as from a power failure) cause an incomplete flush of the memory. It's possible that the actual data gets flushed but the "free_ptr" that indicates the end of used memory does not. The data is still iterable but might try to go beyond what is believed to be the end. The GetBlockData() checks against free_ptr have been removed, relying on the fixed mem_size_ boundary and the required existence of the header cookie to know if the block is valid. A "Flush()" call has been added to allow for signalling when essential changes have been made. It's an empty call except for file-backed memory. A call to it is done after initialization to ensure the file on disk appears initialized. A "memory state" field has been added to the shared header to indicate the overall state of the memory (initalized, deleted, etc.) and accessor methods added. This allows the caller to ensure that major changes to the memory segment get propogated. This is used to indicate if it has been "deleted" because actually deleting the file is prone to failure. Unfortunately, this requires a change to the SharedMetada structure that makes this code incompatible with previous versions. The first version with this change will be unable to read persistent data written by the previous version. It will find invalid data in the header and be unable to find the written data blocks. This is unfortunate but safe due to all the precautions against memory corruption already in the PMA. The PersistentHistogramAllocator has been updated to use this when deleting a file and the FileMetricsProvider now reads it and discards the data if it detects this. Still to do is make use of these features in the Breadcrumbs project. BUG=546019,620813 ==========
https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:106: // architectures. Can you have a static_assert about that? I know you have one checking for 64 explicitly, but this is more specific - i.e. that other assert doesn't indicate that it shouldn't be updated. So maybe have a static_assert specifically for the % 64? Also, can it be right after the structure rather than elsewhere in some function? https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:119: // State of the memory, plus some padding to keep alignment. What does "state of the memory" mean? Is it referencing the enum you have defined elsewhere? If so, mention this. Also why char and not uint8_t? https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:400: if (shared_meta()->size == 0 || shared_meta()->version != kGlobalVersion || So we would previously not use kGlobalVersion field? https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:842: if (ref != kReferenceQueue && block->cookie != kBlockCookieAllocated) Shouldn't you check ref != kReferenceQueue before adding ref to mem_base_ above? I know you're not derefing the pointer yet, but still - maybe separate the if into two. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:858: void PersistentMemoryAllocator::Flush(size_t length, bool sync) {} Nit: Maybe add a comment for why it makes sense to be a no-op. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1025: MS_INVALIDATE | (sync ? MS_SYNC : MS_ASYNC)); From man msync: "The MS_ASYNC flag is not permitted to be combined with other flags." https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:106: MEMORY_DELETED = 2, // The data should be considered deleted. Can you define what "considered deleted" means? https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:320: void Flush(bool sync) { Flush(used(), sync); } Don't overload a protected function with a public one. How about naming the internal one FlushInternal()? Also, this doesn't look like meeting the criteria for inline function from the style guide: "Inline functions Simple accessors should generally be the only inline functions. These should be named unix_hacker_style(). Virtual functions should never be declared this way. For more detail, consult the C++ Dos and Don'ts section on inlining." https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
The CQ bit was checked by bcwhite@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/2742193002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:106: // architectures. On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > Can you have a static_assert about that? > > I know you have one checking for 64 explicitly, but this is more specific - i.e. > that other assert doesn't indicate that it shouldn't be updated. So maybe have a > static_assert specifically for the % 64? Also, can it be right after the > structure rather than elsewhere in some function? It's effectively on line 324 which ensures the same size on all architectures. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:119: // State of the memory, plus some padding to keep alignment. On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > What does "state of the memory" mean? Is it referencing the enum you have > defined elsewhere? If so, mention this. Yes, MemoryState in the .h file. Done. > Also why char and not uint8_t? "char" is the only standard type with a guaranteed sizeof 1 on all architectures. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:400: if (shared_meta()->size == 0 || shared_meta()->version != kGlobalVersion || On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > So we would previously not use kGlobalVersion field? There has only been one version before this so not really a problem. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:842: if (ref != kReferenceQueue && block->cookie != kBlockCookieAllocated) On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > Shouldn't you check ref != kReferenceQueue before adding ref to mem_base_ above? > I know you're not derefing the pointer yet, but still - maybe separate the if > into two. kReferenceQueue is still an offset of mem_base_ so it's necessary to do the add before any dereference below. But I guess it's simpler to check for kReference Queue once at the top that have special conditions within. Done. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:858: void PersistentMemoryAllocator::Flush(size_t length, bool sync) {} On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > Nit: Maybe add a comment for why it makes sense to be a no-op. Done. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1025: MS_INVALIDATE | (sync ? MS_SYNC : MS_ASYNC)); On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > From man msync: > > "The MS_ASYNC flag is not permitted to be combined with other flags." Where did you read that? http://man7.org/linux/man-pages/man2/msync.2.html The flags argument should specify exactly one of MS_ASYNC and MS_SYNC, and may additionally include the MS_INVALIDATE bit. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:106: MEMORY_DELETED = 2, // The data should be considered deleted. On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > Can you define what "considered deleted" means? Done. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:320: void Flush(bool sync) { Flush(used(), sync); } On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > Don't overload a protected function with a public one. How about naming the > internal one FlushInternal()? > > Also, this doesn't look like meeting the criteria for inline function from the > style guide: > > "Inline functions > > Simple accessors should generally be the only inline functions. These should be > named unix_hacker_style(). Virtual functions should never be declared this way. > For more detail, consult the C++ Dos and Don'ts section on inlining." > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. At some point it may be useful to become FlushRange with an additional |start| parameter but right now that would just be wasted.
https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:119: // State of the memory, plus some padding to keep alignment. On 2017/03/15 19:21:47, bcwhite wrote: > > Also why char and not uint8_t? > > "char" is the only standard type with a guaranteed sizeof 1 on all > architectures. I think on all platforms Chrome supports uint8_t should also be sizeof 1. I think it's clearer to use that and be consistent in types between here and other places - and there are sizeof checks to ensure there's no problems with it. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1025: MS_INVALIDATE | (sync ? MS_SYNC : MS_ASYNC)); On 2017/03/15 19:21:48, bcwhite wrote: > On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > > From man msync: > > > > "The MS_ASYNC flag is not permitted to be combined with other flags." > > Where did you read that? > > http://man7.org/linux/man-pages/man2/msync.2.html > The flags argument should specify exactly one of MS_ASYNC and > MS_SYNC, and may additionally include the MS_INVALIDATE bit. I typed "man msync" on my Mac. So sounds like it's platform specific. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:320: void Flush(bool sync) { Flush(used(), sync); } On 2017/03/15 19:21:48, bcwhite wrote: > On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > > Don't overload a protected function with a public one. How about naming the > > internal one FlushInternal()? > > > > Also, this doesn't look like meeting the criteria for inline function from the > > style guide: > > > > "Inline functions > > > > Simple accessors should generally be the only inline functions. These should > be > > named unix_hacker_style(). Virtual functions should never be declared this > way. > > For more detail, consult the C++ Dos and Don'ts section on inlining." > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Done. At some point it may be useful to become FlushRange with an additional > |start| parameter but right now that would just be wasted. The second part of this comment was about not having it inline "Simple accessors should generally be the only inline functions". Move it out of line. It's not like it should be called often, so being out of line shouldn't be a problem.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@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/2742193002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:119: // State of the memory, plus some padding to keep alignment. On 2017/03/15 20:05:29, Alexei Svitkine (very slow) wrote: > On 2017/03/15 19:21:47, bcwhite wrote: > > > Also why char and not uint8_t? > > > > "char" is the only standard type with a guaranteed sizeof 1 on all > > architectures. > > I think on all platforms Chrome supports uint8_t should also be sizeof 1. I > think it's clearer to use that and be consistent in types between here and other > places - and there are sizeof checks to ensure there's no problems with it. Done. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:1025: MS_INVALIDATE | (sync ? MS_SYNC : MS_ASYNC)); On 2017/03/15 20:05:29, Alexei Svitkine (very slow) wrote: > On 2017/03/15 19:21:48, bcwhite wrote: > > On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > > > From man msync: > > > > > > "The MS_ASYNC flag is not permitted to be combined with other flags." > > > > Where did you read that? > > > > http://man7.org/linux/man-pages/man2/msync.2.html > > The flags argument should specify exactly one of MS_ASYNC and > > MS_SYNC, and may additionally include the MS_INVALIDATE bit. > > I typed "man msync" on my Mac. So sounds like it's platform specific. Done. https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.h (right): https://codereview.chromium.org/2742193002/diff/20001/base/metrics/persistent... base/metrics/persistent_memory_allocator.h:320: void Flush(bool sync) { Flush(used(), sync); } On 2017/03/15 20:05:29, Alexei Svitkine (very slow) wrote: > On 2017/03/15 19:21:48, bcwhite wrote: > > On 2017/03/15 15:47:20, Alexei Svitkine (very slow) wrote: > > > Don't overload a protected function with a public one. How about naming the > > > internal one FlushInternal()? > > > > > > Also, this doesn't look like meeting the criteria for inline function from > the > > > style guide: > > > > > > "Inline functions > > > > > > Simple accessors should generally be the only inline functions. These should > > be > > > named unix_hacker_style(). Virtual functions should never be declared this > > way. > > > For more detail, consult the C++ Dos and Don'ts section on inlining." > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > Done. At some point it may be useful to become FlushRange with an additional > > |start| parameter but right now that would just be wasted. > > The second part of this comment was about not having it inline "Simple accessors > should generally be the only inline functions". Move it out of line. It's not > like it should be called often, so being out of line shouldn't be a problem. Done. Originally it was just providing a default parameter which is, as I understand it, allowed by the style.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % comment https://codereview.chromium.org/2742193002/diff/60001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2742193002/diff/60001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:120: volatile std::atomic<char> memory_state; // MemoryState enum values. I meant for this to be std::atomic<uint8_t> in my comment.
The CQ bit was checked by bcwhite@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/2742193002/diff/60001/base/metrics/persistent... File base/metrics/persistent_memory_allocator.cc (right): https://codereview.chromium.org/2742193002/diff/60001/base/metrics/persistent... base/metrics/persistent_memory_allocator.cc:120: volatile std::atomic<char> memory_state; // MemoryState enum values. On 2017/03/16 16:12:53, Alexei Svitkine (very slow) wrote: > I meant for this to be std::atomic<uint8_t> in my comment. Ah, of course! I should have realized that. Done.
The CQ bit was unchecked by bcwhite@chromium.org
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2742193002/#ps80001 (title: "addressed final review comments")
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": 80001, "attempt_start_ts": 1489684860714250,
"parent_rev": "5f6c9c530b8cda7429c2ca86655439b5ea952b40", "commit_rev":
"96cbd09dbea1abb9850b191fca29c71cae607cf2"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489684860714250,
"parent_rev": "a46782374d15333661c7d3295f7c7413af92b4c0", "commit_rev":
"42561dc4b39708c610a0149fb61b6f17c0e21f8e"}
Message was sent while issue was closed.
Description was changed from ========== Harden allocator for file-backed memory. Data persisted to disk can end up with some oddities should a hard shutdown of the machine (such as from a power failure) cause an incomplete flush of the memory. It's possible that the actual data gets flushed but the "free_ptr" that indicates the end of used memory does not. The data is still iterable but might try to go beyond what is believed to be the end. The GetBlockData() checks against free_ptr have been removed, relying on the fixed mem_size_ boundary and the required existence of the header cookie to know if the block is valid. A "Flush()" call has been added to allow for signalling when essential changes have been made. It's an empty call except for file-backed memory. A call to it is done after initialization to ensure the file on disk appears initialized. A "memory state" field has been added to the shared header to indicate the overall state of the memory (initalized, deleted, etc.) and accessor methods added. This allows the caller to ensure that major changes to the memory segment get propogated. This is used to indicate if it has been "deleted" because actually deleting the file is prone to failure. Unfortunately, this requires a change to the SharedMetada structure that makes this code incompatible with previous versions. The first version with this change will be unable to read persistent data written by the previous version. It will find invalid data in the header and be unable to find the written data blocks. This is unfortunate but safe due to all the precautions against memory corruption already in the PMA. The PersistentHistogramAllocator has been updated to use this when deleting a file and the FileMetricsProvider now reads it and discards the data if it detects this. Still to do is make use of these features in the Breadcrumbs project. BUG=546019,620813 ========== to ========== Harden allocator for file-backed memory. Data persisted to disk can end up with some oddities should a hard shutdown of the machine (such as from a power failure) cause an incomplete flush of the memory. It's possible that the actual data gets flushed but the "free_ptr" that indicates the end of used memory does not. The data is still iterable but might try to go beyond what is believed to be the end. The GetBlockData() checks against free_ptr have been removed, relying on the fixed mem_size_ boundary and the required existence of the header cookie to know if the block is valid. A "Flush()" call has been added to allow for signalling when essential changes have been made. It's an empty call except for file-backed memory. A call to it is done after initialization to ensure the file on disk appears initialized. A "memory state" field has been added to the shared header to indicate the overall state of the memory (initalized, deleted, etc.) and accessor methods added. This allows the caller to ensure that major changes to the memory segment get propogated. This is used to indicate if it has been "deleted" because actually deleting the file is prone to failure. Unfortunately, this requires a change to the SharedMetada structure that makes this code incompatible with previous versions. The first version with this change will be unable to read persistent data written by the previous version. It will find invalid data in the header and be unable to find the written data blocks. This is unfortunate but safe due to all the precautions against memory corruption already in the PMA. The PersistentHistogramAllocator has been updated to use this when deleting a file and the FileMetricsProvider now reads it and discards the data if it detects this. Still to do is make use of these features in the Breadcrumbs project. BUG=546019,620813 Review-Url: https://codereview.chromium.org/2742193002 Cr-Commit-Position: refs/heads/master@{#457501} Committed: https://chromium.googlesource.com/chromium/src/+/42561dc4b39708c610a0149fb61b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/42561dc4b39708c610a0149fb61b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
