Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2601893002:
SharedWorker: Remove message forwarding mechanism (Closed)
DescriptionSharedWorker: Remove message forwarding mechanism
SharedWorker service has a mechanism to forward a message sent from a document
to a worker. With this mechanism, a sender creates a nested IPC message and
sends it to the SharedWorker service in the browser process. The service
interprets the nested message and forwards it to an appropriate receiver.
This mechanism is available for any IPC messages but it has been used only for
worker connection message. If we use it only for one message type, we don't have
to manage such a complex mechanism. Therefore, this CL removes the mechanism and
uses a plain IPC message for connecting to a worker.
This simplification also makes it easier to mojofy SharedWorker.
BUG=612308
Review-Url: https://codereview.chromium.org/2601893002
Cr-Commit-Position: refs/heads/master@{#441785}
Committed: https://chromium.googlesource.com/chromium/src/+/b46ff78092b42dfe9f3d816a4c16972217f9947a
Patch Set 1 #
Total comments: 11
Patch Set 2 : address kinuko's comments #Patch Set 3 : tweak #Patch Set 4 : style fix #Dependent Patchsets: Messages
Total messages: 48 (31 generated)
The CQ bit was checked by nhiroki@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_...)
Description was changed from ========== SharedWorker: Remove common message forwarding mechanism BUG=612308 ========== to ========== SharedWorker: Remove generic message forwarding mechanism SharedWorker service has a generic mechanism to forward a message sent from a document to a worker via browser process. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets it and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. This CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ==========
The CQ bit was checked by nhiroki@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 ========== SharedWorker: Remove generic message forwarding mechanism SharedWorker service has a generic mechanism to forward a message sent from a document to a worker via browser process. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets it and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. This CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ========== to ========== SharedWorker: Remove message forwarding mechanism from SharedWorkerServiceImpl SharedWorkerServiceImpl has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ==========
Description was changed from ========== SharedWorker: Remove message forwarding mechanism from SharedWorkerServiceImpl SharedWorkerServiceImpl has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ========== to ========== SharedWorker: Remove message forwarding mechanism from SharedWorkerServiceImpl SharedWorkerServiceImpl has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ==========
The CQ bit was checked by nhiroki@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
nhiroki@chromium.org changed reviewers: + horo@chromium.org, kinuko@chromium.org
Hi, can you review this? This change is useful for SharedWorker mojofication: https://codereview.chromium.org/2600113003/ Thank you.
Description was changed from ========== SharedWorker: Remove message forwarding mechanism from SharedWorkerServiceImpl SharedWorkerServiceImpl has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ========== to ========== SharedWorker: Remove message forwarding mechanism SharedWorkerServiceImpl has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ==========
Description was changed from ========== SharedWorker: Remove message forwarding mechanism SharedWorkerServiceImpl has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ========== to ========== SharedWorker: Remove message forwarding mechanism SharedWorker service has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ==========
https://codereview.chromium.org/2601893002/diff/40001/content/renderer/websha... File content/renderer/websharedworker_proxy.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/renderer/websha... content/renderer/websharedworker_proxy.cc:47: // also want to queue the message. Question: How is the case where |route_id_| is none handled? Apparently, |route_id_| is never updated before sending a queued messages, and the messages are ignored in the browser process. Am I missing something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2601893002/diff/40001/content/renderer/websha... File content/renderer/websharedworker_proxy.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/renderer/websha... content/renderer/websharedworker_proxy.cc:47: // also want to queue the message. On 2016/12/28 08:04:20, nhiroki wrote: > Question: How is the case where |route_id_| is none handled? Apparently, > |route_id_| is never updated before sending a queued messages, and the messages > are ignored in the browser process. Am I missing something? I understand this. A route_id passed to the ctor of this class is always valid: https://chromium.googlesource.com/chromium/src/+/8a00f15f144888ccbee12c733185... My recent patch changed this code, but the route_id should be still always valid: https://codereview.chromium.org/2604733002 Probably we should update these comments and make |route_id| const. I'll make a separate patch.
https://codereview.chromium.org/2601893002/diff/40001/content/renderer/websha... File content/renderer/websharedworker_proxy.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/renderer/websha... content/renderer/websharedworker_proxy.cc:47: // also want to queue the message. On 2016/12/28 08:16:19, nhiroki (OOO until Jan 5) wrote: > On 2016/12/28 08:04:20, nhiroki wrote: > > Question: How is the case where |route_id_| is none handled? Apparently, > > |route_id_| is never updated before sending a queued messages, and the > messages > > are ignored in the browser process. Am I missing something? > > I understand this. A route_id passed to the ctor of this class is always valid: > https://chromium.googlesource.com/chromium/src/+/8a00f15f144888ccbee12c733185... > > My recent patch changed this code, but the route_id should be still always > valid: > https://codereview.chromium.org/2604733002 > > Probably we should update these comments and make |route_id| const. I'll make a > separate patch. Created a patch: https://codereview.chromium.org/2607753002/
Patchset #2 (id:60001) has been deleted
lgtm with a nit. https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... File content/browser/shared_worker/shared_worker_host.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... content/browser/shared_worker/shared_worker_host.cc:120: if (!instance_) nit: DCHECK(instance_);
Thank you for your comment. https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... File content/browser/shared_worker/shared_worker_host.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... content/browser/shared_worker/shared_worker_host.cc:120: if (!instance_) On 2017/01/05 03:54:13, horo wrote: > nit: DCHECK(instance_); I'll make a separate CL to replace all such if-statements at once.
Description was changed from ========== SharedWorker: Remove message forwarding mechanism SharedWorker service has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ========== to ========== SharedWorker: Remove message forwarding mechanism SharedWorker service has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ==========
nhiroki@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng@, can you review content/common/view_messages.h? Thanks.
https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... File content/browser/shared_worker/shared_worker_service_impl.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... content/browser/shared_worker/shared_worker_service_impl.cc:334: if (iter->second->CanHandleMessage(route_id, filter)) { It feels this method doesn't need to be split from Connect, and no need to worry about DCHECK(instance_) etc
(lgtm/2)
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by nhiroki@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...
https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... File content/browser/shared_worker/shared_worker_host.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... content/browser/shared_worker/shared_worker_host.cc:120: if (!instance_) On 2017/01/05 04:11:40, nhiroki wrote: > On 2017/01/05 03:54:13, horo wrote: > > nit: DCHECK(instance_); > > I'll make a separate CL to replace all such if-statements at once. Why is it OK to DCHECK instance_? https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... content/browser/shared_worker/shared_worker_host.cc:123: DCHECK(container_render_filter_); Ditto: why is this OK to DCHECK? These messages are coming from the renderer and untrusted. So it could send them at an invalid time, I think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for your comments. https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... File content/browser/shared_worker/shared_worker_host.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... content/browser/shared_worker/shared_worker_host.cc:120: if (!instance_) On 2017/01/05 06:19:18, dcheng wrote: > On 2017/01/05 04:11:40, nhiroki wrote: > > On 2017/01/05 03:54:13, horo wrote: > > > nit: DCHECK(instance_); > > > > I'll make a separate CL to replace all such if-statements at once. > > Why is it OK to DCHECK instance_? |instance_| is set at the ctor of SharedWorkerHost and unset in SharedWorkerHost::WorkerContextDestroyed. The host is destroyed immediately after WorkerContextDestroyed (see [1]), so |instance_| should not be null here. [1] https://chromium.googlesource.com/chromium/src/+/a2f777316c98a1bcc06f2d34120b... https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... content/browser/shared_worker/shared_worker_host.cc:123: DCHECK(container_render_filter_); On 2017/01/05 06:19:18, dcheng wrote: > Ditto: why is this OK to DCHECK? These messages are coming from the renderer and > untrusted. So it could send them at an invalid time, I think. Because this filter is set at the ctor and the host is destroyed when the filter is closed (see [2]) [2] https://chromium.googlesource.com/chromium/src/+/a2f777316c98a1bcc06f2d34120b...
https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... File content/browser/shared_worker/shared_worker_service_impl.cc (right): https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... content/browser/shared_worker/shared_worker_service_impl.cc:334: if (iter->second->CanHandleMessage(route_id, filter)) { On 2017/01/05 04:28:25, kinuko wrote: > It feels this method doesn't need to be split from Connect, and no need to worry > about DCHECK(instance_) etc Done.
On 2017/01/05 07:54:33, nhiroki (slow) wrote: > Thank you for your comments. > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > File content/browser/shared_worker/shared_worker_host.cc (right): > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > content/browser/shared_worker/shared_worker_host.cc:120: if (!instance_) > On 2017/01/05 06:19:18, dcheng wrote: > > On 2017/01/05 04:11:40, nhiroki wrote: > > > On 2017/01/05 03:54:13, horo wrote: > > > > nit: DCHECK(instance_); > > > > > > I'll make a separate CL to replace all such if-statements at once. > > > > Why is it OK to DCHECK instance_? > > |instance_| is set at the ctor of SharedWorkerHost and unset in > SharedWorkerHost::WorkerContextDestroyed. The host is destroyed immediately > after WorkerContextDestroyed (see [1]), so |instance_| should not be null here. > > [1] > https://chromium.googlesource.com/chromium/src/+/a2f777316c98a1bcc06f2d34120b... > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > content/browser/shared_worker/shared_worker_host.cc:123: > DCHECK(container_render_filter_); > On 2017/01/05 06:19:18, dcheng wrote: > > Ditto: why is this OK to DCHECK? These messages are coming from the renderer > and > > untrusted. So it could send them at an invalid time, I think. > > Because this filter is set at the ctor and the host is destroyed when the filter > is closed (see [2]) > > [2] > https://chromium.googlesource.com/chromium/src/+/a2f777316c98a1bcc06f2d34120b... IPC LGTM. thanks for the explanation. it's a little subtle, it might be worth documenting lifetime in comments somewhere. I notice there's one location that still null checks this, even though it can never be null: https://cs.chromium.org/chromium/src/content/browser/shared_worker/shared_wor... is that worth cleaning up in the followup as well?
On 2017/01/05 16:33:34, dcheng wrote: > On 2017/01/05 07:54:33, nhiroki (slow) wrote: > > Thank you for your comments. > > > > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > > File content/browser/shared_worker/shared_worker_host.cc (right): > > > > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > > content/browser/shared_worker/shared_worker_host.cc:120: if (!instance_) > > On 2017/01/05 06:19:18, dcheng wrote: > > > On 2017/01/05 04:11:40, nhiroki wrote: > > > > On 2017/01/05 03:54:13, horo wrote: > > > > > nit: DCHECK(instance_); > > > > > > > > I'll make a separate CL to replace all such if-statements at once. > > > > > > Why is it OK to DCHECK instance_? > > > > |instance_| is set at the ctor of SharedWorkerHost and unset in > > SharedWorkerHost::WorkerContextDestroyed. The host is destroyed immediately > > after WorkerContextDestroyed (see [1]), so |instance_| should not be null > here. > > > > [1] > > > https://chromium.googlesource.com/chromium/src/+/a2f777316c98a1bcc06f2d34120b... > > > > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > > content/browser/shared_worker/shared_worker_host.cc:123: > > DCHECK(container_render_filter_); > > On 2017/01/05 06:19:18, dcheng wrote: > > > Ditto: why is this OK to DCHECK? These messages are coming from the renderer > > and > > > untrusted. So it could send them at an invalid time, I think. > > > > Because this filter is set at the ctor and the host is destroyed when the > filter > > is closed (see [2]) > > > > [2] > > > https://chromium.googlesource.com/chromium/src/+/a2f777316c98a1bcc06f2d34120b... > > IPC LGTM. thanks for the explanation. it's a little subtle, it might be worth > documenting lifetime in comments somewhere. I notice there's one location that > still null checks this, even though it can never be null: > https://cs.chromium.org/chromium/src/content/browser/shared_worker/shared_wor... > is that worth cleaning up in the followup as well? Thank you. Documenting lifetime is a good idea. I'll make a followup CL.
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, horo@chromium.org Link to the patchset: https://codereview.chromium.org/2601893002/#ps140001 (title: "style fix")
Description was changed from ========== SharedWorker: Remove message forwarding mechanism SharedWorker service has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ========== to ========== SharedWorker: Remove message forwarding mechanism SharedWorker service has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ==========
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": 140001, "attempt_start_ts": 1483658895870350,
"parent_rev": "b21270407f03c618ce6b187dacd743e359d998d0", "commit_rev":
"b46ff78092b42dfe9f3d816a4c16972217f9947a"}
Message was sent while issue was closed.
Description was changed from ========== SharedWorker: Remove message forwarding mechanism SharedWorker service has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 ========== to ========== SharedWorker: Remove message forwarding mechanism SharedWorker service has a mechanism to forward a message sent from a document to a worker. With this mechanism, a sender creates a nested IPC message and sends it to the SharedWorker service in the browser process. The service interprets the nested message and forwards it to an appropriate receiver. This mechanism is available for any IPC messages but it has been used only for worker connection message. If we use it only for one message type, we don't have to manage such a complex mechanism. Therefore, this CL removes the mechanism and uses a plain IPC message for connecting to a worker. This simplification also makes it easier to mojofy SharedWorker. BUG=612308 Review-Url: https://codereview.chromium.org/2601893002 Cr-Commit-Position: refs/heads/master@{#441785} Committed: https://chromium.googlesource.com/chromium/src/+/b46ff78092b42dfe9f3d816a4c16... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/b46ff78092b42dfe9f3d816a4c16...
Message was sent while issue was closed.
On 2017/01/05 23:28:06, nhiroki wrote: > On 2017/01/05 16:33:34, dcheng wrote: > > On 2017/01/05 07:54:33, nhiroki (slow) wrote: > > > Thank you for your comments. > > > > > > > > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > > > File content/browser/shared_worker/shared_worker_host.cc (right): > > > > > > > > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > > > content/browser/shared_worker/shared_worker_host.cc:120: if (!instance_) > > > On 2017/01/05 06:19:18, dcheng wrote: > > > > On 2017/01/05 04:11:40, nhiroki wrote: > > > > > On 2017/01/05 03:54:13, horo wrote: > > > > > > nit: DCHECK(instance_); > > > > > > > > > > I'll make a separate CL to replace all such if-statements at once. > > > > > > > > Why is it OK to DCHECK instance_? > > > > > > |instance_| is set at the ctor of SharedWorkerHost and unset in > > > SharedWorkerHost::WorkerContextDestroyed. The host is destroyed immediately > > > after WorkerContextDestroyed (see [1]), so |instance_| should not be null > > here. > > > > > > [1] > > > > > > https://chromium.googlesource.com/chromium/src/+/a2f777316c98a1bcc06f2d34120b... > > > > > > > > > https://codereview.chromium.org/2601893002/diff/40001/content/browser/shared_... > > > content/browser/shared_worker/shared_worker_host.cc:123: > > > DCHECK(container_render_filter_); > > > On 2017/01/05 06:19:18, dcheng wrote: > > > > Ditto: why is this OK to DCHECK? These messages are coming from the > renderer > > > and > > > > untrusted. So it could send them at an invalid time, I think. > > > > > > Because this filter is set at the ctor and the host is destroyed when the > > filter > > > is closed (see [2]) > > > > > > [2] > > > > > > https://chromium.googlesource.com/chromium/src/+/a2f777316c98a1bcc06f2d34120b... > > > > IPC LGTM. thanks for the explanation. it's a little subtle, it might be worth > > documenting lifetime in comments somewhere. I notice there's one location that > > still null checks this, even though it can never be null: > > > https://cs.chromium.org/chromium/src/content/browser/shared_worker/shared_wor... > > is that worth cleaning up in the followup as well? > > Thank you. Documenting lifetime is a good idea. I'll make a followup CL. JFYI: I created a CL: https://codereview.chromium.org/2614013004/ |
