Revert of [tracing] Simplify logic of MemoryDumpManager

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}

[Primiano Tucci (use gerrit), 2015-12-21 15:23:31]

Created Revert of [tracing] Simplify logic of MemoryDumpManager

[commit-bot: I haz the power, 2015-12-21 15:23:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1540283003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1540283003/1

[commit-bot: I haz the power, 2015-12-21 15:24:40]

Description was changed from

==========
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/build...


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
==========

to

==========
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/build...


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
==========

[commit-bot: I haz the power, 2015-12-21 15:24:41]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-12-21 15:25:27]

Description was changed from

==========
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/build...


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
==========

to

==========
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/build...

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}
==========

[commit-bot: I haz the power, 2015-12-21 15:25:28]

Patchset 1 (id:??) landed as
https://crrev.com/6a7365c4c5ad3e3083ce3d9a269c3e1d8fdb35bc
Cr-Commit-Position: refs/heads/master@{#366386}

[Primiano Tucci (use gerrit), 2015-12-22 10:32:37]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1546583002/ by primiano@chromium.org.

The reason for reverting is: 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.
.