Skip to content

Commit

Permalink
[tracing] Fix all places passing non-void* pointers to TRACE_EVENT.
Browse files Browse the repository at this point in the history
Passing pointers to to TRACE_EVENT without being explicitly
converted to void* is going to be disallowed.

With TracedValue v2 (http://bit.ly/traced-value-v2), if a reference or
pointer to an object are passed TRACE_EVENT will call
WriteIntoTracedValue method for this object. This patch prepares for
that by ensuring no non-void* pointers are passed to TRACE_EVENT to avoid
confusion between "write the value of this pointer" and "serialise underlying
object" cases.

R=eseckler@chromium.org
BUG=1137154

Change-Id: Ibf6c03a5311a4a7dd3070983d81fa52fe5407618
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2678146
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Alexander Timin <altimin@chromium.org>
Auto-Submit: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#851582}
  • Loading branch information
Alexander Timin authored and Chromium LUCI CQ committed Feb 8, 2021
1 parent d32c53a commit fb70a2d
Show file tree
Hide file tree
Showing 28 changed files with 200 additions and 160 deletions.
4 changes: 2 additions & 2 deletions base/trace_event/trace_arguments_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ TEST(TraceArguments, TraceValueAppend) {
CheckStringFor(v, TRACE_VALUE_TYPE_COPY_STRING, "Some \"nice\" String");

int* p = nullptr;
v.Init(p);
v.Init(static_cast<void*>(p));
CheckJSONFor(v, TRACE_VALUE_TYPE_POINTER, "\"0x0\"");
CheckStringFor(v, TRACE_VALUE_TYPE_POINTER, "0x0");

Expand Down Expand Up @@ -223,7 +223,7 @@ TEST(TraceArguments, ConstructorSinglePointer) {
// TraceArguments destructor. This should only be possible for
// TRACE_VALUE_TYPE_CONVERTABLE instances.
{
TraceArguments args("foo_pointer", foo.get());
TraceArguments args("foo_pointer", static_cast<void*>(foo.get()));
EXPECT_EQ(1U, args.size());
EXPECT_EQ(TRACE_VALUE_TYPE_POINTER, args.types()[0]);
EXPECT_STREQ("foo_pointer", args.names()[0]);
Expand Down
2 changes: 1 addition & 1 deletion cc/tiles/gpu_image_decode_cache.cc
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,7 @@ GpuImageDecodeCache::GpuImageDecodeCache(

TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
"GpuImageDecodeCache::DarkModeFilter", "dark_mode_filter",
dark_mode_filter_);
static_cast<void*>(dark_mode_filter_));
}

GpuImageDecodeCache::~GpuImageDecodeCache() {
Expand Down
2 changes: 1 addition & 1 deletion cc/trees/layer_tree_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ void DidVisibilityChange(LayerTreeHostImpl* id, bool visible) {
if (visible) {
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(
"cc,benchmark", "LayerTreeHostImpl::SetVisible", TRACE_ID_LOCAL(id),
"LayerTreeHostImpl", id);
"LayerTreeHostImpl", static_cast<void*>(id));
return;
}

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/profiles/profile.cc
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ bool Profile::ShouldPersistSessionCookies() const {

void Profile::MaybeSendDestroyedNotification() {
TRACE_EVENT1("shutdown", "Profile::MaybeSendDestroyedNotification", "profile",
this);
static_cast<void*>(this));

if (!sent_destroyed_notification_) {
sent_destroyed_notification_ = true;
Expand Down
16 changes: 9 additions & 7 deletions chrome/browser/profiles/profile_destroyer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ ProfileDestroyer::DestroyerSet* ProfileDestroyer::pending_destroyers_ = nullptr;
// static
void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
TRACE_EVENT2("shutdown", "ProfileDestroyer::DestroyProfileWhenAppropriate",
"profile", profile, "is_off_the_record",
"profile", static_cast<void*>(profile), "is_off_the_record",
profile->IsOffTheRecord());

DCHECK(profile);
Expand Down Expand Up @@ -73,7 +73,7 @@ void ProfileDestroyer::DestroyOffTheRecordProfileNow(Profile* const profile) {
DCHECK(profile);
DCHECK(profile->IsOffTheRecord());
TRACE_EVENT2("shutdown", "ProfileDestroyer::DestroyOffTheRecordProfileNow",
"profile", profile, "OTRProfileID",
"profile", static_cast<void*>(profile), "OTRProfileID",
profile->GetOTRProfileID().ToString());
if (ResetPendingDestroyers(profile)) {
// We want to signal this in debug builds so that we don't lose sight of
Expand All @@ -92,7 +92,7 @@ void ProfileDestroyer::DestroyRegularProfileNow(Profile* const profile) {
DCHECK(profile);
DCHECK(profile->IsRegularProfile());
TRACE_EVENT1("shutdown", "ProfileDestroyer::DestroyRegularProfileNow",
"profile", profile);
"profile", static_cast<void*>(profile));

#if DCHECK_IS_ON()
// Save the raw pointers of profile and off-the-record profile for DCHECKing
Expand Down Expand Up @@ -147,7 +147,7 @@ bool ProfileDestroyer::ResetPendingDestroyers(Profile* const profile) {
ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)
: num_hosts_(0), profile_(profile) {
TRACE_EVENT2("shutdown", "ProfileDestroyer::ProfileDestroyer", "profile",
profile, "host_count", hosts->size());
static_cast<void*>(profile), "host_count", hosts->size());
if (pending_destroyers_ == NULL)
pending_destroyers_ = new DestroyerSet;
pending_destroyers_->insert(this);
Expand All @@ -164,7 +164,7 @@ ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)

ProfileDestroyer::~ProfileDestroyer() {
TRACE_EVENT2("shutdown", "ProfileDestroyer::~ProfileDestroyer", "profile",
profile_, "remaining_hosts", num_hosts_);
static_cast<void*>(profile_), "remaining_hosts", num_hosts_);

// Check again, in case other render hosts were added while we were
// waiting for the previous ones to go away...
Expand Down Expand Up @@ -193,7 +193,8 @@ ProfileDestroyer::~ProfileDestroyer() {
void ProfileDestroyer::RenderProcessHostDestroyed(
content::RenderProcessHost* host) {
TRACE_EVENT2("shutdown", "ProfileDestroyer::RenderProcessHostDestroyed",
"profile", profile_, "render_process_host", host);
"profile", static_cast<void*>(profile_), "render_process_host",
static_cast<void*>(host));
observations_.RemoveObservation(host);
if (!observations_.IsObservingAnySource()) {
// Delay the destruction one step further in case other observers need to
Expand Down Expand Up @@ -248,7 +249,8 @@ ProfileDestroyer::HostSet ProfileDestroyer::GetHostsForProfile(
continue;

TRACE_EVENT2("shutdown", "ProfileDestroyer::GetHostsForProfile", "profile",
profile_ptr, "render_process_host", render_process_host);
profile_ptr, "render_process_host",
static_cast<void*>(render_process_host));
hosts.insert(render_process_host);
}
return hosts;
Expand Down
16 changes: 9 additions & 7 deletions components/exo/buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ bool Buffer::ProduceTransferableResource(
bool secure_output_only,
viz::TransferableResource* resource) {
TRACE_EVENT1("exo", "Buffer::ProduceTransferableResource", "buffer_id",
gfx_buffer());
static_cast<const void*>(gfx_buffer()));
DCHECK(attach_count_);

// If textures are lost, destroy them to ensure that we create new ones below.
Expand Down Expand Up @@ -425,7 +425,8 @@ bool Buffer::ProduceTransferableResource(

if (release_contents_callback_.IsCancelled())
TRACE_EVENT_ASYNC_BEGIN1("exo", kBufferInUse, gpu_memory_buffer_.get(),
"buffer_id", gfx_buffer());
"buffer_id",
static_cast<const void*>(gfx_buffer()));

// Cancel pending contents release callback.
release_contents_callback_.Reset(
Expand Down Expand Up @@ -485,15 +486,15 @@ bool Buffer::ProduceTransferableResource(
void Buffer::OnAttach() {
DLOG_IF(WARNING, attach_count_)
<< "Reattaching a buffer that is already attached to another surface.";
TRACE_EVENT2("exo", "Buffer::OnAttach", "buffer_id", gfx_buffer(), "count",
attach_count_);
TRACE_EVENT2("exo", "Buffer::OnAttach", "buffer_id",
static_cast<const void*>(gfx_buffer()), "count", attach_count_);
++attach_count_;
}

void Buffer::OnDetach() {
DCHECK_GT(attach_count_, 0u);
TRACE_EVENT2("exo", "Buffer::OnAttach", "buffer_id", gfx_buffer(), "count",
attach_count_);
TRACE_EVENT2("exo", "Buffer::OnAttach", "buffer_id",
static_cast<const void*>(gfx_buffer()), "count", attach_count_);
--attach_count_;

// Release buffer if no longer attached to a surface and content has been
Expand Down Expand Up @@ -532,7 +533,8 @@ void Buffer::ReleaseContentsTexture(std::unique_ptr<Texture> texture,
}

void Buffer::ReleaseContents() {
TRACE_EVENT1("exo", "Buffer::ReleaseContents", "buffer_id", gfx_buffer());
TRACE_EVENT1("exo", "Buffer::ReleaseContents", "buffer_id",
static_cast<const void*>(gfx_buffer()));

// Cancel callback to indicate that buffer has been released.
release_contents_callback_.Cancel();
Expand Down
14 changes: 8 additions & 6 deletions components/exo/surface.cc
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,10 @@ void Surface::Attach(Buffer* buffer) {
}

void Surface::Attach(Buffer* buffer, gfx::Vector2d offset) {
TRACE_EVENT2("exo", "Surface::Attach", "buffer_id",
buffer ? buffer->gfx_buffer() : nullptr, "app_id",
GetApplicationId(window_.get()));
TRACE_EVENT2(
"exo", "Surface::Attach", "buffer_id",
buffer ? static_cast<const void*>(buffer->gfx_buffer()) : nullptr,
"app_id", GetApplicationId(window_.get()));
has_pending_contents_ = true;
pending_state_.buffer.Reset(buffer ? buffer->AsWeakPtr()
: base::WeakPtr<Buffer>());
Expand Down Expand Up @@ -620,9 +621,10 @@ bool Surface::HasPendingAcquireFence() const {

void Surface::Commit() {
TRACE_EVENT1("exo", "Surface::Commit", "buffer_id",
pending_state_.buffer.buffer()
? pending_state_.buffer.buffer()->gfx_buffer()
: nullptr);
static_cast<const void*>(
pending_state_.buffer.buffer()
? pending_state_.buffer.buffer()->gfx_buffer()
: nullptr));

for (auto& observer : observers_)
observer.OnCommit(this);
Expand Down
15 changes: 8 additions & 7 deletions content/browser/browser_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -356,10 +356,10 @@ void BrowserContext::FirePushSubscriptionChangeEvent(
// static
void BrowserContext::NotifyWillBeDestroyed(BrowserContext* browser_context) {
TRACE_EVENT1("shutdown", "BrowserContext::NotifyWillBeDestroyed",
"browser_context", browser_context);
"browser_context", static_cast<void*>(browser_context));
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(
"shutdown", "BrowserContext::NotifyWillBeDestroyed() called.",
browser_context, "browser_context", browser_context);
browser_context, "browser_context", static_cast<void*>(browser_context));
// Make sure NotifyWillBeDestroyed is idempotent. This helps facilitate the
// pattern where NotifyWillBeDestroyed is called from *both*
// ShellBrowserContext and its derived classes (e.g. WebTestBrowserContext).
Expand Down Expand Up @@ -459,14 +459,15 @@ void BrowserContext::SetPermissionControllerForTesting(
BrowserContext::BrowserContext()
: unique_id_(base::UnguessableToken::Create().ToString()) {
TRACE_EVENT1("shutdown", "BrowserContext::BrowserContext", "browser_context",
this);
static_cast<void*>(this));
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1("shutdown", "Browser.BrowserContext", this,
"browser_context", this);
"browser_context",
static_cast<void*>(this));
}

BrowserContext::~BrowserContext() {
TRACE_EVENT1("shutdown", "BrowserContext::~BrowserContext", "browser_context",
this);
static_cast<void*>(this));
DCHECK(!GetUserData(kStoragePartitionMapKeyName))
<< "StoragePartitionMap is not shut down properly";

Expand Down Expand Up @@ -512,9 +513,9 @@ BrowserContext::~BrowserContext() {

TRACE_EVENT_NESTABLE_ASYNC_END1(
"shutdown", "BrowserContext::NotifyWillBeDestroyed() called.", this,
"browser_context", this);
"browser_context", static_cast<void*>(this));
TRACE_EVENT_NESTABLE_ASYNC_END1("shutdown", "Browser.BrowserContext", this,
"browser_context", this);
"browser_context", static_cast<void*>(this));
}

void BrowserContext::ShutdownStoragePartitions() {
Expand Down
6 changes: 4 additions & 2 deletions content/browser/loader/navigation_url_loader_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1261,7 +1261,8 @@ void NavigationURLLoaderImpl::NotifyResponseStarted(
// "Navigation.NavigationURLLoaderImplIOPostTime" histogram as well.

TRACE_EVENT_ASYNC_END2("navigation", "Navigation timeToResponseStarted", this,
"&NavigationURLLoaderImpl", this, "success", true);
"&NavigationURLLoaderImpl", static_cast<void*>(this),
"success", true);

if (is_download)
download_policy_.RecordHistogram();
Expand Down Expand Up @@ -1289,7 +1290,8 @@ void NavigationURLLoaderImpl::NotifyRequestRedirected(
void NavigationURLLoaderImpl::NotifyRequestFailed(
const network::URLLoaderCompletionStatus& status) {
TRACE_EVENT_ASYNC_END2("navigation", "Navigation timeToResponseStarted", this,
"&NavigationURLLoaderImpl", this, "success", false);
"&NavigationURLLoaderImpl", static_cast<void*>(this),
"success", false);
delegate_->OnRequestFailed(status);
}

Expand Down
5 changes: 3 additions & 2 deletions content/browser/renderer_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4673,7 +4673,8 @@ void RenderFrameHostImpl::EvictFromBackForwardCacheWithReason(
void RenderFrameHostImpl::EvictFromBackForwardCacheWithReasons(
const BackForwardCacheCanStoreDocumentResult& can_store) {
TRACE_EVENT2("navigation", "RenderFrameHostImpl::EvictFromBackForwardCache",
"can_store", can_store.ToString(), "rfh", this);
"can_store", can_store.ToString(), "rfh",
static_cast<void*>(this));
DCHECK(IsBackForwardCacheEnabled());

if (is_evicted_from_back_forward_cache_)
Expand Down Expand Up @@ -8910,7 +8911,7 @@ bool RenderFrameHostImpl::DidCommitNavigationInternal(

if (navigated_to_new_document) {
TRACE_EVENT1("content", "DidCommitProvisionalLoad_StateResetForNewDocument",
"render_frame_host", this);
"render_frame_host", static_cast<void*>(this));

if (navigation_request->IsInMainFrame()) {
render_view_host_->ResetPerPageState();
Expand Down
Loading

0 comments on commit fb70a2d

Please sign in to comment.