tree c05875056a74fb9e4d8c3746e508f0e7e725aa3a
parent 42ebda43f1848f0d765d3fef9a4ca34411f2cfc4
author primiano <primiano@chromium.org> 1450785460 +0900
committer Qijiang Fan <fqj@google.com> 1591290461 +0900

Reland of [tracing] Simplify logic of MemoryDumpManager

Reason for revert:
Original CL: crrev.com/1536533004
Revert: crrev.com/1540283003
Reason for reland:
  The CL was reverted because caused a LayoutTest failure
  (pending-version-change-stuck-works-with-terminate.html).
  The failure was due to a race condition in an unrelated part of the
  codebase (see crbug.com/571432). The original CL managed to just
  unveil reliably the race. The race condition has been fixed in
  crrev.com/1544663002. I verified that the LayoutTest doesn't crash
  locally anymore.

Original issue's description:
> Revert of [tracing] Simplify logic of MemoryDumpManager (patchset #9 id:160001 of https://codereview.chromium.org/1536533004/ )
>
> Reason for revert:
> Caused crash in pending-version-change-stuck-works-with-terminate.html layout test.
>
> See:
> https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/builds/9506/steps/webkit_tests/logs/stdio
>
> Original issue's description:
> > [tracing] Simplify logic of MemoryDumpManager
> >
> > This CL simplifies the unregistration logic of MemoryDumpManager.
> > This is to make the overall code more readable and simplify upcoming
> > changes.
> > Prior to this CL, the MemoryDumpManager was keeping only one list to
> > keep track of the registered dump providers. This caused the
> > unregistration logic to be tricky because:
> >  - We couldn't remove the MDPInfo straight away, as that might
> >    cause invalidation of the |next_dump_provider| iterator.
> >  - We had to flag the MDPInfo as unregistered and postpone the deletion
> >    on the next dump.
> > This has a major drawback: if we keep registering and unregistering
> > dump providers when no tracing is happening at all, the dump_providers_
> > list grows without bounds. This is bad.
> >
> > This CL changes the logic as follows:
> >  - MDPInfo becomes refcounted. At any time it can be referenced by:
> >    - The MDM's |dump_providers_| list.
> >    - The |ProcessMemoryDumpAsyncState.pending_dump_providers| list.
> >  - Upon each dump, the dump provider list is snapshotted in the
> >    |ProcessMemoryDumpAsyncState.pending_dump_providers|
> >  - Upon unregistration of a dump provider we just remove the MDPInfo
> >    from the MDM's |dump_providers_|. If a dump is ongoing, the
> >    ProcessMemoryDumpAsyncState will keep it refcounted and delete it
> >    during the dump.
> >
> > This CL does not add or change any behavior of the MemoryDumpManager,
> > with the exception of:
> >  - Fixing a corner case when dumping with no dump providers registered
> >    (See changes in the unittest).
> >  - Making the fail-safe logic more strict: if a dumper fails once, it
> >    will stay disabled forever.
> >
> > BUG=461788
> >
> > Committed: https://crrev.com/9734733909e7cb41ef5c153f3c2d0927e8209133
> > Cr-Commit-Position: refs/heads/master@{#366374}
>
> TBR=ruuda@google.com,ssid@chromium.org
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=461788
>
> Committed: https://crrev.com/6a7365c4c5ad3e3083ce3d9a269c3e1d8fdb35bc
> Cr-Commit-Position: refs/heads/master@{#366386}

TBR=ruuda@google.com,ssid@chromium.org
BUG=461788

Review URL: https://codereview.chromium.org/1546583002

Cr-Commit-Position: refs/heads/master@{#366588}


CrOS-Libchrome-Original-Commit: 6dfc15bc20d12fdcd401011b00a521c4e557386e