Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2766063003:
Extensions: Keep track of loaded extensions in RendererStartupHelper. (Closed)
|
Created:
3 years, 9 months ago by karandeepb Modified:
3 years, 8 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtensions: Keep track of loaded extensions in RendererStartupHelper.
This CL:
- Adds tracking of currently loaded extensions and the processes they are loaded
in, to RendererStartupHelper. This helps to:
- Prevent sending redundant loading/unloading IPCs to renderer processes.
- Prevent activating an unloaded extension. This should also help ensure
that incorrect browser side logic is not causing issue 528026 and fix the
crashes if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
- Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED to take care of
cases where a RenderProcessHost is re-used for a different render process.
- Changes ShellExtensionSystem::LoadApp to notify the RendererStartupHelper when
an extension is loaded.
- Adds multiple checks to the RendererStartupHelper to ensure consistent state.
- Adds multiple tests for RendererStartupHelper.
BUG=527548, 528026
Review-Url: https://codereview.chromium.org/2766063003
Cr-Commit-Position: refs/heads/master@{#462326}
Committed: https://chromium.googlesource.com/chromium/src/+/53c8920dfe5c6e5798b617454b318692250e9134
Patch Set 1 : -- #
Total comments: 5
Patch Set 2 : -- #
Total comments: 18
Patch Set 3 : Address review. #Patch Set 4 : Fix tests. #Patch Set 5 : Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED. #Patch Set 6 : Fix more tests.. #
Total comments: 11
Patch Set 7 : Address review #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 55 (45 generated)
The CQ bit was checked by karandeepb@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 checked by karandeepb@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: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from
==========
Extensions: Better loading/unloading mgmt.
==========
to
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL adds tracking of currently loaded extensions corresponding to each
initialized RenderProcessHost in RendererStartupHelper. The
initialized_processes_ set is removed since it's redundant now. Tracking the
currently loaded extensions has the following benefits:
- Prevent loading an extension twice.
- Prevent unloading an unloaded extension.
- Prevent activating an unloaded extension. This should also help ensure that
incorrect browser side logic is not causing issue 528026 and fix the crashes
if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
BUG=527548, 528026
==========
Patchset #1 (id:60001) has been deleted
Description was changed from
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL adds tracking of currently loaded extensions corresponding to each
initialized RenderProcessHost in RendererStartupHelper. The
initialized_processes_ set is removed since it's redundant now. Tracking the
currently loaded extensions has the following benefits:
- Prevent loading an extension twice.
- Prevent unloading an unloaded extension.
- Prevent activating an unloaded extension. This should also help ensure that
incorrect browser side logic is not causing issue 528026 and fix the crashes
if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
BUG=527548, 528026
==========
to
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL adds tracking of currently loaded extensions corresponding to each
initialized RenderProcessHost in RendererStartupHelper. The
initialized_processes_ set is removed since it's redundant now. Tracking the
currently loaded extensions has the following benefits:
- Prevent loading an extension twice.
- Prevent unloading an unloaded extension.
- Prevent activating an unloaded extension. This should also help ensure that
incorrect browser side logic is not causing issue 528026 and fix the crashes
if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
A testing harness for RendererStartupHelper is also added.
BUG=527548, 528026
==========
karandeepb@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
PTAL Devlin. This is a prerequisite for https://codereview.chromium.org/2766263003/. https://codereview.chromium.org/2766063003/diff/80001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766063003/diff/80001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:151: NOTREACHED() << "Extension " << extension.id() We crash in the renderer in this scenario. Is that to prevent against a compromised renderer? https://codereview.chromium.org/2766063003/diff/80001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:176: if (base::ContainsKey(process_extensions_pair.second, extension.id())) This would have been a NOTREACHED in the renderer, but I think this is fine since these are public functions. We shouldn't assume that clients won't call these functions in an incorrect manner.
The CQ bit was checked by karandeepb@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/2766063003/diff/80001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766063003/diff/80001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:151: NOTREACHED() << "Extension " << extension.id() On 2017/03/24 23:54:12, karandeepb wrote: > We crash in the renderer in this scenario. Is that to prevent against a > compromised renderer? We crash because it's a logical state that we should never be in, and we're trying to track down and eliminate the scenarios in which it can happen. I'd encourage us to at least DumpWithoutCrashing here to help trace it. https://codereview.chromium.org/2766063003/diff/80001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:176: if (base::ContainsKey(process_extensions_pair.second, extension.id())) On 2017/03/24 23:54:12, karandeepb wrote: > This would have been a NOTREACHED in the renderer, but I think this is fine > since these are public functions. We shouldn't assume that clients won't call > these functions in an incorrect manner. Hmm... I'm not sure I agree. I think it's perfectly reasonable to assume that callers will only call functions in certain ways - that's why we can, e.g., DCHECK() that a pointer argument is non-null. I think that classes should be able to document and enforce expectations, including that function A is called before function B - which is basically what this is doing here. Especially when, if it's not the case, it could hide a potential bug. I agree that there are times when we want to be lenient in the API we expose, but we should balance that with the ability to create a deterministic state. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:95: std::set<ExtensionId>* loaded_extensions = &loaded_extensions_[process]; optional nit: maybe just prefer a reference here? std::set<ExtensionId>& loaded_extensions = loaded_extensions_[process]; https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:95: std::set<ExtensionId>* loaded_extensions = &loaded_extensions_[process]; Can we DCHECK(loaded_extensions.empty())? https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:147: const auto& process_extensions_pair = loaded_extensions_.find(process); This is an iterator, so having const auto& isn't really right, since the return result is a value, not a reference (though luckily the C++ ensures this is safe). Prefer just auto iter = loaded_extensions_.find(process); https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:155: process->Send(new ExtensionMsg_ActivateExtension(extension.id())); Don't we need to add the extension to the set? if (!iter->second.insert(extension.id()).second) { // Handle error } process->Send(new ExtensionMsg...); https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:175: // If extension is already loaded in process, do nothing. Hmm... can this happen, assuming OnExtensionLoaded/Unloaded are called properly? https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... File extensions/browser/renderer_startup_helper_unittest.cc (right): https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:19: namespace test { We don't frequently use namespace test {} in extensions unless it's designed to be a shared class (and even then, relatively rarely). Any particular reason for doing it here? https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:140: EXPECT_EQ(1u, sink.message_count()); The checks for count may as well be ASSERTs since you'll crash if it's 0. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:208: TEST_F(RendererStartupHelperTest, UnloadBeforeLoad) { As mentioned in the other comment, I'd prefer that we have a stricter contract here, since many of these would be indicative of a bug we need to fix.
https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... File extensions/browser/renderer_startup_helper_unittest.cc (right): https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:208: TEST_F(RendererStartupHelperTest, UnloadBeforeLoad) { On 2017/03/27 14:47:58, Devlin wrote: > As mentioned in the other comment, I'd prefer that we have a stricter contract > here, since many of these would be indicative of a bug we need to fix. So one of the reasons of doing this was that with https://codereview.chromium.org/2766263003/, I was hitting a NOTREACHED. If we enabled an extension for incognito(which was enabled, but disabled for incognito), the extension prefs would be changed to reflect that the extension was enabled for incognito and then the extension would be reloaded. During reloading, the extension would be unloaded from the incognito renderer process (since it's now enabled in incognito) which would cause the NOTEACHED, since it wasn't loaded into it earlier. I guess the right thing to do there then is to change the enable incognito code to: -Disable the extension. - Enable it for incognito. - Enable the extension?
https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... File extensions/browser/renderer_startup_helper_unittest.cc (right): https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:208: TEST_F(RendererStartupHelperTest, UnloadBeforeLoad) { On 2017/03/27 20:54:11, karandeepb wrote: > On 2017/03/27 14:47:58, Devlin wrote: > > As mentioned in the other comment, I'd prefer that we have a stricter contract > > here, since many of these would be indicative of a bug we need to fix. > > So one of the reasons of doing this was that with > https://codereview.chromium.org/2766263003/, I was hitting a NOTREACHED. If we > enabled an extension for incognito(which was enabled, but disabled for > incognito), the extension prefs would be changed to reflect that the extension > was enabled for incognito and then the extension would be reloaded. During > reloading, the extension would be unloaded from the incognito renderer process > (since it's now enabled in incognito) which would cause the NOTEACHED, since it > wasn't loaded into it earlier. > > I guess the right thing to do there then is to change the enable incognito code > to: > -Disable the extension. > - Enable it for incognito. > - Enable the extension? Doing this reordering would actually be non-trivial, since currently the unload-and-disable-for-reload and reload process are fairly intertwined, and I'm not sure we'd want to expose certain elements (e.g. disabling for DISABLE_REASON_RELOAD) outside of their current scope. That said, this scenario sounds like something that should be caught by the check in renderer_startup_helper.cc where we don't dispatch an unload IPC if the extension was never unloaded in the process. That aspect seems reasonable, since the contract with consumers of calling ExtensionLoaded before ExtensionUnloaded wasn't violated; instead it was just that the extension wasn't loaded in a given process. So I think what we should do here is: - Make sure that we have strict checks in RendererStartupHelper for when we expect loaded/unloaded/etc to be called, and enforce them - Allow RendererStartupHelper to maintain its state and determine whether or not to dispatch appropriate IPCs - Update this test to follow the possible scenario you described. SGTY?
On 2017/03/28 23:10:08, Devlin wrote: > https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... > File extensions/browser/renderer_startup_helper_unittest.cc (right): > > https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... > extensions/browser/renderer_startup_helper_unittest.cc:208: > TEST_F(RendererStartupHelperTest, UnloadBeforeLoad) { > On 2017/03/27 20:54:11, karandeepb wrote: > > On 2017/03/27 14:47:58, Devlin wrote: > > > As mentioned in the other comment, I'd prefer that we have a stricter > contract > > > here, since many of these would be indicative of a bug we need to fix. > > > > So one of the reasons of doing this was that with > > https://codereview.chromium.org/2766263003/, I was hitting a NOTREACHED. If we > > enabled an extension for incognito(which was enabled, but disabled for > > incognito), the extension prefs would be changed to reflect that the extension > > was enabled for incognito and then the extension would be reloaded. During > > reloading, the extension would be unloaded from the incognito renderer process > > (since it's now enabled in incognito) which would cause the NOTEACHED, since > it > > wasn't loaded into it earlier. > > > > I guess the right thing to do there then is to change the enable incognito > code > > to: > > -Disable the extension. > > - Enable it for incognito. > > - Enable the extension? > > Doing this reordering would actually be non-trivial, since currently the > unload-and-disable-for-reload and reload process are fairly intertwined, and I'm > not sure we'd want to expose certain elements (e.g. disabling for > DISABLE_REASON_RELOAD) outside of their current scope. > > That said, this scenario sounds like something that should be caught by the > check in renderer_startup_helper.cc where we don't dispatch an unload IPC if the > extension was never unloaded in the process. That aspect seems reasonable, > since the contract with consumers of calling ExtensionLoaded before > ExtensionUnloaded wasn't violated; instead it was just that the extension wasn't > loaded in a given process. > > So I think what we should do here is: > - Make sure that we have strict checks in RendererStartupHelper for when we > expect loaded/unloaded/etc to be called, and enforce them > - Allow RendererStartupHelper to maintain its state and determine whether or not > to dispatch appropriate IPCs > - Update this test to follow the possible scenario you described. > > SGTY? SGTM. This might require additional book-keeping though. (In this patch, I was maintaining a map of initialized processes->extensions loaded in the process). We'll now also want to know whether a extension has been loaded or not to enforce the contract.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Patchset #3 (id:120001) has been deleted
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by karandeepb@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@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.
PTAL Devlin. The code has changed, so you might be better off not reviewing the diff. Also, I've added some strict checks, let me know if any of these seem unreasonable. https://codereview.chromium.org/2766063003/diff/80001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766063003/diff/80001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:151: NOTREACHED() << "Extension " << extension.id() On 2017/03/27 14:47:57, Devlin wrote: > On 2017/03/24 23:54:12, karandeepb wrote: > > We crash in the renderer in this scenario. Is that to prevent against a > > compromised renderer? > > We crash because it's a logical state that we should never be in, and we're > trying to track down and eliminate the scenarios in which it can happen. I'd > encourage us to at least DumpWithoutCrashing here to help trace it. Done. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:95: std::set<ExtensionId>* loaded_extensions = &loaded_extensions_[process]; On 2017/03/27 14:47:58, Devlin wrote: > Can we DCHECK(loaded_extensions.empty())? Not relevant anymore. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:95: std::set<ExtensionId>* loaded_extensions = &loaded_extensions_[process]; On 2017/03/27 14:47:57, Devlin wrote: > optional nit: maybe just prefer a reference here? > std::set<ExtensionId>& loaded_extensions = loaded_extensions_[process]; Done. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:147: const auto& process_extensions_pair = loaded_extensions_.find(process); On 2017/03/27 14:47:57, Devlin wrote: > This is an iterator, so having const auto& isn't really right, since the return > result is a value, not a reference (though luckily the C++ ensures this is > safe). Prefer just > auto iter = loaded_extensions_.find(process); Oh yeah, this takes reference to a temporary. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:155: process->Send(new ExtensionMsg_ActivateExtension(extension.id())); On 2017/03/27 14:47:57, Devlin wrote: > Don't we need to add the extension to the set? > > if (!iter->second.insert(extension.id()).second) { > // Handle error > } > process->Send(new ExtensionMsg...); Not sure I understand. It is being activated. The set tracks loaded extensions. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:175: // If extension is already loaded in process, do nothing. On 2017/03/27 14:47:57, Devlin wrote: > Hmm... can this happen, assuming OnExtensionLoaded/Unloaded are called properly? For the incognito case I mentioned, yes. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... File extensions/browser/renderer_startup_helper_unittest.cc (right): https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:19: namespace test { On 2017/03/27 14:47:58, Devlin wrote: > We don't frequently use namespace test {} in extensions unless it's designed to > be a shared class (and even then, relatively rarely). Any particular reason for > doing it here? Removed. Reason was that it couldn't be in an anon namespace (due to friend declaration) and I didn't want to put test code in the extensions namespace. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:140: EXPECT_EQ(1u, sink.message_count()); On 2017/03/27 14:47:58, Devlin wrote: > The checks for count may as well be ASSERTs since you'll crash if it's 0. Done. https://codereview.chromium.org/2766063003/diff/100001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:208: TEST_F(RendererStartupHelperTest, UnloadBeforeLoad) { On 2017/03/28 23:10:08, Devlin wrote: > On 2017/03/27 20:54:11, karandeepb wrote: > > On 2017/03/27 14:47:58, Devlin wrote: > > > As mentioned in the other comment, I'd prefer that we have a stricter > contract > > > here, since many of these would be indicative of a bug we need to fix. > > > > So one of the reasons of doing this was that with > > https://codereview.chromium.org/2766263003/, I was hitting a NOTREACHED. If we > > enabled an extension for incognito(which was enabled, but disabled for > > incognito), the extension prefs would be changed to reflect that the extension > > was enabled for incognito and then the extension would be reloaded. During > > reloading, the extension would be unloaded from the incognito renderer process > > (since it's now enabled in incognito) which would cause the NOTEACHED, since > it > > wasn't loaded into it earlier. > > > > I guess the right thing to do there then is to change the enable incognito > code > > to: > > -Disable the extension. > > - Enable it for incognito. > > - Enable the extension? > > Doing this reordering would actually be non-trivial, since currently the > unload-and-disable-for-reload and reload process are fairly intertwined, and I'm > not sure we'd want to expose certain elements (e.g. disabling for > DISABLE_REASON_RELOAD) outside of their current scope. > > That said, this scenario sounds like something that should be caught by the > check in renderer_startup_helper.cc where we don't dispatch an unload IPC if the > extension was never unloaded in the process. That aspect seems reasonable, > since the contract with consumers of calling ExtensionLoaded before > ExtensionUnloaded wasn't violated; instead it was just that the extension wasn't > loaded in a given process. > > So I think what we should do here is: > - Make sure that we have strict checks in RendererStartupHelper for when we > expect loaded/unloaded/etc to be called, and enforce them > - Allow RendererStartupHelper to maintain its state and determine whether or not > to dispatch appropriate IPCs > - Update this test to follow the possible scenario you described. > > SGTY? Done. https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:54: case content::NOTIFICATION_RENDERER_PROCESS_CLOSED: So there were cases where InitializeProcess was being called twice for a RenderProcessHost. I found that this occurred when the RenderProcessHost was reused for a different render process. (See test failures from PS4->PS5). Observing NOTIFICATION_RENDERER_PROCESS_CLOSED takes care of this case. https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:106: // OnLoadedExtension should have already been called for the extension. This was not true for extensions loaded through ShellExtensionSystem, hence the change for it. https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:212: if (!base::ContainsKey(extension_process_map_, extension.id())) I was DCHECKing this (and the corresponding check in OnExtensionLoaded) earlier, but this caused some tests to fail. The code in ExtensionService doesn't always ensure that NotifyExtensionUnloaded is not called for an already unloaded extension. E.g., See ExtensionService::BlockAllExtensions (https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_serv...) which'll call UnloadExtension for a disabled extension(which is already unloaded). There are also some other cases I think. If you think this is a bug, I can handle this in a subsequent CL.
Description was changed from
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL adds tracking of currently loaded extensions corresponding to each
initialized RenderProcessHost in RendererStartupHelper. The
initialized_processes_ set is removed since it's redundant now. Tracking the
currently loaded extensions has the following benefits:
- Prevent loading an extension twice.
- Prevent unloading an unloaded extension.
- Prevent activating an unloaded extension. This should also help ensure that
incorrect browser side logic is not causing issue 528026 and fix the crashes
if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
A testing harness for RendererStartupHelper is also added.
BUG=527548, 528026
==========
to
==========
This CL:
- Adds tracking of currently loaded extensions and the processes they are loaded
in, to RendererStartupHelper. This helps to:
- Prevent sending redundant loading/unloading IPCs to renderer processes.
- Prevent activating an unloaded extension. This should also help ensure
that incorrect browser side logic is not causing issue 528026 and fix the
crashes if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
- Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED to take care of
cases where a RenderProcessHost is re-used for a different render process.
- Adds multiple checks to the RendererStartupHelper to ensure consistent state.
- Adds a test harness for RendererStartupHelper.
BUG=527548, 528026
==========
Description was changed from
==========
This CL:
- Adds tracking of currently loaded extensions and the processes they are loaded
in, to RendererStartupHelper. This helps to:
- Prevent sending redundant loading/unloading IPCs to renderer processes.
- Prevent activating an unloaded extension. This should also help ensure
that incorrect browser side logic is not causing issue 528026 and fix the
crashes if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
- Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED to take care of
cases where a RenderProcessHost is re-used for a different render process.
- Adds multiple checks to the RendererStartupHelper to ensure consistent state.
- Adds a test harness for RendererStartupHelper.
BUG=527548, 528026
==========
to
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL:
- Adds tracking of currently loaded extensions and the processes they are loaded
in, to RendererStartupHelper. This helps to:
- Prevent sending redundant loading/unloading IPCs to renderer processes.
- Prevent activating an unloaded extension. This should also help ensure
that incorrect browser side logic is not causing issue 528026 and fix the
crashes if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
- Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED to take care of
cases where a RenderProcessHost is re-used for a different render process.
- Adds multiple checks to the RendererStartupHelper to ensure consistent state.
- Adds multiple tests for RendererStartupHelper.
BUG=527548, 528026
==========
lgtm % nits https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:54: case content::NOTIFICATION_RENDERER_PROCESS_CLOSED: On 2017/04/04 00:49:42, karandeepb wrote: > So there were cases where InitializeProcess was being called twice for a > RenderProcessHost. I found that this occurred when the RenderProcessHost was > reused for a different render process. (See test failures from PS4->PS5). > Observing NOTIFICATION_RENDERER_PROCESS_CLOSED takes care of this case. Wild! Good find. We should add a comment for this. https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:161: char extension_id_copy[33]; Do you think the extension id help us debug the crash? If so, then this is fine, but otherwise we should avoid collecting it. https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:212: if (!base::ContainsKey(extension_process_map_, extension.id())) On 2017/04/04 00:49:42, karandeepb wrote: > I was DCHECKing this (and the corresponding check in OnExtensionLoaded) earlier, > but this caused some tests to fail. The code in ExtensionService doesn't always > ensure that NotifyExtensionUnloaded is not called for an already unloaded > extension. E.g., See ExtensionService::BlockAllExtensions > (https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_serv...) > which'll call UnloadExtension for a disabled extension(which is already > unloaded). There are also some other cases I think. If you think this is a bug, > I can handle this in a subsequent CL. I think this should be fixed. Mind filing a bug or a todo? https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... File extensions/browser/renderer_startup_helper_unittest.cc (right): https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:206: EXPECT_LE(2u, sink.message_count()); Why LE instead of EQ?
https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:54: case content::NOTIFICATION_RENDERER_PROCESS_CLOSED: On 2017/04/04 15:25:00, Devlin wrote: > On 2017/04/04 00:49:42, karandeepb wrote: > > So there were cases where InitializeProcess was being called twice for a > > RenderProcessHost. I found that this occurred when the RenderProcessHost was > > reused for a different render process. (See test failures from PS4->PS5). > > Observing NOTIFICATION_RENDERER_PROCESS_CLOSED takes care of this case. > > Wild! Good find. We should add a comment for this. Done. https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:161: char extension_id_copy[33]; On 2017/04/04 15:25:00, Devlin wrote: > Do you think the extension id help us debug the crash? If so, then this is > fine, but otherwise we should avoid collecting it. Removed. https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:212: if (!base::ContainsKey(extension_process_map_, extension.id())) On 2017/04/04 15:25:00, Devlin wrote: > On 2017/04/04 00:49:42, karandeepb wrote: > > I was DCHECKing this (and the corresponding check in OnExtensionLoaded) > earlier, > > but this caused some tests to fail. The code in ExtensionService doesn't > always > > ensure that NotifyExtensionUnloaded is not called for an already unloaded > > extension. E.g., See ExtensionService::BlockAllExtensions > > > (https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_serv...) > > which'll call UnloadExtension for a disabled extension(which is already > > unloaded). There are also some other cases I think. If you think this is a > bug, > > I can handle this in a subsequent CL. > > I think this should be fixed. Mind filing a bug or a todo? Done. https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... File extensions/browser/renderer_startup_helper_unittest.cc (right): https://codereview.chromium.org/2766063003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:206: EXPECT_LE(2u, sink.message_count()); On 2017/04/04 15:25:00, Devlin wrote: > Why LE instead of EQ? For other initialization messages. Added a comment.
Description was changed from
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL:
- Adds tracking of currently loaded extensions and the processes they are loaded
in, to RendererStartupHelper. This helps to:
- Prevent sending redundant loading/unloading IPCs to renderer processes.
- Prevent activating an unloaded extension. This should also help ensure
that incorrect browser side logic is not causing issue 528026 and fix the
crashes if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
- Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED to take care of
cases where a RenderProcessHost is re-used for a different render process.
- Adds multiple checks to the RendererStartupHelper to ensure consistent state.
- Adds multiple tests for RendererStartupHelper.
BUG=527548, 528026
==========
to
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL:
- Adds tracking of currently loaded extensions and the processes they are loaded
in, to RendererStartupHelper. This helps to:
- Prevent sending redundant loading/unloading IPCs to renderer processes.
- Prevent activating an unloaded extension. This should also help ensure
that incorrect browser side logic is not causing issue 528026 and fix the
crashes if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
- Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED to take care of
cases where a RenderProcessHost is re-used for a different render process.
- Changes ShellExtensionSystem::LoadApp to notify the RendererStartupHelper when
an extension is loaded.
- Adds multiple checks to the RendererStartupHelper to ensure consistent state.
- Adds multiple tests for RendererStartupHelper.
BUG=527548, 528026
==========
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2766063003/#ps220001 (title: "Address review")
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": 220001, "attempt_start_ts": 1491440026987240,
"parent_rev": "b5e134483c3d0fe3bcd4d2ed7390979d62d6abbb", "commit_rev":
"53c8920dfe5c6e5798b617454b318692250e9134"}
Message was sent while issue was closed.
Description was changed from
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL:
- Adds tracking of currently loaded extensions and the processes they are loaded
in, to RendererStartupHelper. This helps to:
- Prevent sending redundant loading/unloading IPCs to renderer processes.
- Prevent activating an unloaded extension. This should also help ensure
that incorrect browser side logic is not causing issue 528026 and fix the
crashes if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
- Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED to take care of
cases where a RenderProcessHost is re-used for a different render process.
- Changes ShellExtensionSystem::LoadApp to notify the RendererStartupHelper when
an extension is loaded.
- Adds multiple checks to the RendererStartupHelper to ensure consistent state.
- Adds multiple tests for RendererStartupHelper.
BUG=527548, 528026
==========
to
==========
Extensions: Keep track of loaded extensions in RendererStartupHelper.
This CL:
- Adds tracking of currently loaded extensions and the processes they are loaded
in, to RendererStartupHelper. This helps to:
- Prevent sending redundant loading/unloading IPCs to renderer processes.
- Prevent activating an unloaded extension. This should also help ensure
that incorrect browser side logic is not causing issue 528026 and fix the
crashes if it's the case.
- This is also needed for a subsequent CL which enters a NOTREACHED due to
unloading an unloaded extension.
- Also observe content::NOTIFICATION_RENDERER_PROCESS_CLOSED to take care of
cases where a RenderProcessHost is re-used for a different render process.
- Changes ShellExtensionSystem::LoadApp to notify the RendererStartupHelper when
an extension is loaded.
- Adds multiple checks to the RendererStartupHelper to ensure consistent state.
- Adds multiple tests for RendererStartupHelper.
BUG=527548, 528026
Review-Url: https://codereview.chromium.org/2766063003
Cr-Commit-Position: refs/heads/master@{#462326}
Committed:
https://chromium.googlesource.com/chromium/src/+/53c8920dfe5c6e5798b617454b31...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as https://chromium.googlesource.com/chromium/src/+/53c8920dfe5c6e5798b617454b31... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
