Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2520833003:
Create ExtensionSystemImpl::quota_service_ before loading any extension (Closed)
DescriptionCreate ExtensionSystemImpl::quota_service_ before loading any extension
There is a possibility that during Init(), ExtensionFunctionDispatcher
tries to access |quota_service_|. This can happen in chromeos as the bug
suggests: ExtensionService::Init() loads component extensions and then
non-component extensions. If one of those non-component extensions (easy
to repro with --load-extension flag) is showing a modal error dialog
via ReportExtensionLoadError, extension APIs from the component
extension can arrive in ExtensionFunctionDispatcher while the modal
dialog is being shown. This can lead to accessing
ExtensionSystemImpl::quota_service_ before it is initialized.
This CL avoid that by initializing quota_service_ before loading any
extension.
BUG=642941
Committed: https://crrev.com/a885afcca33e88c96d4070dc885f26315bdd4c2a
Cr-Commit-Position: refs/heads/master@{#434009}
Patch Set 1 #Patch Set 2 : ok #
Total comments: 2
Patch Set 3 : move QuotaService init before loading any extension #Messages
Total messages: 21 (14 generated)
lazyboy@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
I spent few hours trying to write a test but couldn't find a way that works, on chromeos it seems there are two Profile-s being created during test run and I'm not seeing the component extension I'm trying to load via --load-component-extension flag is ending up in the right profile. I gave up as the CL seems small enough ;) or maybe not?
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2520833003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2520833003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:278: // ExtensionService::Init() might finish loading a component extension before Could we shorten this to simply: // Extension API calls require the QuotaService, so create it before // loading any extensions. ? On that note, I also wonder if we should move this instantiation above loading component extensions. In reality, it *probably* doesn't matter much, since any API call from the renderer will be after we finish this (and component extensions don't report load errors modally), but that seems like a fragile guarantee. And the QuotaService is nearly free to construct. WDYT?
Description was changed from ========== Create ExtensionSystemImpl::quota_service_ before ExtensionService::Init() There is a possibility that during Init(), ExtensionFunctionDispatcher tries to access |quota_service_|. This can happen in chromeos as the bug suggests: ExtensionService::Init() loads component extensions and then non-component extensions. If one of those non-component extensions (easy to repro with --load-extension flag) is showing a modal error dialog via ReportExtensionLoadError, extension APIs from the component extension can arrive in ExtensionFunctionDispatcher while the modal dialog is being shown. This can lead to accessing ExtensionSystemImpl::quota_service_ before it is initialized. This CL avoid that by initializing quota_service_ before calling ExtensionService::Init(). BUG=642941 ========== to ========== Create ExtensionSystemImpl::quota_service_ before loading any extension There is a possibility that during Init(), ExtensionFunctionDispatcher tries to access |quota_service_|. This can happen in chromeos as the bug suggests: ExtensionService::Init() loads component extensions and then non-component extensions. If one of those non-component extensions (easy to repro with --load-extension flag) is showing a modal error dialog via ReportExtensionLoadError, extension APIs from the component extension can arrive in ExtensionFunctionDispatcher while the modal dialog is being shown. This can lead to accessing ExtensionSystemImpl::quota_service_ before it is initialized. This CL avoid that by initializing quota_service_ before loading any extension. BUG=642941 ==========
https://codereview.chromium.org/2520833003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2520833003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_system_impl.cc:278: // ExtensionService::Init() might finish loading a component extension before On 2016/11/22 16:02:20, Devlin wrote: > Could we shorten this to simply: > // Extension API calls require the QuotaService, so create it before > // loading any extensions. > ? Done. > > On that note, I also wonder if we should move this instantiation above loading > component extensions. In reality, it *probably* doesn't matter much, since any > API call from the renderer will be after we finish this (and component > extensions don't report load errors modally), but that seems like a fragile > guarantee. And the QuotaService is nearly free to construct. WDYT? Sounds good, done.
lgtm
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lazyboy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479853386161360,
"parent_rev": "dffb29a91af14971fab116ad71b910855fdb5062", "commit_rev":
"40bf44e173985e7f531c6435ce4f642483feb6a2"}
Message was sent while issue was closed.
Description was changed from ========== Create ExtensionSystemImpl::quota_service_ before loading any extension There is a possibility that during Init(), ExtensionFunctionDispatcher tries to access |quota_service_|. This can happen in chromeos as the bug suggests: ExtensionService::Init() loads component extensions and then non-component extensions. If one of those non-component extensions (easy to repro with --load-extension flag) is showing a modal error dialog via ReportExtensionLoadError, extension APIs from the component extension can arrive in ExtensionFunctionDispatcher while the modal dialog is being shown. This can lead to accessing ExtensionSystemImpl::quota_service_ before it is initialized. This CL avoid that by initializing quota_service_ before loading any extension. BUG=642941 ========== to ========== Create ExtensionSystemImpl::quota_service_ before loading any extension There is a possibility that during Init(), ExtensionFunctionDispatcher tries to access |quota_service_|. This can happen in chromeos as the bug suggests: ExtensionService::Init() loads component extensions and then non-component extensions. If one of those non-component extensions (easy to repro with --load-extension flag) is showing a modal error dialog via ReportExtensionLoadError, extension APIs from the component extension can arrive in ExtensionFunctionDispatcher while the modal dialog is being shown. This can lead to accessing ExtensionSystemImpl::quota_service_ before it is initialized. This CL avoid that by initializing quota_service_ before loading any extension. BUG=642941 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Create ExtensionSystemImpl::quota_service_ before loading any extension There is a possibility that during Init(), ExtensionFunctionDispatcher tries to access |quota_service_|. This can happen in chromeos as the bug suggests: ExtensionService::Init() loads component extensions and then non-component extensions. If one of those non-component extensions (easy to repro with --load-extension flag) is showing a modal error dialog via ReportExtensionLoadError, extension APIs from the component extension can arrive in ExtensionFunctionDispatcher while the modal dialog is being shown. This can lead to accessing ExtensionSystemImpl::quota_service_ before it is initialized. This CL avoid that by initializing quota_service_ before loading any extension. BUG=642941 ========== to ========== Create ExtensionSystemImpl::quota_service_ before loading any extension There is a possibility that during Init(), ExtensionFunctionDispatcher tries to access |quota_service_|. This can happen in chromeos as the bug suggests: ExtensionService::Init() loads component extensions and then non-component extensions. If one of those non-component extensions (easy to repro with --load-extension flag) is showing a modal error dialog via ReportExtensionLoadError, extension APIs from the component extension can arrive in ExtensionFunctionDispatcher while the modal dialog is being shown. This can lead to accessing ExtensionSystemImpl::quota_service_ before it is initialized. This CL avoid that by initializing quota_service_ before loading any extension. BUG=642941 Committed: https://crrev.com/a885afcca33e88c96d4070dc885f26315bdd4c2a Cr-Commit-Position: refs/heads/master@{#434009} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a885afcca33e88c96d4070dc885f26315bdd4c2a Cr-Commit-Position: refs/heads/master@{#434009} |
