New issue
Advanced search Search tips

Issue 821453 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

DCHECK: Performance Monitor posts un-owned handles

Project Member Reported by siggi@chromium.org, Mar 13 2018

Issue description

I just had a DCHECK in my local build at 

base::debug::BreakDebugger() Line 21
logging::LogMessage::~LogMessage() Line 857
base::ProcessMetrics::ProcessMetrics(void * process) Line 298
base::ProcessMetrics::CreateProcessMetrics(void * process) Line 46
performance_monitor::ProcessMetricsHistory::Initialize(const performance_monitor::ProcessMetricsMetadata & process_data, int initial_update_sequence) Line 53
performance_monitor::PerformanceMonitor::MarkProcessAsAlive(const performance_monitor::ProcessMetricsMetadata & process_data, int current_update_sequence) Line 133
performance_monitor::PerformanceMonitor::MarkProcessesAsAliveOnUIThread(std::unique_ptr<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> >,std::default_delete<std::vector<performance_monitor::ProcessMetricsMetadata,std::allocator<performance_monitor::ProcessMetricsMetadata> > > > process_data_list, int current_update_sequence) Line 179

On inspection, I see that PerformanceMonitor::UpdateMetricsOnIOThread posts a vector of ProcessMetricsMetadata from IO to UI thread. This contains un-owned process handles, which can't be used except in-context on Windows. 

On the UI thread, these handles are then duplicated by the ProcessMetrics, which may lead to all sorts of trouble if the handles have been closed and reused in the meantime (Windows reuses handles aggressively).

I don't know what we use this data for, but this will cause false positive process tracking, false negative process tracking and possibly rare random hangs on Windows.

A reasonable way to deal with this would be something like duplicating the handles at source and to pass them owned by a base::Process instance.
 

Comment 1 by siggi@chromium.org, Mar 13 2018

Cc: simonhatch@chromium.org

Comment 2 by siggi@chromium.org, Mar 13 2018

Summary: DCHECK: Performance Monitor posts un-owned handles (was: DCHECK: Process Monitor posts un-owned handles)

Comment 3 by siggi@chromium.org, Mar 14 2018

I'm getting regular DCHECKs here in my local build - looks like this is a god-honest race.

Comment 4 by siggi@chromium.org, Mar 14 2018

Cc: fdoray@chromium.org

Comment 5 by siggi@chromium.org, Apr 5 2018

Labels: Hotlist-TooManyTabs
Owner: siggi@chromium.org
The same problem exists in the render process probe for the resource coordinator.

Comment 6 by siggi@chromium.org, Apr 10 2018

Status: Started (was: Untriaged)

Comment 7 by siggi@chromium.org, Apr 10 2018

Cc: siggi@chromium.org
 Issue 831163  has been merged into this issue.
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 16 2018

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

commit eb95666dc91287342eeaeb0bc82c032de379f0db
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Mon Apr 16 16:16:03 2018

Fix a process handle usage race in resource coordinator render probe.

The GetHandle function of RenderProcessHost returned a
base::ProcessHandle, which cannot be passed around to other
threads because the caller does not have ownership. This
lead to handle abuse problems on Windows if the RPH goes
away as the handle is being bounced around.
This CL replaces the GetHandle function of RPH with a
GetProcess function, which returns a const base::Process&.
This in turn allows callers to duplicate the underlying
base::Process, and the duplicate can then be safely bounced
around to other threads.

Bug: 821453
Change-Id: I7d9956c3ab92352d99ccea70fde3f2013aa66d33
Reviewed-on: https://chromium-review.googlesource.com/998213
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550988}
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/chromeos/power/renderer_freezer.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/extensions/api/processes/processes_api.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/media/webrtc/webrtc_video_display_perf_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/memory_details.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/metrics/process_memory_metrics_emitter.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/performance_monitor/performance_monitor.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/profiling_host/profiling_test_driver.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/spellchecker/spellcheck_service.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/task_manager/providers/render_process_host_task_provider.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/task_manager/providers/web_contents/renderer_task.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/ui/views/hung_renderer_view.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/ui/webui/memory_internals_ui.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/components/crash/content/browser/crash_dump_observer_android.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/components/heap_profiling/client_connection_manager.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/accessibility/captioning_controller.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/devtools/protocol/tracing_handler.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/frame_host/frame_tree_node_blame_context.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/memory/memory_coordinator_impl.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/memory/memory_coordinator_impl_unittest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/renderer_host/pepper/pepper_file_io_host.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/service_worker/service_worker_internals_ui.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/tracing/memory_instrumentation_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/webrtc/webrtc_audio_debug_recordings_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/public/browser/render_process_host.h
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/shell/browser/layout_test/blink_test_controller.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/extensions/browser/user_script_loader.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb95666dc91287342eeaeb0bc82c032de379f0db

commit eb95666dc91287342eeaeb0bc82c032de379f0db
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Mon Apr 16 16:16:03 2018

Fix a process handle usage race in resource coordinator render probe.

The GetHandle function of RenderProcessHost returned a
base::ProcessHandle, which cannot be passed around to other
threads because the caller does not have ownership. This
lead to handle abuse problems on Windows if the RPH goes
away as the handle is being bounced around.
This CL replaces the GetHandle function of RPH with a
GetProcess function, which returns a const base::Process&.
This in turn allows callers to duplicate the underlying
base::Process, and the duplicate can then be safely bounced
around to other threads.

Bug: 821453
Change-Id: I7d9956c3ab92352d99ccea70fde3f2013aa66d33
Reviewed-on: https://chromium-review.googlesource.com/998213
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550988}
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/chromeos/power/renderer_freezer.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/extensions/api/processes/processes_api.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/media/webrtc/webrtc_video_display_perf_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/memory_details.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/metrics/process_memory_metrics_emitter.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/performance_monitor/performance_monitor.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/profiling_host/profiling_test_driver.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/spellchecker/spellcheck_service.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/task_manager/providers/render_process_host_task_provider.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/task_manager/providers/web_contents/renderer_task.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/ui/views/hung_renderer_view.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/chrome/browser/ui/webui/memory_internals_ui.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/components/crash/content/browser/crash_dump_observer_android.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/components/heap_profiling/client_connection_manager.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/accessibility/captioning_controller.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/devtools/protocol/tracing_handler.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/frame_host/frame_tree_node_blame_context.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/memory/memory_coordinator_impl.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/memory/memory_coordinator_impl_unittest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/renderer_host/pepper/pepper_file_io_host.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/service_worker/service_worker_internals_ui.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/tracing/memory_instrumentation_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/browser/webrtc/webrtc_audio_debug_recordings_browsertest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/public/browser/render_process_host.h
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/content/shell/browser/layout_test/blink_test_controller.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/eb95666dc91287342eeaeb0bc82c032de379f0db/extensions/browser/user_script_loader.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 26

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

commit c31de3260f48a6c197d698f8f2e295cd9f04d49b
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Thu Jul 26 20:24:07 2018

Fix invalid handle errors found with App Verifier

ChildProcessData was holding a raw ProcessHandle which meant that there
was no guarantee that the handle would be valid when it was used. This
change switches to a ScopedHandle on Windows, with the associated
duplication of the handle, so that it is guaranteed to be valid. This
fixes a frequently encountered use of an invalid handle that was
detected by Application Verifier. This change also adds a CHECK to
base::GetProcId to check for use of invalid handles even when not using
Application Verifier.

ChildProcessData::handle is now a private member so all references to
it were updated to use accessors.

When ChildProcessData::SetHandle is called the handle is duplicated.
When a ChildProcessData object needs to be copied an explicit
Duplicate function is called.

An IsHandleValid helper function was added.

An invalid test was removed because it was triggering the CHECK in
base::GetProcId.

Bug: 417532,821453, 865805 
Change-Id: I7e38140335c0140536919341f011f144f150c88f
Reviewed-on: https://chromium-review.googlesource.com/1145767
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578419}
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/base/process/process_handle_win.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/android/compositor/compositor_view.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/android/feedback/process_id_feedback_source.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/chrome_content_browser_client_browsertest_chromeos.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/chrome_plugin_browsertest.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/chromeos/app_mode/app_session.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/extensions/api/processes/processes_api.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/image_decoder_browsertest.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/memory_details.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/performance_monitor/performance_monitor.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/resource_coordinator/browser_child_process_watcher.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/task_manager/providers/child_process_task.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/task_manager/providers/child_process_task_provider.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/task_manager/providers/child_process_task_provider.h
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/task_manager/providers/child_process_task_unittest.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/task_manager/providers/render_process_host_task_provider.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/ui/hung_plugin_tab_helper.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/chrome/browser/ui/webui/memory_internals_ui.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/components/browser_watcher/exit_code_watcher_win_unittest.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/components/crash/content/browser/child_exit_observer_android.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/components/heap_profiling/client_connection_manager.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/components/nacl/browser/nacl_broker_host_win.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/components/nacl/browser/nacl_process_host.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/browser/histogram_controller.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/browser/ppapi_plugin_process_host.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/browser/renderer_host/media/audio_service_listener.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/browser/renderer_host/media/audio_service_listener_unittest.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/public/browser/BUILD.gn
[add] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/public/browser/child_process_data.cc
[modify] https://crrev.com/c31de3260f48a6c197d698f8f2e295cd9f04d49b/content/public/browser/child_process_data.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 27

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

commit c03a192a88c2514a21121251fe6ea0300d0d2353
Author: Scott Violet <sky@chromium.org>
Date: Fri Jul 27 15:38:34 2018

Revert "Fix invalid handle errors found with App Verifier"

This reverts commit c31de3260f48a6c197d698f8f2e295cd9f04d49b.

Reason for revert: Findit thinks this is the cause for SingleClientAppsSyncTest.StartWithSomePlatformApps is Flaky becoming flaky. Debug output from the bot shows CHECK in process_handle_win:

[5744:6016:0727/011726.878:FATAL:process_handle_win.cc(29)] Check failed: result != 0 || GetLastError() != ERROR_INVALID_HANDLE. process handle = 00000FB4
Backtrace:
	base::debug::StackTrace::StackTrace [0x73D7A1D6+102]
	base::debug::StackTrace::StackTrace [0x73D7927B+27]
	logging::LogMessage::~LogMessage [0x73DDEFA4+148]
	base::GetProcId [0x73EC8E8E+254]
	content::ChildConnection::IOThreadContext::SetProcessHandleOnIOThread [0x617A7C58+232]
	base::internal::FunctorTraits<void (__thiscall content::ChildConnection::IOThreadContext::*)(void *),void>::Invoke<void (__thiscall content::ChildConnection::IOThreadContext::*)(void *),scoped_refptr<content::ChildConnection::IOThreadContext>,void *> [0x617A8001+81]
	base::internal::InvokeHelper<0,void>::MakeItSo<void (__thiscall content::ChildConnection::IOThreadContext::*)(void *),scoped_refptr<content::ChildConnection::IOThreadContext>,void *> [0x617A7F0C+124]
	base::internal::Invoker<base::internal::BindState<void (__thiscall content::ChildConnection::IOThreadContext::*)(void *),scoped_refptr<content::ChildConnection::IOThreadContext>,void *>,void __cdecl(void)>::RunImpl<void (__thiscall content::ChildConnectio [0x617A7E3F+111]
	base::internal::Invoker<base::internal::BindState<void (__thiscall content::ChildConnection::IOThreadContext::*)(void *),scoped_refptr<content::ChildConnection::IOThreadContext>,void *>,void __cdecl(void)>::RunOnce [0x617A7CE4+84]
	base::OnceCallback<void __cdecl(void)>::Run [0x73D1CA90+80]
	base::debug::TaskAnnotator::RunTask [0x73D7E9F3+1075]
	base::MessageLoop::RunTask [0x73E19A64+884]
	base::MessageLoop::DeferOrRunPendingTask [0x73E1A249+73]
	base::MessageLoop::DoWork [0x73E1A5BA+442]
	base::MessagePumpForIO::DoRunLoop [0x73E2775C+28]
	base::MessagePumpWin::Run [0x73E25CA7+119]
	base::MessageLoop::Run [0x73E19366+486]
	base::RunLoop::Run [0x73EDF518+488]
	base::Thread::Run [0x73FF8EF7+375]
	content::BrowserProcessSubThread::IOThreadRun [0x61B456A0+48]
	content::BrowserProcessSubThread::Run [0x61B45589+313]
	base::Thread::ThreadMain [0x73FF97BA+1402]
	base::PlatformThread::GetCurrentThreadPriority [0x73FEBC76+934]
	BaseThreadInitThunk [0x74AD336A+18]
	RtlInitializeExceptionChain [0x771F9902+99]
	RtlInitializeExceptionChain [0x771F98D5+54]

Original change's description:
> Fix invalid handle errors found with App Verifier
> 
> ChildProcessData was holding a raw ProcessHandle which meant that there
> was no guarantee that the handle would be valid when it was used. This
> change switches to a ScopedHandle on Windows, with the associated
> duplication of the handle, so that it is guaranteed to be valid. This
> fixes a frequently encountered use of an invalid handle that was
> detected by Application Verifier. This change also adds a CHECK to
> base::GetProcId to check for use of invalid handles even when not using
> Application Verifier.
> 
> ChildProcessData::handle is now a private member so all references to
> it were updated to use accessors.
> 
> When ChildProcessData::SetHandle is called the handle is duplicated.
> When a ChildProcessData object needs to be copied an explicit
> Duplicate function is called.
> 
> An IsHandleValid helper function was added.
> 
> An invalid test was removed because it was triggering the CHECK in
> base::GetProcId.
> 
> Bug: 417532,821453, 865805 
> Change-Id: I7e38140335c0140536919341f011f144f150c88f
> Reviewed-on: https://chromium-review.googlesource.com/1145767
> Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#578419}

TBR=brucedawson@chromium.org,jochen@chromium.org

Change-Id: I8569cb32ec0cd9cbd99ecf6d5bba7feb672a88cd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 417532, 821453,  865805 ,  868384 
Reviewed-on: https://chromium-review.googlesource.com/1152374
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578652}
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/base/process/process_handle_win.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/android/compositor/compositor_view.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/android/feedback/process_id_feedback_source.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/chrome_content_browser_client_browsertest_chromeos.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/chrome_plugin_browsertest.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/chromeos/app_mode/app_session.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/extensions/api/processes/processes_api.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/image_decoder_browsertest.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/memory_details.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/performance_monitor/performance_monitor.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/resource_coordinator/browser_child_process_watcher.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/task_manager/providers/child_process_task.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/task_manager/providers/child_process_task_provider.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/task_manager/providers/child_process_task_provider.h
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/task_manager/providers/child_process_task_unittest.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/task_manager/providers/render_process_host_task_provider.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/ui/hung_plugin_tab_helper.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/chrome/browser/ui/webui/memory_internals_ui.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/components/browser_watcher/exit_code_watcher_win_unittest.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/components/crash/content/browser/child_exit_observer_android.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/components/heap_profiling/client_connection_manager.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/components/nacl/browser/nacl_broker_host_win.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/components/nacl/browser/nacl_process_host.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/content/browser/histogram_controller.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/content/browser/ppapi_plugin_process_host.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/content/browser/renderer_host/media/audio_service_listener.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/content/browser/renderer_host/media/audio_service_listener_unittest.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/content/public/browser/BUILD.gn
[delete] https://crrev.com/e30800ae4a616c0e3fc040c57a7fd20aa7b3f7c1/content/public/browser/child_process_data.cc
[modify] https://crrev.com/c03a192a88c2514a21121251fe6ea0300d0d2353/content/public/browser/child_process_data.h

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 31

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

commit 02f07de804a9c247c5b6405f0d7dde88da6bee3c
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Tue Jul 31 17:28:26 2018

Reland "Fix invalid handle errors found with App Verifier"

This is a reland of c31de3260f48a6c197d698f8f2e295cd9f04d49b. The CHECK
that was triggering (crbug.com/868341) has been removed but the change
is otherwise the same. The CHECK will be readded once all of the bugs
that it reveals are fixed (crbug.com/869154)

Original change's description:
> Fix invalid handle errors found with App Verifier
>
> ChildProcessData was holding a raw ProcessHandle which meant that there
> was no guarantee that the handle would be valid when it was used. This
> change switches to a ScopedHandle on Windows, with the associated
> duplication of the handle, so that it is guaranteed to be valid. This
> fixes a frequently encountered use of an invalid handle that was
> detected by Application Verifier. This change also adds a CHECK to
> base::GetProcId to check for use of invalid handles even when not using
> Application Verifier.
>
> ChildProcessData::handle is now a private member so all references to
> it were updated to use accessors.
>
> When ChildProcessData::SetHandle is called the handle is duplicated.
> When a ChildProcessData object needs to be copied an explicit
> Duplicate function is called.
>
> An IsHandleValid helper function was added.
>
> An invalid test was removed because it was triggering the CHECK in
> base::GetProcId.
>
> Bug: 417532,821453, 865805 
> Change-Id: I7e38140335c0140536919341f011f144f150c88f
> Reviewed-on: https://chromium-review.googlesource.com/1145767
> Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#578419}

Bug: 417532, 821453,  865805 , 868341, 869154
Change-Id: I022982f447cce4649618f4413e960f5639ad750a
Reviewed-on: https://chromium-review.googlesource.com/1155498
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579454}
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/android/compositor/compositor_view.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/android/feedback/process_id_feedback_source.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/chrome_content_browser_client_browsertest_chromeos.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/chrome_plugin_browsertest.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/chromeos/app_mode/app_session.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/extensions/api/processes/processes_api.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/image_decoder_browsertest.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/memory_details.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/performance_monitor/performance_monitor.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/resource_coordinator/browser_child_process_watcher.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/child_process_task.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/child_process_task_provider.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/child_process_task_provider.h
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/child_process_task_unittest.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/render_process_host_task_provider.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/ui/hung_plugin_tab_helper.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/ui/webui/memory_internals_ui.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/browser_watcher/exit_code_watcher_win_unittest.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/crash/content/browser/child_exit_observer_android.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/heap_profiling/client_connection_manager.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/nacl/browser/nacl_broker_host_win.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/nacl/browser/nacl_process_host.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/histogram_controller.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/ppapi_plugin_process_host.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/renderer_host/media/audio_service_listener.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/renderer_host/media/audio_service_listener_unittest.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/public/browser/BUILD.gn
[add] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/public/browser/child_process_data.cc
[modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/public/browser/child_process_data.h

Sign in to add a comment