Invoke trace callback even when trace buffer is full

Invoke trace callback even when trace buffer is full

We used to invoke trace event callbacks only when we successfully
stored an event in the buffer. This does not work for clients that
maintain their own buffers, so let's invoke callbacks unconditionally.

BUG=173184
R=dsinclair@google.com, jar@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197009

[jar (doing other things), 2013-04-26 16:43:31]

Please also get approval from dsinclair... but patch set 1 LGTM

https://codereview.chromium.org/13912028/diff/1/base/debug/trace_event_unitte...
File base/debug/trace_event_unittest.cc (right):

https://codereview.chromium.org/13912028/diff/1/base/debug/trace_event_unitte...
base/debug/trace_event_unittest.cc:1672: ASSERT_EQ(2u,
collected_events_.size());
nice catch/correction.

[dsinclair1, 2013-04-26 23:26:07]

Lgtm
On Apr 26, 2013 5:31 AM, <caseq@google.com> wrote:

> Reviewers: dsinclair, jar,
>
> Description:
> Invoke trace callback even when trace buffer is full
>
> We used to invoke trace event callbacks only when we successfully
> stored an event in the buffer. This does not work for clients that
> maintain their own buffers, so let's invoke callbacks unconditionally.
>
> BUG=173184
>
> Please review this at
https://codereview.chromium.**org/13912028/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M base/debug/trace_event_impl.cc
>   M base/debug/trace_event_**unittest.cc
>
>
> Index: base/debug/trace_event_impl.cc
> diff --git a/base/debug/trace_event_impl.**cc
> b/base/debug/trace_event_impl.**cc
> index 860d989489488b8407292b3ba91699**26e39e7463..**
> 38a6c0fe311df6ae92f716559d939b**19d318de28 100644
> --- a/base/debug/trace_event_impl.**cc
> +++ b/base/debug/trace_event_impl.**cc
> @@ -1000,12 +1000,14 @@ void TraceLog::**AddTraceEventWithThreadIdAndTi**
> mestamp(
>
>    NotificationHelper notifier(this);
>
> -  {
> +  do {
>      AutoLock lock(lock_);
>      if (*category_group_enabled != CATEGORY_ENABLED)
>        return;
> +
> +    event_callback_copy = event_callback_;
>      if (logged_events_->IsFull())
> -      return;
> +      break;
>
>      const char* new_name = ThreadIdNameManager::**GetInstance()->
>          GetName(thread_id);
> @@ -1047,9 +1049,7 @@ void TraceLog::**AddTraceEventWithThreadIdAndTi**
> mestamp(
>
>      if (watch_category_ == category_group_enabled && watch_event_name_ ==
> name)
>        notifier.**AddNotificationWhileLocked(**EVENT_WATCH_NOTIFICATION);
> -
> -    event_callback_copy = event_callback_;
> -  }  // release lock
> +  } while (0); // release lock
>
>    notifier.**SendNotificationIfAny();
>    if (event_callback_copy != NULL) {
> Index: base/debug/trace_event_**unittest.cc
> diff --git a/base/debug/trace_event_**unittest.cc
> b/base/debug/trace_event_**unittest.cc
> index 64f992d50302110a2cc1bd2f8b0cea**da92790987..**
> a78b002066eb0ffb17b17a91d81ce0**0a037e9b34 100644
> --- a/base/debug/trace_event_**unittest.cc
> +++ b/base/debug/trace_event_**unittest.cc
> @@ -55,6 +55,7 @@ class TraceEventTestFixture : public testing::Test {
>    void OnTraceNotification(int notification) {
>      if (notification & TraceLog::EVENT_WATCH_**NOTIFICATION)
>        ++event_watch_notification_;
> +      notifications_received_ |= notification;
>    }
>    DictionaryValue* FindMatchingTraceEntry(const JsonKeyValue* key_values);
>    DictionaryValue* FindNamePhase(const char* name, const char* phase);
> @@ -73,6 +74,7 @@ class TraceEventTestFixture : public testing::Test {
>
>    void BeginTrace() {
>      event_watch_notification_ = 0;
> +    notifications_received_ = 0;
>      TraceLog::GetInstance()->**SetEnabled(CategoryFilter("*")**,
>                                          TraceLog::RECORD_UNTIL_FULL);
>    }
> @@ -102,6 +104,7 @@ class TraceEventTestFixture : public testing::Test {
>    base::debug::TraceResultBuffer trace_buffer_;
>    base::debug::**TraceResultBuffer::**SimpleOutput json_output_;
>    int event_watch_notification_;
> +  int notifications_received_;
>
>   private:
>    // We want our singleton torn down after each test.
> @@ -1666,11 +1669,24 @@ TEST_F(TraceEventCallbackTest, TraceEventCallback)
> {
>    TraceLog::GetInstance()->**SetEventCallback(NULL);
>    TRACE_EVENT_INSTANT0("all", "after callback removed",
>                         TRACE_EVENT_SCOPE_GLOBAL);
> -  EXPECT_EQ(2u, collected_events_.size());
> +  ASSERT_EQ(2u, collected_events_.size());
>    EXPECT_EQ("event1", collected_events_[0]);
>    EXPECT_EQ("event2", collected_events_[1]);
>  }
>
> +TEST_F(**TraceEventCallbackTest, TraceEventCallbackWhileFull) {
> +  TraceLog::GetInstance()->**SetEnabled(CategoryFilter("*")**,
> +      TraceLog::RECORD_UNTIL_FULL);
> +  do {
> +    TRACE_EVENT_INSTANT0("all", "badger badger",
> TRACE_EVENT_SCOPE_GLOBAL);
> +  } while ((notifications_received_ & TraceLog::TRACE_BUFFER_FULL) == 0);
> +  TraceLog::GetInstance()->**SetEventCallback(Callback);
> +  TRACE_EVENT_INSTANT0("all", "a snake", TRACE_EVENT_SCOPE_GLOBAL);
> +  TraceLog::GetInstance()->**SetEventCallback(NULL);
> +  ASSERT_EQ(1u, collected_events_.size());
> +  EXPECT_EQ("a snake", collected_events_[0]);
> +}
> +
>  // TODO(dsinclair): Continuous Tracing unit test.
>
>  // Test the category filter.
>
>
>

[caseq1, 2013-04-29 09:08:36]

Committed patchset #1 manually as r197009 (presubmit successful).