New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 783071 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 772713
issue 772788
issue 772793



Sign in to add a comment

Detach EmbeddedWorkerInstanceClient interface from the legacy IPC channel

Project Member Reported by leon....@intel.com, Nov 9 2017

Issue description

EmbeddedWorkerInstanceClient interface was associated on the legacy IPC channel by CL https://chromium-review.googlesource.com/618077 to solve crashes seen at 753708.

Those crashes were caused by ordering issue between some legacy IPCs and SWContainerHost interface disconnection IPC (https://bugs.chromium.org/p/chromium/issues/detail?id=753708#c11), but now all those related legacy IPCs have been mojofied into SWContainerHost interface, so the root cause of 753708 before should have disappeared.

Then, once we mojofied the left IPCs (772788, 772713, 772793), we can detach EmbeddedWorkerInstanceClient from the legacy IPC channel completely.
 

Comment 1 by leon....@intel.com, Nov 10 2017

After this work, we should be able to address the TODO at https://cs.chromium.org/chromium/src/content/renderer/service_worker/web_service_worker_registration_impl.h?rcl=514d6492a599d544e46b2508c54f69a4b7bf16ea&l=249
'
  // |host_for_global_scope_| is for service worker execution contexts. It is
  // used on the worker thread but bound on the IO thread, because it's a
  // channel- associated interface which can be bound only on the main or IO
  // thread.
  // TODO(leonhsl): Once we can detach this interface out from the legacy IPC
  // channel-associated interfaces world, we should bind it always on the worker
  // thread on which |this| lives.
'
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5

commit d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5
Author: Han Leon <leon.han@intel.com>
Date: Tue Apr 17 15:39:25 2018

[ServiceWorker] Enter the pure Mojo world

This CL aims to mojofy the last one legacy IPC
ServiceWorkerMsg_ServiceWorkerStateChanged, then service worker code
finally enter the pure Mojo world!

Currently ServiceWorkerRegistrationObject and ServiceWorkerContainer are
channel-associated interfaces. For correctness, ServiceWorkerObject
needs to have FIFO ordering with those interfaces, which means it would
need to be a channel-associated interface as well. But when used for
service worker execution contexts, we have been binding
channel-associated interfaces on the IO thread rather than the service
worker thread due to constraints of channel-associated interfaces.[1]
But we want ServiceWorkerObject to be bound on the service worker
thread. Actually we want SWContainer and SWRO to be bound on the service
worker thread too instead of bouncing to the IO thread. Until now, we
have worked around this using hacks like CallbackWrapperOnWorkerThread
to mediate between the IO thread and service worker thread. But we do
not want to keep doing these hacks. Since we no longer have legacy IPCs,
there is no need for these interfaces to be channel-associated
interfaces. So this CL makes these no longer channel-associated
interfaces so they can be bound on the service worker thread directly.
[1] https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/RDO46Py0F3s

Based on the above analysis, this CL goes a long way to introduce
ServiceWorkerObject interface. This CL:
ps#1 - firstly decouples EmbeddedWorkerInstanceClient interface from the
   world of Channel-associated interfaces, makes it use a dedicated Mojo
   message pipe, this turns out to make its several attached interfaces
   not be Channel-associated interfaces, either, like
   ServiceWorkerRegistrationObject, ServiceWorkerHost etc.
ps#2 - then, for service worker execution contexts, starts to bind
   endpoints for interfaces like ServiceWorkerHost,
   ServiceWorkerObject[Host], ServiceWorkerRegistrationObject[Host] etc.
   on the service worker thread directly, rather than binding them on
   the IO thread and using PostTask to actually send/receive Mojo
   messages on the worker thread, this had been forced to be so because
   they were Channel-associated interfaces before.
ps#6 - after the above preparations, introduces ServiceWorkerObject
   interface and mojofies the state changed legacy IPC, it's also
   associated with EmbeddedWorkerInstanceClient interface and now we can
   just bind its receiver endpoint directly on the service worker
   thread.

Follow-up work:
  - CL https://chromium-review.googlesource.com/c/chromium/src/+/981898
    Associate all interfaces living on the service worker thread with
    ServiceWorkerEventDispatcher interface rather than current
    EmbeddedWorkerInstanceClient interface. This will not only guarantee
    the FIFO ordering with ServiceWorkerEventDispatcher, but also make
    all Mojo messages dispatch skip the main thread completely to avoid
    possible congestion.
  - Fix the layout test
    external/wpt/service-workers/service-worker/skip-waiting-installed.https.html
    which becomes flaky due to this CL.
  - Remove channel-associated interfaces entirely for service worker
    code. This CL makes interfaces for SW execution contexts to be not
    channel-associated. A follow-up CL will make the same change for
    interfaces for SW client contexts.

BUG= 772788 , 783071 

Change-Id: I3a6a666a41b4edc2289dceee6e071ef9510e2012
Reviewed-on: https://chromium-review.googlesource.com/967863
Commit-Queue: Han Leon <leon.han@intel.com>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Han Leon <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#551343}
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/embedded_worker_instance_unittest.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/embedded_worker_test_helper.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/embedded_worker_test_helper.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_dispatcher_host.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_dispatcher_host.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_handle.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_handle.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_handle_unittest.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_registration_object_host.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_registration_object_host.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_registration_unittest.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_test_utils.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/common/renderer.mojom
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/common/service_worker/service_worker_messages.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/render_thread_impl.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/embedded_worker_instance_client_impl.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/embedded_worker_instance_client_impl.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_dispatcher.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_dispatcher.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_dispatcher_unittest.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_message_filter.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_message_filter.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_provider_context.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_provider_context.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/service_worker_provider_context_unittest.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/web_service_worker_impl.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/web_service_worker_impl.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/web_service_worker_registration_impl.cc
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/content/renderer/service_worker/web_service_worker_registration_impl.h
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/third_party/blink/public/mojom/service_worker/service_worker_object.mojom
[modify] https://crrev.com/d24f8b2abbb47b3b76e3b3cf3c2af24b96ad37e5/third_party/blink/public/mojom/service_worker/service_worker_registration.mojom

Comment 3 by leon....@intel.com, May 15 2018

Status: Fixed (was: Assigned)

Sign in to add a comment