Need a common extensions-related file task runner |
|||
Issue descriptionWith the task scheduler migration ( issue 689520 ), we are moving away from using the FILE thread. However, with this migration, we also lose guarantees that multiple systems aren't going to try and read the same file at the same time. For instance, if we try to read a js file from the user script loader and from extension protocols, they can collide. We need to have a common extensions-related file task runner (and ensure that tasks outside the extensions system reading extensions files also use it, if any such tasks exist). This is complicated slightly because there's no way (yet!) of either a) having a single task runner accept tasks with different TaskTraits or b) sharing a sequence between two task runners. This means that any tasks posted to this task runner will be run with the same priority. This is unfortunate, since some tasks (extension protocols, loading an unpacked extension/installing an extension) are very visible to the user, and should likely have a priority of USER_BLOCKING. Other tasks (like garbage collection) should have a much lower priority (likely BACKGROUND). For now, the best solution is probably to pick a reasonable default, and determine which tasks absolutely must be run in sequence, and eat the cost of not being able to customize priority. There are a few tasks that we might avoid putting on this shared runner for now, since they might not be subject to races. For instance, installation is *probably* safe to have on a separate runner, since before the extension is installed, nothing should be reading from it. Once we have the ability to share sequences or adjust priorities, it might be worth migrating these over as well, just to avoid any future races that might occur. I'll assign this to myself for now, but other people are encouraged to jump in. :) +gab@ as well, to check that the above makes sense and jives with the general task scheduler direction.
,
Jul 28 2017
Is it a problem if two different sequences read the same file simultaneously? Isn't it ok as long as one of them does not modify the file on disk?
,
Jul 28 2017
Also worth noting, we already have a SequencedTaskRunner provided by ExtensionService::GetFileTaskRunner. The coomment says it is for crx installations, but it seems it is used inconsistently for other file IO operations as well.
,
Jul 28 2017
> Is it a problem if two different sequences read the same file simultaneously? Isn't it ok as long as one of them does not modify the file on disk? I think that yes, as long as each task is only reading the file, and not modifying it, then there is no concern here (though we use OS-specific libraries for file reading, and I don't know them well enough to guarantee that's the case). However, *modifying* and reading a file is not safe, and we can't just have a task runner for modification and do reading on any runner, since then there's a chance that a read and modify happen at the same time. So we need to post read, modify, and delete tasks to the same task runner to ensure safety. > Also worth noting, we already have a SequencedTaskRunner provided by ExtensionService::GetFileTaskRunner. The coomment says it is for crx installations, but it seems it is used inconsistently for other file IO operations as well. Good find! We should definitely address that (likely by migrating usage of that to a common shared task runner - I don't think we need separate task runners for installation vs everything else).
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40b4cbc3db8c603c52af3a9b255ccf009672a38c commit 40b4cbc3db8c603c52af3a9b255ccf009672a38c Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Aug 02 01:57:59 2017 [Task Migration][Extensions] Add and use a devoted file task runner With the new task scheduling system, there are no guarantees that different operations will not collide - each caller has to ensure that no conflicting tasks will be scheduled and executed synchronously. This means that for file tasks, we need to have a common task runner for any tasks that may touch the same files. Introduce a common SequencedTaskRunner for the extension system, and use it in the UserScriptLoader. Additionally, use it in ExtensionService, in lieu of the prior ExtensionService-owned task runner that's used for file tasks in installation. Finally, also make the GetFileTaskRunner() method in ExtensionService non-virtual, since it's not needed on any of the interfaces. Bug: 689520 Bug: 750122 Change-Id: I9d0b993689821e7aca9b2553283609cce796a162 Reviewed-on: https://chromium-review.googlesource.com/593854 Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#491209} [modify] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/chrome/browser/extensions/extension_service.cc [modify] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/chrome/browser/extensions/extension_service.h [modify] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/chrome/browser/extensions/extension_user_script_loader_unittest.cc [modify] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/chrome/browser/extensions/test_extension_service.cc [modify] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/chrome/browser/extensions/test_extension_service.h [modify] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/extensions/browser/BUILD.gn [add] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/extensions/browser/extension_file_task_runner.cc [add] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/extensions/browser/extension_file_task_runner.h [modify] https://crrev.com/40b4cbc3db8c603c52af3a9b255ccf009672a38c/extensions/browser/extension_user_script_loader.cc
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ecda024e8866239e213929e212698ef4afdeed57 commit ecda024e8866239e213929e212698ef4afdeed57 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Aug 02 21:44:40 2017 [Task Migration][Extensions] Update echoPrivate Update the echoPrivate API to use the new task scheduling API, as well as the dedicated Extensions-related file task runner. Bug: 689520 Bug: 750122 Change-Id: Ie6d403cb700c71bc9faabd828b2f40f0826f85bd Reviewed-on: https://chromium-review.googlesource.com/598464 Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Karan Bhatia <karandeepb@chromium.org> Cr-Commit-Position: refs/heads/master@{#491515} [modify] https://crrev.com/ecda024e8866239e213929e212698ef4afdeed57/chrome/browser/chromeos/extensions/echo_private_api.cc [modify] https://crrev.com/ecda024e8866239e213929e212698ef4afdeed57/chrome/browser/chromeos/extensions/echo_private_api.h
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9a51c15301a1a31da511a5e9d05d5bb7f4a90e1 commit a9a51c15301a1a31da511a5e9d05d5bb7f4a90e1 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Aug 02 22:06:09 2017 [Task Migration][Extensions] Update ExtensionDownloader Update the ExtensionDownloader class to use the new task scheduling API, as well as the dedicated Extensions-related file task runner. Bug: 689520 Bug: 750122 Change-Id: I201adece7ae5d5f7a24f535faa86958d4fcff632 Reviewed-on: https://chromium-review.googlesource.com/594833 Reviewed-by: Karan Bhatia <karandeepb@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#491522} [modify] https://crrev.com/a9a51c15301a1a31da511a5e9d05d5bb7f4a90e1/extensions/browser/updater/extension_downloader.cc
,
Aug 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06ba081e89937dea18efa31eeecf8162dafd7434 commit 06ba081e89937dea18efa31eeecf8162dafd7434 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Thu Aug 03 00:23:33 2017 [Task Migration][Extensions] Update FileReader Update the FileReader class to use the new task scheduling API, as well as the dedicated Extensions-related file task runner. Bug: 689520 Bug: 750122 Change-Id: I3dd119e9c6bc368ab0186e599a4a67df02b872d6 Reviewed-on: https://chromium-review.googlesource.com/594010 Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Karan Bhatia <karandeepb@chromium.org> Cr-Commit-Position: refs/heads/master@{#491566} [modify] https://crrev.com/06ba081e89937dea18efa31eeecf8162dafd7434/extensions/browser/api/execute_code_function.cc [modify] https://crrev.com/06ba081e89937dea18efa31eeecf8162dafd7434/extensions/browser/file_reader.cc [modify] https://crrev.com/06ba081e89937dea18efa31eeecf8162dafd7434/extensions/browser/file_reader.h [modify] https://crrev.com/06ba081e89937dea18efa31eeecf8162dafd7434/extensions/browser/file_reader_unittest.cc
,
Aug 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ad3c52ba5b36c31eebdd884e34555733a199cf62 commit ad3c52ba5b36c31eebdd884e34555733a199cf62 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Thu Aug 10 01:49:37 2017 [Extensions][Task Scheduler] Remove ExtensionService::GetFileTaskRunner ExtensionService::GetFileTaskRunner() now just forwards to returning extensions::GetExtensionFileTaskRunner() (the devoted sequenced task runner for extension-related file tasks). Update callers to just use this instead to avoid confusion and simplify the code, as well as remove unneeded dependencies on ExtensionService. Bug: 750122 Change-Id: Ic0d6a6ddc087d211d7c9290287e2698b7898ea4a Reviewed-on: https://chromium-review.googlesource.com/598946 Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Reviewed-by: Toni Barzic <tbarzic@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#493218} [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/chromeos/lock_screen_apps/app_manager_impl.cc [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/chromeos/lock_screen_apps/app_manager_impl_unittest.cc [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/extensions/crx_installer.cc [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/extensions/extension_assets_manager_chromeos.cc [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/extensions/extension_assets_manager_chromeos.h [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/extensions/extension_garbage_collector.cc [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/extensions/extension_garbage_collector_chromeos.cc [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/extensions/extension_service.cc [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/extensions/extension_service.h [modify] https://crrev.com/ad3c52ba5b36c31eebdd884e34555733a199cf62/chrome/browser/themes/theme_service.cc
,
Aug 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d9cbc1f8ef96abc3401e83019f10374f38509733 commit d9cbc1f8ef96abc3401e83019f10374f38509733 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Tue Aug 15 19:24:51 2017 [Task Migration][Extensions] Add and use a devoted file task runner With the new task scheduling system, there are no guarantees that different operations will not collide - each caller has to ensure that no conflicting tasks will be scheduled and executed synchronously. This means that for file tasks, we need to have a common task runner for any tasks that may touch the same files. Introduce a common SequencedTaskRunner for the extension system, and use it in the UserScriptLoader. Additionally, use it in ExtensionService, in lieu of the prior ExtensionService-owned task runner that's used for file tasks in installation. Finally, also make the GetFileTaskRunner() method in ExtensionService non-virtual, since it's not needed on any of the interfaces. TBR=rdevlin.cronin@chromium.org (cherry picked from commit 40b4cbc3db8c603c52af3a9b255ccf009672a38c) Bug: 689520 Bug: 750122 Change-Id: I9d0b993689821e7aca9b2553283609cce796a162 Reviewed-on: https://chromium-review.googlesource.com/593854 Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#491209} Reviewed-on: https://chromium-review.googlesource.com/615880 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#577} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/chrome/browser/extensions/extension_service.cc [modify] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/chrome/browser/extensions/extension_service.h [modify] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/chrome/browser/extensions/extension_user_script_loader_unittest.cc [modify] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/chrome/browser/extensions/test_extension_service.cc [modify] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/chrome/browser/extensions/test_extension_service.h [modify] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/extensions/browser/BUILD.gn [add] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/extensions/browser/extension_file_task_runner.cc [add] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/extensions/browser/extension_file_task_runner.h [modify] https://crrev.com/d9cbc1f8ef96abc3401e83019f10374f38509733/extensions/browser/extension_user_script_loader.cc
,
Aug 15 2017
(Note: #10 was merged for crbug.com/746155)
,
Aug 18 2017
Marking this as fixed. Additional improvements (such as using this directly rather than currying it in to various classes) can be filed as separate bugs. |
|||
►
Sign in to add a comment |
|||
Comment 1 by gab@chromium.org
, Jul 28 2017