Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446543004:
Fix RPHI message queueing during process death (Closed)
|
Created:
4 years, 1 month ago by Ken Rockot(use gerrit already) Modified:
4 years, 1 month ago Reviewers:
jam CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix RPHI message queueing during process death
Please see https://crbug.com/658759 for more context.
The TL;DR is I broke some subtle behavior in
https://crrev.com/425358, and this fixes it. Here's what happens
in ProcessDied, in this order:
1. We reset channel_ so that future Send() calls throw away
messages. Note that at this point, any Channel-associated
interfaces remain valid but are intentionally dropping
all messages as well.
2. We fire various notifications about the death of the process.
3. We re-initialize channel_, allowing subsequent Send() calls
and associated interface calls to be queued and eventually
delivered to the new process once it's restarted.
The subtlety here is that if a RenderViewHostImpl is created at any
point during step (2) above, we must re-initialize channel_ and
start allowing messages to be queued on it. Formerly this was
accomplished by EnableSendQueue() setting is_initialized_ to
false. This CL establishes the same behavior by restoring
EnableSendQueue() to essentially have the same effect by re-
initializing the channel earlier.
This is likely not the precise behavior we want, since it seems like
this can cause messages intended for the old process to end up getting
delivered to the new process, but it is the behavior we already had in
place before https://crrev.com/425358 and we apparently rely on it.
BUG=658759
Committed: https://crrev.com/8a1482f49a54148bdb239261e7582555c8984663
Cr-Commit-Position: refs/heads/master@{#427257}
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #
Messages
Total messages: 24 (18 generated)
The CQ bit was checked by rockot@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...
Description was changed from ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in r425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initializes channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must to re-initialize channel_ and start allowing messages to be queued on it. This is likely not the precise behavior we *want*, but it is the behavior we already had in place before r425358. BUG=658759 ========== to ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in r425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must to re-initialize channel_ and start allowing messages to be queued on it. This is likely not the precise behavior we *want*, but it is the behavior we already had in place before r425358. BUG=658759 ==========
Description was changed from ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in r425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must to re-initialize channel_ and start allowing messages to be queued on it. This is likely not the precise behavior we *want*, but it is the behavior we already had in place before r425358. BUG=658759 ========== to ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in r425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it (formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false). This is likely not the precise behavior we *want*, but it is the behavior we already had in place before r425358. BUG=658759 ==========
Description was changed from ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in r425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it (formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false). This is likely not the precise behavior we *want*, but it is the behavior we already had in place before r425358. BUG=658759 ========== to ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it (formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false). This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358. BUG=658759 ==========
Description was changed from ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it (formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false). This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358. BUG=658759 ========== to ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it (formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false). This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358. BUG=658759 ==========
rockot@chromium.org changed reviewers: + jam@chromium.org
Description was changed from ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it (formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false). This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358. BUG=658759 ========== to ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it. Formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false. This CL establishes the same behavior by adding EnsureHasChannel(), which essentially has the same effect. This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358 and we apparently rely on it. BUG=658759 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
lgtm maybe keep old name of EnableSendQueue for consistency (i.e. mostly for code archeology) https://codereview.chromium.org/2446543004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2446543004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.h:324: void ResetChannelProxy(); nit: document
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Description was changed from ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it. Formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false. This CL establishes the same behavior by adding EnsureHasChannel(), which essentially has the same effect. This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358 and we apparently rely on it. BUG=658759 ========== to ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it. Formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false. This CL establishes the same behavior by restoring EnableSendQueue() to essentially has the same effect by re- initializing the channel earlier. This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358 and we apparently rely on it. BUG=658759 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK, EnableSendQueue() it is. https://codereview.chromium.org/2446543004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2446543004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.h:324: void ResetChannelProxy(); On 2016/10/25 at 00:22:55, jam wrote: > nit: document Done
The CQ bit was unchecked by rockot@chromium.org
The CQ bit was checked by rockot@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org Link to the patchset: https://codereview.chromium.org/2446543004/#ps20001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it. Formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false. This CL establishes the same behavior by restoring EnableSendQueue() to essentially has the same effect by re- initializing the channel earlier. This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358 and we apparently rely on it. BUG=658759 ========== to ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it. Formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false. This CL establishes the same behavior by restoring EnableSendQueue() to essentially have the same effect by re- initializing the channel earlier. This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358 and we apparently rely on it. BUG=658759 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it. Formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false. This CL establishes the same behavior by restoring EnableSendQueue() to essentially have the same effect by re- initializing the channel earlier. This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358 and we apparently rely on it. BUG=658759 ========== to ========== Fix RPHI message queueing during process death Please see https://crbug.com/658759 for more context. The TL;DR is I broke some subtle behavior in https://crrev.com/425358, and this fixes it. Here's what happens in ProcessDied, in this order: 1. We reset channel_ so that future Send() calls throw away messages. Note that at this point, any Channel-associated interfaces remain valid but are intentionally dropping all messages as well. 2. We fire various notifications about the death of the process. 3. We re-initialize channel_, allowing subsequent Send() calls and associated interface calls to be queued and eventually delivered to the new process once it's restarted. The subtlety here is that if a RenderViewHostImpl is created at any point during step (2) above, we must re-initialize channel_ and start allowing messages to be queued on it. Formerly this was accomplished by EnableSendQueue() setting is_initialized_ to false. This CL establishes the same behavior by restoring EnableSendQueue() to essentially have the same effect by re- initializing the channel earlier. This is likely not the precise behavior we want, since it seems like this can cause messages intended for the old process to end up getting delivered to the new process, but it is the behavior we already had in place before https://crrev.com/425358 and we apparently rely on it. BUG=658759 Committed: https://crrev.com/8a1482f49a54148bdb239261e7582555c8984663 Cr-Commit-Position: refs/heads/master@{#427257} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8a1482f49a54148bdb239261e7582555c8984663 Cr-Commit-Position: refs/heads/master@{#427257} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
