New issue
Advanced search Search tips

Issue 715633 link

Starred by 1 user
Status: Fixed
Owner:
joedow@chromium.org
Closed: May 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Ensure classes which use ThreadChecker call CalledOnValidThread() in their d'tor

Project Member Reported by joedow@chromium.org, Apr 26 2017 
In a recent code review, we discovered an invalid assumption in how base::ThreadChecker works.

The assumption was that base::ThreadChecker would DCHECK if it was destroyed on the wrong thread.

This is not correct and as this assumption was shared by several folks on the team, there are a number of classes in our codebase which are not correctly verifying the thread they are being destroyed on.

This bug tracks the work needed to add 'DCHECK(thread_checker_.CalledOnValidThread()); to the d'tors of the classes which it is missing (and clean up any resulting bugs).

Note: We want to continue using ThreadChecker (vs. SequenceChecker or some of the newer thread pool classes) as our architecture relies on thread affinity for several interactions.

Comment 1 by joedow@chromium.org, Apr 26 2017 

FYI, I'm happy to work on this bug unless triage thinks it should go elsewhere.

Comment 2 by jamiewa...@chromium.org, Apr 27 2017

Labels: -Pri-2 Pri-3
Status: Assigned (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, May 15 2017 

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

commit 4d4db58f0615aa162f870821f1bba6815a8b8dd3
Author: joedow <joedow@chromium.org>
Date: Mon May 15 18:34:50 2017

Adding ThreadChecker validation to d'tors for client classes

There was an incorrect assumption about how ThreadChecker worked which lead
to several classes not having the d'tor guarantees they expected.  The
assumption was that a ThreadChecker would DCHECK if it was not destroyed on
the same thread it was bound to.  This is not correct so I am adding DCHECKs
to the d'tors of the remoting classes which were missed.

BUG= 715633 

Review-Url: https://codereview.chromium.org/2886453004
Cr-Commit-Position: refs/heads/master@{#471839}

[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/chromoting_client.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/client_telemetry_logger.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/display/fake_canvas.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/display/gl_cursor.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/display/gl_cursor_feedback.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/display/gl_desktop.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/display/gl_renderer.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/plugin/pepper_video_renderer_2d.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/client/software_video_renderer.cc
[modify] https://crrev.com/4d4db58f0615aa162f870821f1bba6815a8b8dd3/remoting/ios/display/gl_demo_screen.mm
Project Member

Comment 5 by bugdroid1@chromium.org, May 16 2017 

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

commit 1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61
Author: joedow <joedow@chromium.org>
Date: Tue May 16 23:39:34 2017

Adding ThreadChecker validation to d'tors for host classes

There was an incorrect assumption about how ThreadChecker worked which lead
to several classes not having the d'tor guarantees they expected.  The
assumption was that a ThreadChecker would DCHECK if it was not destroyed on
the same thread it was bound to.  This is not correct so I am adding DCHECKs
to the d'tors of the remoting classes which were missed.

BUG= 715633 

Review-Url: https://codereview.chromium.org/2888733002
Cr-Commit-Position: refs/heads/master@{#472250}

[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/chromeos/clipboard_aura.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/gcd_state_updater.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/heartbeat_sender.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/linux/certificate_watcher.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/linux/x11_character_injector.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/mouse_shape_pump.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/security_key/security_key_auth_handler_posix.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/security_key/security_key_auth_handler_win.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/security_key/security_key_extension_session.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/security_key/security_key_ipc_client.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/security_key/security_key_ipc_server_impl.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/security_key/security_key_message_handler.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/security_key/security_key_socket.cc
[modify] https://crrev.com/1f3e742fa36fe5d3e8e87f4084a84cf7cd98ad61/remoting/host/win/elevated_native_messaging_host.cc
Project Member

Comment 6 by bugdroid1@chromium.org, May 17 2017 

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

commit 18837eb479d946f6fd741791b2de20311235672f
Author: joedow <joedow@chromium.org>
Date: Wed May 17 18:28:24 2017

Adding ThreadChecker validation to d'tors for protocol classes

There was an incorrect assumption about how ThreadChecker worked which lead
to several classes not having the d'tor guarantees they expected.  The
assumption was that a ThreadChecker would DCHECK if it was not destroyed on
the same thread it was bound to.  This is not correct so I am adding DCHECKs
to the d'tors of the remoting classes which were missed.

BUG= 715633 

Review-Url: https://codereview.chromium.org/2890833002
Cr-Commit-Position: refs/heads/master@{#472507}

[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/audio_decode_scheduler.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/capture_scheduler.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/channel_socket_adapter.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/ice_connection_to_client.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/ice_transport_channel.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/jingle_session.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/monitored_video_stub.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/video_frame_pump.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/webrtc_audio_source_adapter.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/webrtc_connection_to_client.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/webrtc_frame_scheduler_simple.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/webrtc_transport.cc
[modify] https://crrev.com/18837eb479d946f6fd741791b2de20311235672f/remoting/protocol/webrtc_video_stream.cc

Comment 7 by joedow@chromium.org, May 17 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment