New issue
Advanced search Search tips

Issue 751882 link

Starred by 1 user
Status: Assigned
Owner:
lazyboy@chromium.org
Cc:
rdevlin....@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

simple_feature.cc's g_whitelisted_extension_id get/set has race

Project Member Reported by lazyboy@chromium.org, Aug 2 2017 
 crbug.com/750382  shows that different threads accessing the global can easily cause a race, because we are using a bare pointer.

However, I'm hesitant to just add a lock because it is accessed from SimpleFeature::GetManifestAvailability(), hence it might cause slowness. The value of this is loaded from kWhitelistedExtensionID switch.
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 5 2017 

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

commit b22f239691286a33960ca46a4d346c12878dab06
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Sat Aug 05 03:34:48 2017

[SimpleFeature] Fix a race in whitelisted extension id global init.

Using bare pointer to store an extension_id isn't thread-safe, and
can lead to data races. A TSAN repro was found while moving
extension installer code to Sequences
(https://chromium-review.googlesource.com/c/585386,  crbug.com/750382 )

Instead, use a LazyInstance to do the same, LazyInstance creation is
thread-safe.

However, ScopedWhitelistForTest is not thread-safe as it assigns
extension_id. This is not a big deal as it is invoked explicity from
test code. Rename the class to ScopedThreadUnsafeWhitelistForTest
to hint that.

Bug: 751882
Bug:  750382 
Change-Id: I5d2301004e5ff295cb3bd05b4c164262c8caf99b
Reviewed-on: https://chromium-review.googlesource.com/599240
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492226}
[modify] https://crrev.com/b22f239691286a33960ca46a4d346c12878dab06/chrome/browser/extensions/permission_message_combinations_unittest.cc
[modify] https://crrev.com/b22f239691286a33960ca46a4d346c12878dab06/chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc
[modify] https://crrev.com/b22f239691286a33960ca46a4d346c12878dab06/chrome/common/extensions/manifest_tests/extension_manifests_launcher_page_unittest.cc
[modify] https://crrev.com/b22f239691286a33960ca46a4d346c12878dab06/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
[modify] https://crrev.com/b22f239691286a33960ca46a4d346c12878dab06/chrome/common/extensions/sync_type_unittest.cc
[modify] https://crrev.com/b22f239691286a33960ca46a4d346c12878dab06/extensions/common/features/simple_feature.cc
[modify] https://crrev.com/b22f239691286a33960ca46a4d346c12878dab06/extensions/common/features/simple_feature.h
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 9 2017 

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

commit 81ff01ee027beef0b387aa9dae51b9a4c11540a8
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Wed Aug 09 16:00:59 2017

[Reland] [TaskScheduler] Migrate some extension installer code to TaskScheduler.

The original race was due to an orthogal issue that was fixed
in https://chromium-review.googlesource.com/c/599240

Use ExtensionFileTaskRunner to make sure there are no possibilities
around accessing installing extensions' resources. Unfortunately,
this also means we no longer run in USER_BLOCKING priority for these
tasks. ExtensionFileTaskRunner runs in USER_VISIBLE priority.


Bug:  750382 
Bug: 751882
Change-Id: Ia85904d81949e6194827477199243ab5b56ccb6f
Reviewed-on: https://chromium-review.googlesource.com/607534
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493004}
[modify] https://crrev.com/81ff01ee027beef0b387aa9dae51b9a4c11540a8/chrome/browser/extensions/unpacked_installer.cc
[modify] https://crrev.com/81ff01ee027beef0b387aa9dae51b9a4c11540a8/chrome/browser/extensions/user_script_listener_unittest.cc
[modify] https://crrev.com/81ff01ee027beef0b387aa9dae51b9a4c11540a8/chrome/browser/extensions/webstore_installer.cc
[modify] https://crrev.com/81ff01ee027beef0b387aa9dae51b9a4c11540a8/chrome/browser/extensions/zipfile_installer.cc
[modify] https://crrev.com/81ff01ee027beef0b387aa9dae51b9a4c11540a8/chrome/browser/extensions/zipfile_installer.h
►

Sign in to add a comment