New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 787554 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Chrome
Pri: 3
Type: Bug

Blocking:
issue 786941



Sign in to add a comment

re2 searches done in the feedback component block the UI thread

Project Member Reported by jochen@chromium.org, Nov 21 2017

Issue description

on my windows canary, this results in ~120s hangs of the browser. If we can't bound the input data, we shouldn't evaluate regular expressions on the ui thread.
 

Comment 1 by jochen@chromium.org, Nov 23 2017

Status: WontFix (was: Untriaged)
my codesearch-fu left me for a bit here, not sure whether feedback is to blame for the RE2 usage

Comment 2 by battre@chromium.org, Nov 23 2017

Cc: -battre@chromium.org eisinger@chromium.org
Owner: battre@chromium.org
Status: Assigned (was: WontFix)
This is valid... See SystemLogsFetcher::OnFetched's call to anonymizer_->Anonymize()
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 28 2017

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

commit 32f03c7af5378ddc65cc849b218a1a66d978b3b3
Author: Dominic Battre <battre@chromium.org>
Date: Tue Nov 28 17:59:49 2017

Clear up ownership of SystemLogsResponse objects in callbacks.

This CL changes the callback of SysLogsSources::Fetch such that the generated
SystemLogsResponse is passed as a unique_ptr instead of a raw pointer. This
allows further cleanup in the form of processing the SystemLogsResponse
asynchronously, because the receiver of the SystemLogsResponse can be sure to
be the true owner and does not need to worry about the caller deallocating the
object.

TBR=rkc@chromium.org

Bug:  787554 
Change-Id: I85ddfd2f3972039cea0eca43b0fce3bc5d0ba105
Reviewed-on: https://chromium-review.googlesource.com/787470
Commit-Queue: Dominic Battré <battre@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519741}
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/command_line_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/dbus_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/debug_daemon_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/debug_daemon_log_source.h
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/device_event_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/lsb_release_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/single_debug_daemon_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/single_debug_daemon_log_source_unittest.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/single_log_file_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/single_log_file_log_source_unittest.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/components/feedback/system_logs/system_logs_fetcher.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/components/feedback/system_logs/system_logs_fetcher.h
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/components/feedback/system_logs/system_logs_source.h
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/extensions/browser/api/feedback_private/feedback_private_api_unittest_base_chromeos.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/extensions/browser/api/feedback_private/log_source_access_manager.cc
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/extensions/browser/api/feedback_private/log_source_access_manager.h
[modify] https://crrev.com/32f03c7af5378ddc65cc849b218a1a66d978b3b3/extensions/shell/browser/system_logs/log_sources/basic_log_source.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 4 2017

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

commit f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc
Author: Simon Que <sque@chromium.org>
Date: Mon Dec 04 23:26:48 2017

Move AnonymizerTool from Single*LogSource to LogSourceAccessManager

Instead of each log source having to handle anonymization, just let the
feedbackPrivate API handle it.

Bug:  787554 
Change-Id: I8e25235fe2aa2f01aeed4e32c4f23aa925712367
Reviewed-on: https://chromium-review.googlesource.com/806681
Commit-Queue: Simon Que <sque@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521508}
[modify] https://crrev.com/f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc/chrome/browser/chromeos/system_logs/single_debug_daemon_log_source.cc
[modify] https://crrev.com/f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc/chrome/browser/chromeos/system_logs/single_log_file_log_source.cc
[modify] https://crrev.com/f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc/chrome/browser/chromeos/system_logs/single_log_file_log_source.h
[modify] https://crrev.com/f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc/chrome/browser/chromeos/system_logs/single_log_file_log_source_unittest.cc
[modify] https://crrev.com/f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc/extensions/browser/api/feedback_private/feedback_private_api_chromeos_unittest.cc
[modify] https://crrev.com/f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc/extensions/browser/api/feedback_private/feedback_private_api_unittest_base_chromeos.cc
[modify] https://crrev.com/f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc/extensions/browser/api/feedback_private/log_source_access_manager.cc
[modify] https://crrev.com/f378a0d6495ad7f97a7cf5ad21ccaade641a4bdc/extensions/browser/api/feedback_private/log_source_access_manager.h

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 7 2017

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

commit f091addfc00ae9e2f82caa15ff5a5e498f654c35
Author: Dominic Battre <battre@chromium.org>
Date: Thu Dec 07 15:45:34 2017

Take AnonymizerTool calls off the UI thread

Executing the AnonymizerTool is an expensive operation and should not happen on
the UI thread. This CL moves all existing calls on the UI thread to spearate
worker TaskRunners.

Bug:  787554 
Change-Id: I3c096873f784cc2a9dd23f9dd538e2aebd075cd0
Reviewed-on: https://chromium-review.googlesource.com/787713
Commit-Queue: Dominic Battré <battre@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>
Reviewed-by: Bartosz Fabianowski <bartfab@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522435}
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/chrome/browser/chromeos/policy/system_log_uploader_unittest.cc
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/components/feedback/anonymizer_tool.cc
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/components/feedback/anonymizer_tool.h
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/components/feedback/anonymizer_tool_unittest.cc
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/components/feedback/system_logs/system_logs_fetcher.cc
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/components/feedback/system_logs/system_logs_fetcher.h
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/extensions/browser/api/feedback_private/feedback_private_api.cc
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/extensions/browser/api/feedback_private/feedback_private_api.h
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/extensions/browser/api/feedback_private/log_source_access_manager.cc
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/extensions/browser/api/feedback_private/log_source_access_manager.h
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/extensions/browser/api/feedback_private/log_source_access_manager_chromeos_unittest.cc
[modify] https://crrev.com/f091addfc00ae9e2f82caa15ff5a5e498f654c35/extensions/shell/browser/system_logs/shell_system_logs_fetcher_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 13 2017

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

commit 67fa514817379c89494bdffee9f199b8ed8831e2
Author: Dominic Battre <battre@chromium.org>
Date: Wed Dec 13 19:47:12 2017

Migrate SysLogsSourceCallback to OnceCallbacks

base::Callback has been deprecated in favor of base::OnceCallback and
base::RepeatingCallback. In the case of SysLogsSourceCallback it turns
out that it is sufficient to always use base::OnceCallback.
For this reason, all base::Bind calls are replaced with base::BindOnce.
As base::OnceCallback does not have a copy-constructor to protect the
single-call semantics, the value is std::move()ed in a couple of places.
This also means that callbacks need to be passed by value instead
of const-reference. See docs/callback.md for more details.

TBR=rdevlin.cronin@chromium.org

Bug:  787554 , 714018
Change-Id: I557c4600b2d927d74345dceb6946b82b711c1cbc
Reviewed-on: https://chromium-review.googlesource.com/808706
Commit-Queue: Dominic Battré <battre@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Simon Que <sque@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523848}
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/command_line_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/command_line_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/dbus_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/dbus_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/debug_daemon_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/debug_daemon_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/device_event_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/device_event_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/lsb_release_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/lsb_release_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/single_debug_daemon_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/single_debug_daemon_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/single_debug_daemon_log_source_unittest.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/single_log_file_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/single_log_file_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/touch_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/chromeos/system_logs/touch_log_source_ozone.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/feedback/system_logs/log_sources/crash_ids_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/feedback/system_logs/log_sources/crash_ids_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/components/feedback/system_logs/system_logs_fetcher.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/components/feedback/system_logs/system_logs_fetcher.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/components/feedback/system_logs/system_logs_source.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/extensions/browser/api/feedback_private/feedback_private_api_unittest_base_chromeos.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/extensions/browser/api/feedback_private/log_source_access_manager.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/extensions/browser/api/feedback_private/log_source_access_manager.h
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/extensions/browser/api/feedback_private/log_source_access_manager_chromeos_unittest.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/extensions/shell/browser/system_logs/log_sources/basic_log_source.cc
[modify] https://crrev.com/67fa514817379c89494bdffee9f199b8ed8831e2/extensions/shell/browser/system_logs/log_sources/basic_log_source.h

Sign in to add a comment