Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2162983002:
[Extensions] Ensure ordering of extension [un]loaded, activated messages (Closed)
|
Created:
4 years, 5 months ago by Devlin Modified:
4 years, 4 months ago Reviewers:
lazyboy CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, asargent_no_longer_on_chrome Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions] Ensure ordering of extension [un]loaded, activated messages
We send three messages to the renderer in order to maintain extension
lifetime and active state: extension loaded, extension unloaded, and
extension activated. The loaded message is sent when a) an extension is
loaded and b) initially when the render process starts. Unloaded
messages are sent when an extension is unloaded to any running process.
Activated messages are sent to a renderer when a render view
corresponding to an extension is created.
Unfortunately, the existent render processes are not always initialized.
We have code to queue up messages until the render process is properly
initialized, which ensures that at least the loaded message with all
currently-loaded extensions is sent first, followed by queued messages
in FIFO. However, this breaks in the following scenario:
- Renderer created, not fully initialized (channel not created)
- Render view created, activate extension 'A' message sent. Since the
process is not initialized, this message is queued.
- Extension unloaded. Extension 'A' unloaded message is also queued.
- Renderer initialized. We initialize the renderer with all loaded
extensions, however, Extension 'A' is no longer loaded, so is
(rightly) included in the list.
- Extension renderer receives messages for loaded extensions, activate
extension 'A', unload extension 'A'. Since extension 'A' was never
loaded, the queued messages break assumptions (and cause a crash).
Fix this by passing all loaded, unloaded, and activated messages through
a common source that keeps track of initialized render processes, and
will not send messages until the process is initialized, as well as
cleaning up unnecessary messages (such as activating extensions that
have since been unloaded).
Also add a regression test.
BUG=528026
Committed: https://crrev.com/5e510e8037bb3cdf0bfb5cf07bbb7fe3c94450f9
Cr-Commit-Position: refs/heads/master@{#407796}
Patch Set 1 : Ready for review #
Total comments: 12
Patch Set 2 : Istiaque's #Messages
Total messages: 49 (42 generated)
The CQ bit was checked by rdevlin.cronin@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 rdevlin.cronin@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rdevlin.cronin@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rdevlin.cronin@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rdevlin.cronin@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: win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
The CQ bit was checked by rdevlin.cronin@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.
Description was changed from ========== randomtest BUG= ========== to ========== [Extensions] Ensure ordering of extension [un]loaded, activated messages We send three messages to the renderer in order to maintain extension lifetime and active state: extension loaded, extension unloaded, and extension activated. The loaded message is sent when a) an extension is loaded and b) initially when the render process starts. Unloaded messages are sent when an extension is unloaded to any running process. Activated messages are sent to a renderer when a render view corresponding to an extension is created. Unfortunately, the existent render processes are not always initialized. We have code to queue up messages until the render process is properly initialized, which ensures that at least the loaded message with all currently-loaded extensions is sent first, followed by queued messages in FIFO. However, this breaks in the following scenario: - Renderer created, not fully initialized (channel not created) - Render view created, activate extension 'A' message sent. Since the process is not initialized, this message is queued. - Extension unloaded. Extension 'A' unloaded message is also queued. - Renderer initialized. We initialize the renderer with all loaded extensions, however, Extension 'A' is no longer loaded, so is (rightly) included in the list. - Extension renderer receives messages for loaded extensions, activate extension 'A', unload extension 'A'. Since extension 'A' was never loaded, the queued messages break assumptions (and cause a crash). Fix this by passing all loaded, unloaded, and activated messages through a common source that keeps track of initialized render processes, and will not send messages until the process is initialized, as well as cleaning up unnecessary messages (such as activating extensions that have since been unloaded). BUG=528026 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #1 (id:1) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + lazyboy@chromium.org
Istiaque, mind taking a look? I think this is what's causing the OnActivateExtension crashes. +asargent@ in case he's curious, since we were all a bit boggled.
Description was changed from ========== [Extensions] Ensure ordering of extension [un]loaded, activated messages We send three messages to the renderer in order to maintain extension lifetime and active state: extension loaded, extension unloaded, and extension activated. The loaded message is sent when a) an extension is loaded and b) initially when the render process starts. Unloaded messages are sent when an extension is unloaded to any running process. Activated messages are sent to a renderer when a render view corresponding to an extension is created. Unfortunately, the existent render processes are not always initialized. We have code to queue up messages until the render process is properly initialized, which ensures that at least the loaded message with all currently-loaded extensions is sent first, followed by queued messages in FIFO. However, this breaks in the following scenario: - Renderer created, not fully initialized (channel not created) - Render view created, activate extension 'A' message sent. Since the process is not initialized, this message is queued. - Extension unloaded. Extension 'A' unloaded message is also queued. - Renderer initialized. We initialize the renderer with all loaded extensions, however, Extension 'A' is no longer loaded, so is (rightly) included in the list. - Extension renderer receives messages for loaded extensions, activate extension 'A', unload extension 'A'. Since extension 'A' was never loaded, the queued messages break assumptions (and cause a crash). Fix this by passing all loaded, unloaded, and activated messages through a common source that keeps track of initialized render processes, and will not send messages until the process is initialized, as well as cleaning up unnecessary messages (such as activating extensions that have since been unloaded). BUG=528026 ========== to ========== [Extensions] Ensure ordering of extension [un]loaded, activated messages We send three messages to the renderer in order to maintain extension lifetime and active state: extension loaded, extension unloaded, and extension activated. The loaded message is sent when a) an extension is loaded and b) initially when the render process starts. Unloaded messages are sent when an extension is unloaded to any running process. Activated messages are sent to a renderer when a render view corresponding to an extension is created. Unfortunately, the existent render processes are not always initialized. We have code to queue up messages until the render process is properly initialized, which ensures that at least the loaded message with all currently-loaded extensions is sent first, followed by queued messages in FIFO. However, this breaks in the following scenario: - Renderer created, not fully initialized (channel not created) - Render view created, activate extension 'A' message sent. Since the process is not initialized, this message is queued. - Extension unloaded. Extension 'A' unloaded message is also queued. - Renderer initialized. We initialize the renderer with all loaded extensions, however, Extension 'A' is no longer loaded, so is (rightly) included in the list. - Extension renderer receives messages for loaded extensions, activate extension 'A', unload extension 'A'. Since extension 'A' was never loaded, the queued messages break assumptions (and cause a crash). Fix this by passing all loaded, unloaded, and activated messages through a common source that keeps track of initialized render processes, and will not send messages until the process is initialized, as well as cleaning up unnecessary messages (such as activating extensions that have since been unloaded). Also add a regression test. BUG=528026 ==========
Few comments, nothing too major. https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:111: process->Send(new ExtensionMsg_ActivateExtension(id)); DCHECK(extensions.Contains(id)) or does that make sense here? https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:119: initialized_processes_.erase(process); Though not necessary, I think bailing out of this method early when process's context doesn't match |browser_context_| is a good idea. For one that is the correct thing to do. The other would be to avoid unnecessary erase calls below. https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:133: if (extension.is_theme()) Retain the old comment: // Renderers don't need to know about themes. https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:150: for (auto& kv : pending_active_extensions_) s/kv/extensions_in_process or sth. https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.h (right): https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.h:81: pending_active_extensions_; nit (fine to ignore): activation_pending_extensions_ or activate_pending_extensions_ https://codereview.chromium.org/2162983002/diff/160001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2162983002/diff/160001/extensions/renderer/di... extensions/renderer/dispatcher.cc:1098: DCHECK(!extension_registry->Contains(extension->id())); Change these to: if (!extension_registry->Insert(extension)) { // TODO.. This may be ... NOTREACHED(); }
The CQ bit was checked by rdevlin.cronin@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/2162983002/diff/160001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:111: process->Send(new ExtensionMsg_ActivateExtension(id)); On 2016/07/22 00:29:31, lazyboy wrote: > DCHECK(extensions.Contains(id)) or does that make sense here? Sure, done. https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:119: initialized_processes_.erase(process); On 2016/07/22 00:29:31, lazyboy wrote: > Though not necessary, I think bailing out of this method early when process's > context doesn't match |browser_context_| is a good idea. > For one that is the correct thing to do. The other would be to avoid unnecessary > erase calls below. Done. https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:133: if (extension.is_theme()) On 2016/07/22 00:29:31, lazyboy wrote: > Retain the old comment: > // Renderers don't need to know about themes. Done. https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:150: for (auto& kv : pending_active_extensions_) On 2016/07/22 00:29:31, lazyboy wrote: > s/kv/extensions_in_process or sth. replaced with process_extensions_pair (it's not extensions_in_process, since it contains the key as well). kv is my go to for map types for key-value, but I suppose this is more legible. https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.h (right): https://codereview.chromium.org/2162983002/diff/160001/extensions/browser/ren... extensions/browser/renderer_startup_helper.h:81: pending_active_extensions_; On 2016/07/22 00:29:31, lazyboy wrote: > nit (fine to ignore): activation_pending_extensions_ or > activate_pending_extensions_ I prefer pending_active_extensions, since activation_pending_extensions makes it sound like extensions that are waiting for activation (vs extensions that are active) and activate_pending_extensions_ sounds like a bool or flag. https://codereview.chromium.org/2162983002/diff/160001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2162983002/diff/160001/extensions/renderer/di... extensions/renderer/dispatcher.cc:1098: DCHECK(!extension_registry->Contains(extension->id())); On 2016/07/22 00:29:31, lazyboy wrote: > Change these to: > if (!extension_registry->Insert(extension)) { > // TODO.. This may be ... > NOTREACHED(); > } Done.
lgtm
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Ensure ordering of extension [un]loaded, activated messages We send three messages to the renderer in order to maintain extension lifetime and active state: extension loaded, extension unloaded, and extension activated. The loaded message is sent when a) an extension is loaded and b) initially when the render process starts. Unloaded messages are sent when an extension is unloaded to any running process. Activated messages are sent to a renderer when a render view corresponding to an extension is created. Unfortunately, the existent render processes are not always initialized. We have code to queue up messages until the render process is properly initialized, which ensures that at least the loaded message with all currently-loaded extensions is sent first, followed by queued messages in FIFO. However, this breaks in the following scenario: - Renderer created, not fully initialized (channel not created) - Render view created, activate extension 'A' message sent. Since the process is not initialized, this message is queued. - Extension unloaded. Extension 'A' unloaded message is also queued. - Renderer initialized. We initialize the renderer with all loaded extensions, however, Extension 'A' is no longer loaded, so is (rightly) included in the list. - Extension renderer receives messages for loaded extensions, activate extension 'A', unload extension 'A'. Since extension 'A' was never loaded, the queued messages break assumptions (and cause a crash). Fix this by passing all loaded, unloaded, and activated messages through a common source that keeps track of initialized render processes, and will not send messages until the process is initialized, as well as cleaning up unnecessary messages (such as activating extensions that have since been unloaded). Also add a regression test. BUG=528026 ========== to ========== [Extensions] Ensure ordering of extension [un]loaded, activated messages We send three messages to the renderer in order to maintain extension lifetime and active state: extension loaded, extension unloaded, and extension activated. The loaded message is sent when a) an extension is loaded and b) initially when the render process starts. Unloaded messages are sent when an extension is unloaded to any running process. Activated messages are sent to a renderer when a render view corresponding to an extension is created. Unfortunately, the existent render processes are not always initialized. We have code to queue up messages until the render process is properly initialized, which ensures that at least the loaded message with all currently-loaded extensions is sent first, followed by queued messages in FIFO. However, this breaks in the following scenario: - Renderer created, not fully initialized (channel not created) - Render view created, activate extension 'A' message sent. Since the process is not initialized, this message is queued. - Extension unloaded. Extension 'A' unloaded message is also queued. - Renderer initialized. We initialize the renderer with all loaded extensions, however, Extension 'A' is no longer loaded, so is (rightly) included in the list. - Extension renderer receives messages for loaded extensions, activate extension 'A', unload extension 'A'. Since extension 'A' was never loaded, the queued messages break assumptions (and cause a crash). Fix this by passing all loaded, unloaded, and activated messages through a common source that keeps track of initialized render processes, and will not send messages until the process is initialized, as well as cleaning up unnecessary messages (such as activating extensions that have since been unloaded). Also add a regression test. BUG=528026 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Ensure ordering of extension [un]loaded, activated messages We send three messages to the renderer in order to maintain extension lifetime and active state: extension loaded, extension unloaded, and extension activated. The loaded message is sent when a) an extension is loaded and b) initially when the render process starts. Unloaded messages are sent when an extension is unloaded to any running process. Activated messages are sent to a renderer when a render view corresponding to an extension is created. Unfortunately, the existent render processes are not always initialized. We have code to queue up messages until the render process is properly initialized, which ensures that at least the loaded message with all currently-loaded extensions is sent first, followed by queued messages in FIFO. However, this breaks in the following scenario: - Renderer created, not fully initialized (channel not created) - Render view created, activate extension 'A' message sent. Since the process is not initialized, this message is queued. - Extension unloaded. Extension 'A' unloaded message is also queued. - Renderer initialized. We initialize the renderer with all loaded extensions, however, Extension 'A' is no longer loaded, so is (rightly) included in the list. - Extension renderer receives messages for loaded extensions, activate extension 'A', unload extension 'A'. Since extension 'A' was never loaded, the queued messages break assumptions (and cause a crash). Fix this by passing all loaded, unloaded, and activated messages through a common source that keeps track of initialized render processes, and will not send messages until the process is initialized, as well as cleaning up unnecessary messages (such as activating extensions that have since been unloaded). Also add a regression test. BUG=528026 ========== to ========== [Extensions] Ensure ordering of extension [un]loaded, activated messages We send three messages to the renderer in order to maintain extension lifetime and active state: extension loaded, extension unloaded, and extension activated. The loaded message is sent when a) an extension is loaded and b) initially when the render process starts. Unloaded messages are sent when an extension is unloaded to any running process. Activated messages are sent to a renderer when a render view corresponding to an extension is created. Unfortunately, the existent render processes are not always initialized. We have code to queue up messages until the render process is properly initialized, which ensures that at least the loaded message with all currently-loaded extensions is sent first, followed by queued messages in FIFO. However, this breaks in the following scenario: - Renderer created, not fully initialized (channel not created) - Render view created, activate extension 'A' message sent. Since the process is not initialized, this message is queued. - Extension unloaded. Extension 'A' unloaded message is also queued. - Renderer initialized. We initialize the renderer with all loaded extensions, however, Extension 'A' is no longer loaded, so is (rightly) included in the list. - Extension renderer receives messages for loaded extensions, activate extension 'A', unload extension 'A'. Since extension 'A' was never loaded, the queued messages break assumptions (and cause a crash). Fix this by passing all loaded, unloaded, and activated messages through a common source that keeps track of initialized render processes, and will not send messages until the process is initialized, as well as cleaning up unnecessary messages (such as activating extensions that have since been unloaded). Also add a regression test. BUG=528026 Committed: https://crrev.com/5e510e8037bb3cdf0bfb5cf07bbb7fe3c94450f9 Cr-Commit-Position: refs/heads/master@{#407796} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5e510e8037bb3cdf0bfb5cf07bbb7fe3c94450f9 Cr-Commit-Position: refs/heads/master@{#407796} |
