tracing v2: Introduce handles for safe usage of ProtoZeroMessage

tracing v2: Introduce handles for safe usage of ProtoZeroMessage

This is to decouple the lifetime of a proto message from the underlying
chunk storage. It gives the following two guarantees:
- The underlying message is finalized (if still alive) if the handle
  goes out of scope. Concretely this guarantees that all events are
  committed to the buffer, without having to wait for the next event
  in the thread.
- In Debug / DCHECK_IS_ON builds, the handle becomes null once the
  underlying message is finalized. This is to enforce the append-only
  API and catch mis-uses bugs early (e.g., when adding two repeated
  fields, the addition of the second one will force the finalization
  of the first and invalidation of its handle)

BUG=608719
TEST=ProtoZeroMessageTest.MessageHandle

Committed: https://crrev.com/4f022543fb102a9c074308cb70e85d0a4d830cd1
Cr-Commit-Position: refs/heads/master@{#406827}

[Primiano Tucci (use gerrit), 2016-07-20 15:23:52]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 15:24:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 15:35:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 15:35:39]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Primiano Tucci (use gerrit), 2016-07-20 15:56:41]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 15:57:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Primiano Tucci (use gerrit), 2016-07-20 16:18:48]

primiano@chromium.org changed reviewers:
+ alph@chromium.org, oysteine@chromium.org

[Primiano Tucci (use gerrit), 2016-07-20 16:18:48]

This one is pretty boring, but the next one is more exciting
(https://codereview.chromium.org/2164013002)

[commit-bot: I haz the power, 2016-07-20 16:53:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 16:53:17]

Dry run: This issue passed the CQ dry run.

[alph, 2016-07-20 18:35:00]

lgtm

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message_handle.cc (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.cc:53: // useles null-check.
typo: useless

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message_handle.h (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.h:22: //   when when adding
two repeated messages, the addition of the 2nd one forces
double when

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.h:27: // Use the templated
version below. This base class is just here to avoid
no need to go into such explanation as this is the common pattern. protected
constructor would explain that.

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message_unittest.cc (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_unittest.cc:318: // This move should
cause |child_msg_1| to be finalized, while |child_msg_2|
s/while/but not/

[oystein (OOO til 10th of July), 2016-07-20 21:19:37]

lgtm!

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message.h (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message.h:57: void
set_handle(ProtoZeroMessageHandleBase* handle) {
Maybe wrap the whole function in DCHECK_IS_ON(), so that usages of this have to
do the same and make it clearer at the callsite that this is just there for
debugging/asserting purposes.

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message_handle.cc (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.cc:51: // doing so, though.
Threfore this case is deliberately ignored (if hit, it
typo: Therefore

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message_handle.h (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.h:24: // Think to this as a
WeakPtr<ProtoZeroMessage> which calls
nit: s/to/about/

[Primiano Tucci (use gerrit), 2016-07-21 10:49:57]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[Primiano Tucci (use gerrit), 2016-07-21 10:50:03]

Thanks folks!

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message.h (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message.h:57: void
set_handle(ProtoZeroMessageHandleBase* handle) {
On 2016/07/20 21:19:37, oystein wrote:
> Maybe wrap the whole function in DCHECK_IS_ON(), so that usages of this have
to
> do the same and make it clearer at the callsite that this is just there for
> debugging/asserting purposes.

Done.

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message_handle.cc (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.cc:51: // doing so, though.
Threfore this case is deliberately ignored (if hit, it
On 2016/07/20 21:19:37, oystein wrote:
> typo: Therefore

Done.

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.cc:53: // useles null-check.
On 2016/07/20 18:35:00, alph wrote:
> typo: useless

Done.

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message_handle.h (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.h:22: //   when when adding
two repeated messages, the addition of the 2nd one forces
On 2016/07/20 18:35:00, alph wrote:
> double when

Done.

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.h:24: // Think to this as a
WeakPtr<ProtoZeroMessage> which calls
On 2016/07/20 21:19:37, oystein wrote:
> nit: s/to/about/

Done.

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_handle.h:27: // Use the templated
version below. This base class is just here to avoid
On 2016/07/20 18:35:00, alph wrote:
> no need to go into such explanation as this is the common pattern. protected
> constructor would explain that.

Oh good point. Also the protected ctor removes the need of deleting the generic
ctor in the template. Didn't think to that.

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
File components/tracing/core/proto_zero_message_unittest.cc (right):

https://codereview.chromium.org/2158323005/diff/20001/components/tracing/core...
components/tracing/core/proto_zero_message_unittest.cc:318: // This move should
cause |child_msg_1| to be finalized, while |child_msg_2|
On 2016/07/20 18:35:00, alph wrote:
> s/while/but not/

Oh I must have accidentally swapped the lines. This comment was meant to be
shifted up two lines :)
Reput in place and reworded.

[commit-bot: I haz the power, 2016-07-21 10:50:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Primiano Tucci (use gerrit), 2016-07-21 11:05:33]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-07-21 11:05:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2158323005/#ps60001
(title: "final nits")

[commit-bot: I haz the power, 2016-07-21 11:05:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 12:50:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-21 12:51:57]

Description was changed from

==========
tracing v2: Introduce handles for safe usage of ProtoZeroMessage

This is to decouple the lifetime of a proto message from the underlying
chunk storage. It gives the following two guarantees:
- The underlying message is finalized (if still alive) if the handle
  goes out of scope. Concretely this guarantees that all events are
  committed to the buffer, without having to wait for the next event
  in the thread.
- In Debug / DCHECK_IS_ON builds, the handle becomes null once the
  underlying message is finalized. This is to enforce the append-only
  API and catch mis-uses bugs early (e.g., when adding two repeated
  fields, the addition of the second one will force the finalization
  of the first and invalidation of its handle)

BUG=608719
TEST=ProtoZeroMessageTest.MessageHandle
==========

to

==========
tracing v2: Introduce handles for safe usage of ProtoZeroMessage

This is to decouple the lifetime of a proto message from the underlying
chunk storage. It gives the following two guarantees:
- The underlying message is finalized (if still alive) if the handle
  goes out of scope. Concretely this guarantees that all events are
  committed to the buffer, without having to wait for the next event
  in the thread.
- In Debug / DCHECK_IS_ON builds, the handle becomes null once the
  underlying message is finalized. This is to enforce the append-only
  API and catch mis-uses bugs early (e.g., when adding two repeated
  fields, the addition of the second one will force the finalization
  of the first and invalidation of its handle)

BUG=608719
TEST=ProtoZeroMessageTest.MessageHandle

Committed: https://crrev.com/4f022543fb102a9c074308cb70e85d0a4d830cd1
Cr-Commit-Position: refs/heads/master@{#406827}
==========

[commit-bot: I haz the power, 2016-07-21 12:51:58]

Patchset 4 (id:??) landed as
https://crrev.com/4f022543fb102a9c074308cb70e85d0a4d830cd1
Cr-Commit-Position: refs/heads/master@{#406827}