New issue
Advanced search Search tips

Issue 899813 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

Tracing tests failing on multiple builders

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Oct 29

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of dmazzoni@chromium.org

services_unittests
MemoryTracingIntegrationTest.GenerationChangeDoesntReenterMDM

content_browsertests
MemoryTracingTest.BrowserInitiatedDump

Builders failed on: 
- linux-chromeos-dbg: 
  https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-dbg
- Mac10.13 Tests (dbg): 
  https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29

Suspecting:
https://chromium-review.googlesource.com/c/chromium/src/+/1298997


 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d

commit 2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Mon Oct 29 18:11:51 2018

Revert "Introduce base::trace_event::TraceArguments helper class."

This reverts commit 8fe65f6f8a2e5bcf2027a7a6e1078c173be6cb87.

Reason for revert: Suspecting that it's causing these tests
to fail:

services_unittests
MemoryTracingIntegrationTest.GenerationChangeDoesntReenterMDM

content_browsertests
MemoryTracingTest.BrowserInitiatedDump

See bug 899813

Original change's description:
> Introduce base::trace_event::TraceArguments helper class.
> 
> This CL introduces the TraceArguments helper class to simplify
> the way TRACE_EVENTXXX macro named arguments are passed to the
> trace log. By using a small dedicated data structure, it is
> possible to generate less code at each trace call site,
> compared to the old way that relies on passing 5 specially
> crafted arguments to TraceLog methods.
> 
> For example, this optimization can save about 12 kiB in
> libmonochrome.so for the 32-bit ARM build of Chrome on Android.
> For more details, see corresponding bug entry.
> 
> This CL does the following:
> 
> - Introduce the new class in base/trace_event/trace_arguments.h
> 
> - Move the definition of TraceValue to trace_arguments.h and
>   augment the union with templated methods to support easy
>   initialization from a large set of C++ value types.
> 
> - Add new TraceLog methods to add trace events using an
>   optional TraceArguments parameter, instead of a bag of
>   5 ones (count + names + types + values + convertables).
> 
> - Simplify internal templates using C++11 universal references
>   and std::forward<> to remove special-casing convertable
>   arguments in trace event calls.
> 
> - Remove the now obsolete TraceValueUnion type from trace_event.h
> 
> - Make the TraceEvent class fully C++11 movable, to simplify
>   its usage (this removes the Initialize() and MoveFrom() methods).
> 
> - Update base/trace_event/ code to use TraceArguments and the
>   new TraceEvent move operations.
> 
> - Note that this keeps a few forwarding data types, values and
>   methods to avoid other direct callers from the Chromium tree,
>   these will be updated in future CLs. One Blink header needs
>   to be modified though because it specializes a template
>   defined in trace_arguments.h, to support sending WTF::CString
>   references as trace arguments.
> 
> BUG= 898794 
> R=​oysteine@chromium.org,chiniforooshan@chromium.org,alexiln@chromium.org
> TBR=primiano@chromium.org,torne@chromium.org
> 
> Change-Id: I8fe2225974f7044515a2f64bdc67f6d36abfe004
> Reviewed-on: https://chromium-review.googlesource.com/c/1298997
> Commit-Queue: David Turner <digit@chromium.org>
> Reviewed-by: Richard Coles <torne@chromium.org>
> Reviewed-by: oysteine <oysteine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#603500}

TBR=digit@chromium.org,primiano@chromium.org,torne@chromium.org,chiniforooshan@chromium.org,oysteine@chromium.org,alexiln@chromium.org

Change-Id: I165b96fde68d927f966bdb4ed8649dd7bb4dd0f5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  898794 , 899813
Reviewed-on: https://chromium-review.googlesource.com/c/1305250
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603564}
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/BUILD.gn
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/blame_context.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/event_name_filter_unittest.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/process_memory_dump.cc
[delete] https://crrev.com/595fa04ecb30e9a034e982d260fa321e8ac6646a/base/trace_event/trace_arguments.cc
[delete] https://crrev.com/595fa04ecb30e9a034e982d260fa321e8ac6646a/base/trace_event/trace_arguments.h
[delete] https://crrev.com/595fa04ecb30e9a034e982d260fa321e8ac6646a/base/trace_event/trace_arguments_unittest.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event.h
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_android.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_argument.h
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_etw_export_win.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_etw_export_win.h
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_impl.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_impl.h
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_event_unittest.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_log.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/trace_log.h
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/base/trace_event/traced_value.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/services/tracing/public/cpp/perfetto/trace_event_data_source.cc
[modify] https://crrev.com/2c7b7e4b92cb110d5622c00d1c7d780eaf2d283d/third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h

Components: Speed>Tracing
Labels: -Sheriff-Chromium
The revert appears to have resolved the test failures. Removing from sheriff queue.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b01ce1745dfcde22bbb4290be99030495862a1a9

commit b01ce1745dfcde22bbb4290be99030495862a1a9
Author: David 'Digit' Turner <digit@google.com>
Date: Mon Nov 19 08:37:22 2018

Avoid dangling pointers in TraceEvent::Reset().

This patch fixed the base::trace_event::TraceEvent::Reset()
method to ensure that it never creates dangling pointers.

This can happen in the following case:

- TraceEvent::Initialize() is called on an instance, with
  TRACE_EVENT_FLAG_COPY set in the |flags| argument.

  This will copy argument names, copyable string values,
  as well as the name and scope into a single heap
  allocated buffer backed by |parameter_copy_storage_|,
  and will also adjust all internal pointer fields to
  point to it.

- TraceEvent::Reset() is called on the same instance,
  this frees the storage area, but before this CL did not
  update the internal pointers, who were now dangling
  into heap-free memory!

- Later, some code will iterate over the arguments with
  a loop like:

    for (int i = 0;
         i < kTraceMaxNumArgs && arg_names_[i] != nullptr;
         ++i) {
      ...
    }

  The assumption being that an arg_names_[i] value of
  nullptr indicates the end of list. Unfortunately, in
  the case above, this will read completely invalid
  values from memory.

+ Fix TraceEvent::MoveFrom() to call other->Reset() to ensure
  that the source instance is left in consistent state.

I believe this is the source of flakiness on many tests
related to TraceEvent, and hope this fixes it.

BUG=905624,899813
R=oystene@chromium.org,primiano@chromium.org,alexilin@chromium.org,pkl@chromium.org

Change-Id: I63cbadc728130cddc68b8c92b28e1e3f584793f4
Reviewed-on: https://chromium-review.googlesource.com/c/1340308
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609207}
[modify] https://crrev.com/b01ce1745dfcde22bbb4290be99030495862a1a9/base/trace_event/trace_event_impl.cc

Sign in to add a comment