Use Associated interfaces for mojo-loading

Use Associated interfaces for mojo-loading

This CL makes URLLoader and URLLoaderClient associated for mojo-loading in
order to deal with the IPC ordering issue with the exising IPC messages.

BUG=603396
Committed: https://crrev.com/fe95d9b23ae824a85531260d87bb6f7897ea139c
Cr-Commit-Position: refs/heads/master@{#430216}

[yhirano, 2016-10-26 07:17:56]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 07:18:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 07:20:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 07:20:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yhirano, 2016-10-26 07:33:16]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 07:33:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 07:36:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 07:36:20]

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_...)

[yhirano, 2016-10-26 07:37:57]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 07:38:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 07:55:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 07:55:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[yhirano, 2016-10-26 08:20:57]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 08:21:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-26 08:24:10]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 08:24:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-26 08:33:38]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 08:33:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 09:49:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 09:49:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-10-26 10:43:19]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 10:43:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-26 11:23:50]

Patchset #1 (id:1) has been deleted

[yhirano, 2016-10-26 11:23:53]

Patchset #4 (id:80001) has been deleted

[yhirano, 2016-10-26 11:24:10]

Patchset #1 (id:20001) has been deleted

[yhirano, 2016-10-26 11:24:12]

Patchset #1 (id:40001) has been deleted

[yhirano, 2016-10-26 11:24:16]

Patchset #1 (id:60001) has been deleted

[yhirano, 2016-10-26 11:24:26]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-10-26 12:12:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 12:12:01]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-26 12:24:41]

Description was changed from

==========
[WIP] Use Associated interfaces for mojo-loading

BUG=603396
==========

to

==========
Use Associated interfaces for mojo-loading

This CL makes URLLoader and URLLoaderClient associated for mojo-loading in
order to deal with the IPC ordering issue with the exising IPC messages.

BUG=603396
==========

[yhirano, 2016-10-26 12:32:56]

yhirano@chromium.org changed reviewers:
+ horo@chromium.org, jam@chromium.org

[yhirano, 2016-10-26 12:32:57]

PTAL.

This change may break service worker navigation preload feature (I'm not sure -
there is no tests for the feature right now), because according to [1] calling
URLLoaderClient functions before passing the associated URLLoaderClientRequest
to the remote end is forbidden.

The only solution I can come up with is creating a proxy URLLoaderClient impl
receiving messages from MojoAsyncResourceHandler and sending mojo message to the
renderer.

1:
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/associated_grou...

[jam, 2016-10-26 15:55:23]

jam@chromium.org changed reviewers:
+ rockot@chromium.org

[jam, 2016-10-26 15:55:55]

lgtm

On 2016/10/26 12:32:57, yhirano wrote:
> PTAL.
> 
> This change may break service worker navigation preload feature (I'm not sure
-
> there is no tests for the feature right now), because according to [1] calling
> URLLoaderClient functions before passing the associated URLLoaderClientRequest
> to the remote end is forbidden.
> 
> The only solution I can come up with is creating a proxy URLLoaderClient impl
> receiving messages from MojoAsyncResourceHandler and sending mojo message to
the
> renderer.
> 
> 1:
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/associated_grou...

+Ken to see if he knows of other ways to do this

[Ken Rockot(use gerrit already), 2016-10-26 16:20:20]

On 2016/10/26 at 15:55:55, jam wrote:
> lgtm
> 
> On 2016/10/26 12:32:57, yhirano wrote:
> > PTAL.
> > 
> > This change may break service worker navigation preload feature (I'm not
sure -
> > there is no tests for the feature right now), because according to [1]
calling
> > URLLoaderClient functions before passing the associated
URLLoaderClientRequest
> > to the remote end is forbidden.
> > 
> > The only solution I can come up with is creating a proxy URLLoaderClient
impl
> > receiving messages from MojoAsyncResourceHandler and sending mojo message to
the
> > renderer.
> > 
> > 1:
> >
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/associated_grou...
> 
> +Ken to see if he knows of other ways to do this

There is no way around this constraint. It is fundamental to the definition of
associated interfaces. After all, the interface request you're sending needs its
messages to be dispatched in order with respect to the interface who's receiving
the interface request. Since it is impossible for us to dispatch messages to an
interface before that interface is bound, and it's impossible for us to bind an
interface before the interface request is received, it is strictly impossible
for us to support the behavior of 

It's not just a maybe-broken situation, it is definitely broken.

I don't have any better suggestions at the moment, but I don't think you should
land this as-is. Please internalize the reason why associated interfaces have
this constraint.

[Ken Rockot(use gerrit already), 2016-10-26 16:21:24]

On 2016/10/26 at 16:20:20, Ken Rockot wrote:
> On 2016/10/26 at 15:55:55, jam wrote:
> > lgtm
> > 
> > On 2016/10/26 12:32:57, yhirano wrote:
> > > PTAL.
> > > 
> > > This change may break service worker navigation preload feature (I'm not
sure -
> > > there is no tests for the feature right now), because according to [1]
calling
> > > URLLoaderClient functions before passing the associated
URLLoaderClientRequest
> > > to the remote end is forbidden.
> > > 
> > > The only solution I can come up with is creating a proxy URLLoaderClient
impl
> > > receiving messages from MojoAsyncResourceHandler and sending mojo message
to the
> > > renderer.
> > > 
> > > 1:
> > >
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/associated_grou...
> > 
> > +Ken to see if he knows of other ways to do this
> 
> There is no way around this constraint. It is fundamental to the definition of
associated interfaces. After all, the interface request you're sending needs its
messages to be dispatched in order with respect to the interface who's receiving
the interface request. Since it is impossible for us to dispatch messages to an
interface before that interface is bound, and it's impossible for us to bind an
interface before the interface request is received, it is strictly impossible
for us to support the behavior of 
> 

Bad UI - sent while editing.  It is strictly impossible for us to support the
behavior of queueing messages on an associated interface before the request end
is sent over the pipe.

[horo, 2016-10-26 16:55:59]

On 2016/10/26 12:32:57, yhirano wrote:
> This change may break service worker navigation preload feature (I'm not sure
-
> there is no tests for the feature right now), because according to [1] calling
> URLLoaderClient functions before passing the associated URLLoaderClientRequest
> to the remote end is forbidden.

I just landed the tests for service worker navigation preload.
https://codereview.chromium.org/2413643005/
Sorry for the delay. It has been blocked by the try bot flakiness :(
I got this crash log while executing
ServiceWorkerBrowserTest/ServiceWorkerNavigationPreloadTest* with debug build.

[12306:12326:1027/014859:3845805656518:FATAL:handle_interface_serialization.h(32)]
Check failed: !input.handle().is_valid() || !input.handle().is_local(). 
#0 0x7f1b107e44ce base::debug::StackTrace::StackTrace()
#1 0x7f1b108539ff logging::LogMessage::~LogMessage()
#2 0x7f1b11a26574 mojo::internal::Serializer<>::Serialize()
#3 0x7f1b11a6940a
_ZN4mojo8internal9SerializeINS_34AssociatedInterfacePtrInfoDataViewIN7content5mojom28URLLoaderClientInterfaceBaseEEERNS_26AssociatedInterfacePtrInfoINS4_15URLLoaderClientEEEJPNS0_24AssociatedInterface_DataEPNS0_20SerializationContextEELPv0EEEvOT0_DpOT1_
#4 0x7f1b11a6748f content::mojom::URLLoaderFactoryProxy::CreateLoaderAndStart()
#5 0x7f1b12a81371
content::ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload()
#6 0x7f1b12b1eab9
content::ServiceWorkerURLRequestJob::RequestBodyBlobsCompleted()
#7 0x7f1b12b1e365 content::ServiceWorkerURLRequestJob::StartRequest()
#8 0x7f1b1169bd87
_ZN4base8internal13FunctorTraitsIMN7content15ChildThreadImplEFvvEvE6InvokeIRKNS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_
#9 0x7f1b12b2415a
_ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN7content26ServiceWorkerURLRequestJobEFvvERKNS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_
#10 0x7f1b12b240e2
_ZN4base8internal7InvokerINS0_9BindStateIMN7content26ServiceWorkerURLRequestJobEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#11 0x7f1b12b2402c
_ZN4base8internal7InvokerINS0_9BindStateIMN7content26ServiceWorkerURLRequestJobEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
#12 0x7f1b107ea3b1
_ZNO4base8internal8RunMixinINS_8CallbackIFvvELNS0_8CopyModeE0ELNS0_10RepeatModeE0EEEE3RunEv
#13 0x7f1b107e9db9 base::debug::TaskAnnotator::RunTask()
#14 0x7f1b1087c8ba base::MessageLoop::RunTask()
#15 0x7f1b1087cb44 base::MessageLoop::DeferOrRunPendingTask()
#16 0x7f1b1087ce2e base::MessageLoop::DoWork()
#17 0x7f1b1089772e base::MessagePumpLibevent::Run()
#18 0x7f1b1087c43a base::MessageLoop::RunHandler()
#19 0x7f1b10923574 base::RunLoop::Run()
#20 0x7f1b109c8d08 base::Thread::Run()
#21 0x7f1b12068406 content::BrowserThreadImpl::IOThreadRun()
#22 0x7f1b1206870b content::BrowserThreadImpl::Run()
#23 0x7f1b109c95aa base::Thread::ThreadMain()
#24 0x7f1b109b05ca base::(anonymous namespace)::ThreadFunc()
#25 0x7f1b179f4184 start_thread
#26 0x7f1b04b3137d clone

[yhirano, 2016-10-26 17:00:46]

On 2016/10/26 16:21:24, Ken Rockot wrote:
> On 2016/10/26 at 16:20:20, Ken Rockot wrote:
> > On 2016/10/26 at 15:55:55, jam wrote:
> > > lgtm
> > > 
> > > On 2016/10/26 12:32:57, yhirano wrote:
> > > > PTAL.
> > > > 
> > > > This change may break service worker navigation preload feature (I'm not
> sure -
> > > > there is no tests for the feature right now), because according to [1]
> calling
> > > > URLLoaderClient functions before passing the associated
> URLLoaderClientRequest
> > > > to the remote end is forbidden.
> > > > 
> > > > The only solution I can come up with is creating a proxy URLLoaderClient
> impl
> > > > receiving messages from MojoAsyncResourceHandler and sending mojo
message
> to the
> > > > renderer.
> > > > 
> > > > 1:
> > > >
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/associated_grou...
> > > 
> > > +Ken to see if he knows of other ways to do this
> > 
> > There is no way around this constraint. It is fundamental to the definition
of
> associated interfaces. After all, the interface request you're sending needs
its
> messages to be dispatched in order with respect to the interface who's
receiving
> the interface request. Since it is impossible for us to dispatch messages to
an
> interface before that interface is bound, and it's impossible for us to bind
an
> interface before the interface request is received, it is strictly impossible
> for us to support the behavior of 
> > 
> 
> Bad UI - sent while editing.  It is strictly impossible for us to support the
> behavior of queueing messages on an associated interface before the request
end
> is sent over the pipe.

There are two customers of URLLoaderClient - one is the loading pipeline which
needs the IPC ordering guarantee. The other is service worker which is not happy
with associated interface but doesn't need the ordering guarantee. My idea at
#33 is having a intermediary between MojoAsyncResourceHandler and the consumer
in the renderer (for the ServiceWorker use-case)

before this CL:
 | ResourceHandler | <= independent => | ServiceWorker customer |

PS1: (not working, as SW customer will be initialized later)
 | ResourceHandler | <= associated => | SW customer |

#33 idea:
 | ResourceHandler | <= associated => | intermediary | <= independent => | SW
customer |

Does the idea still sound bad?

[Ken Rockot(use gerrit already), 2016-10-26 17:01:29]

It looks like you're trying to send an unexpected side of an associated
interface pipe.

Note that mojo::GetProxy for associated interfaces can only be used when
you want to send the request side over a pipe. If you want to send the ptr
side, as you do in some cases, you have to either use the variant of
mojo::AssociatedBinding<T>::Bind which takes an AssociatedPtrInfo*, or
manually call
AssociatedGroup::CreateAssociatedInterface(AssociatedGroup::WILL_PASS_PTR,
&ptr_info, &request);


On Wed, Oct 26, 2016 at 9:55 AM, <horo@chromium.org> wrote:

> On 2016/10/26 12:32:57, yhirano wrote:
> > This change may break service worker navigation preload feature (I'm not
> sure
> -
> > there is no tests for the feature right now), because according to [1]
> calling
> > URLLoaderClient functions before passing the associated
> URLLoaderClientRequest
> > to the remote end is forbidden.
>
> I just landed the tests for service worker navigation preload.
> https://codereview.chromium.org/2413643005/
> Sorry for the delay. It has been blocked by the try bot flakiness :(
> I got this crash log while executing
> ServiceWorkerBrowserTest/ServiceWorkerNavigationPreloadTest* with debug
> build.
>
> [12306:12326:1027/014859:3845805656518:FATAL:handle_
> interface_serialization.h(32)]
> Check failed: !input.handle().is_valid() || !input.handle().is_local().
> #0 0x7f1b107e44ce base::debug::StackTrace::StackTrace()
> #1 0x7f1b108539ff logging::LogMessage::~LogMessage()
> #2 0x7f1b11a26574 mojo::internal::Serializer<>::Serialize()
> #3 0x7f1b11a6940a
> _ZN4mojo8internal9SerializeINS_34AssociatedInterfacePtrInfoDa
> taViewIN7content5mojom28URLLoaderClientInterfaceBaseEEERNS_
> 26AssociatedInterfacePtrInfoINS4_15URLLoaderClientEEEJPNS0_
> 24AssociatedInterface_DataEPNS0_20SerializationContextEELPv0EEEvOT0_DpOT1_
> #4 0x7f1b11a6748f content::mojom::URLLoaderFactoryProxy::
> CreateLoaderAndStart()
> #5 0x7f1b12a81371
> content::ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload()
> #6 0x7f1b12b1eab9
> content::ServiceWorkerURLRequestJob::RequestBodyBlobsCompleted()
> #7 0x7f1b12b1e365 content::ServiceWorkerURLRequestJob::StartRequest()
> #8 0x7f1b1169bd87
> _ZN4base8internal13FunctorTraitsIMN7content15ChildThreadImplE
> FvvEvE6InvokeIRKNS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_
> #9 0x7f1b12b2415a
> _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN7content2
> 6ServiceWorkerURLRequestJobEFvvERKNS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_
> #10 0x7f1b12b240e2
> _ZN4base8internal7InvokerINS0_9BindStateIMN7content26Service
> WorkerURLRequestJobEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIRKS6_
> RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
> #11 0x7f1b12b2402c
> _ZN4base8internal7InvokerINS0_9BindStateIMN7content26Service
> WorkerURLRequestJobEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE3RunEPNS0_
> 13BindStateBaseE
> #12 0x7f1b107ea3b1
> _ZNO4base8internal8RunMixinINS_8CallbackIFvvELNS0_8CopyModeE0ELNS0_
> 10RepeatModeE0EEEE3RunEv
> #13 0x7f1b107e9db9 base::debug::TaskAnnotator::RunTask()
> #14 0x7f1b1087c8ba base::MessageLoop::RunTask()
> #15 0x7f1b1087cb44 base::MessageLoop::DeferOrRunPendingTask()
> #16 0x7f1b1087ce2e base::MessageLoop::DoWork()
> #17 0x7f1b1089772e base::MessagePumpLibevent::Run()
> #18 0x7f1b1087c43a base::MessageLoop::RunHandler()
> #19 0x7f1b10923574 base::RunLoop::Run()
> #20 0x7f1b109c8d08 base::Thread::Run()
> #21 0x7f1b12068406 content::BrowserThreadImpl::IOThreadRun()
> #22 0x7f1b1206870b content::BrowserThreadImpl::Run()
> #23 0x7f1b109c95aa base::Thread::ThreadMain()
> #24 0x7f1b109b05ca base::(anonymous namespace)::ThreadFunc()
> #25 0x7f1b179f4184 start_thread
> #26 0x7f1b04b3137d clone
>
>
>
> https://codereview.chromium.org/2449933003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yhirano, 2016-10-27 04:24:10]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 04:24:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-27 04:35:09]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 04:35:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 05:07:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 05:07:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[yhirano, 2016-10-27 05:44:49]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 05:45:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-27 06:56:29]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 06:56:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-27 07:02:26]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 07:03:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-27 08:41:17]

Patchset #6 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-10-27 09:02:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 09:02:23]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-10-27 09:38:42]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 09:39:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-27 10:43:43]

Rebased, and fixed the test failure. PTAL again.

[commit-bot: I haz the power, 2016-10-27 11:07:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 11:07:23]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-10-27 11:11:23]

On 2016/10/27 10:43:43, yhirano wrote:
> Rebased, and fixed the test failure. PTAL again.

Oops, I thought I fixed them but two tests keep failing on Windows. I will look
into the issue tomorrow.

[Ken Rockot(use gerrit already), 2016-10-27 16:20:23]

Taking a step back, what do you mean in the CL description by "deal with the IPC
ordering issue with existing IPC messages"?

If your intent is to order URLLoader/Client messages with respect to arbitrary
legacy IPC, this does not accomplish that at all.

Associated interfaces are in the general case only associated within a closed
group of interfaces. In this case, URLLoaderFactory acts as the master interface
(i.e. the only one with a real pipe underneath), and you're only guaranteeing
FIFO among URLLoaderFactory, URLLoader, and URLLoaderClient messages.

We provide a way of constructing Channel-associated interfaces which shared the
IPC::Channel's pipe. If you want FIFO against all legacy IPC, you need
URLLoaderFactory itself to become a Channel-associated interface, in addition to
your changes here.

It's also worth noting that I don't think your delegating solution solves the
problem of writing to an associated interface before its request end has been
transferred. You bind both of the delegating impls immediately, so it's still
possible for someone to (e.g.) write to the non-associated URLLoaderPtr, and
have that message be dispatched by the Delegating impl to the associated
URLLoaderPtr, all before Fetch() has been called.

It would be helpful to understand what precisely you're trying to solve by
establishing ordering for loader IPCs. We might have a much simpler solution
(https://codereview.chromium.org/2455823002) that could be used without needing
to spread associated interfaces everywhere. The linked CL provides a sort of
Channel-associated barrier that can signal over a non-associated interface to
allow for ad hoc synchronization without requiring strict FIFO everywhere.

https://codereview.chromium.org/2449933003/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2449933003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:394: //
https://ggetithub.com/w3c/ServiceWorker/issues/920#issuecomment-251150270
nit: trampled URL

[yhirano, 2016-10-28 02:12:35]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 02:13:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-28 02:19:51]

On 2016/10/27 16:20:23, Ken Rockot wrote:
> Taking a step back, what do you mean in the CL description by "deal with the
IPC
> ordering issue with existing IPC messages"?
> 
> If your intent is to order URLLoader/Client messages with respect to arbitrary
> legacy IPC, this does not accomplish that at all.
> 
> Associated interfaces are in the general case only associated within a closed
> group of interfaces. In this case, URLLoaderFactory acts as the master
interface
> (i.e. the only one with a real pipe underneath), and you're only guaranteeing
> FIFO among URLLoaderFactory, URLLoader, and URLLoaderClient messages.
> 
> We provide a way of constructing Channel-associated interfaces which shared
the
> IPC::Channel's pipe. If you want FIFO against all legacy IPC, you need
> URLLoaderFactory itself to become a Channel-associated interface, in addition
to
> your changes here.
> 
I'm doing that in the previous CL (https://codereview.chromium.org/2433233004).
As I wrote before there are two use-cases. The Chrome loading pipeline, the one
caring about the ordering guarantee with legacy IPCs, uses
ChannelAssociatedInterface via ResourceMessageFilter.

[horo, 2016-10-28 03:01:28]

https://codereview.chromium.org/2449933003/diff/260001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.h (right):

https://codereview.chromium.org/2449933003/diff/260001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.h:91:
std::unique_ptr<mojom::URLLoaderClient> url_loader_client_;
As I talked offline, you must keep |url_loader_client_| alive until OnComplete()
is called.

[commit-bot: I haz the power, 2016-10-28 03:02:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 03:02:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-10-28 03:25:15]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 03:25:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 03:53:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 03:53:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[yhirano, 2016-10-28 03:58:49]

Patchset #8 (id:280001) has been deleted

[yhirano, 2016-10-31 04:16:47]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 04:16:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 04:28:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 04:28:04]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-10-31 05:40:54]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 05:41:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-10-31 05:47:33]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 05:47:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 07:25:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 07:25:37]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-10-31 10:05:38]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 10:05:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 11:27:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 11:27:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-11-01 02:34:36]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 02:34:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 03:41:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 03:41:58]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-01 06:12:35]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 06:12:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 07:36:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 07:36:59]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-01 07:46:28]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 07:46:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 08:27:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 08:27:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-11-01 08:56:39]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 08:56:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-01 10:10:31]

https://codereview.chromium.org/2449933003/diff/260001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.h (right):

https://codereview.chromium.org/2449933003/diff/260001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.h:91:
std::unique_ptr<mojom::URLLoaderClient> url_loader_client_;
On 2016/10/28 03:01:28, horo wrote:
> As I talked offline, you must keep |url_loader_client_| alive until
OnComplete()
> is called.

Done.

[yhirano, 2016-11-01 10:25:59]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 10:26:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-01 10:45:19]

https://codereview.chromium.org/2449933003/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2449933003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:394: //
https://ggetithub.com/w3c/ServiceWorker/issues/920#issuecomment-251150270
On 2016/10/27 16:20:23, Ken Rockot wrote:
> nit: trampled URL

Done.

[yhirano, 2016-11-01 11:00:16]

> It's also worth noting that I don't think your delegating solution solves the
> problem of writing to an associated interface before its request end has been
> transferred. You bind both of the delegating impls immediately, so it's still
> possible for someone to (e.g.) write to the non-associated URLLoaderPtr, and
> have that message be dispatched by the Delegating impl to the associated
> URLLoaderPtr, all before Fetch() has been called.

I'm confused: In my understanding, 

  mojo::GetProxy(&url_loader_factory_)

creates a pair of handles, and the following code passes the
AssociatedURLLoaderRequest to the factory before making any calls on
|url_loader_associated_ptr|.

  url_loader_factory_->CreateLoaderAndStart(
      mojo::GetProxy(&url_loader_associated_ptr,
                     url_loader_factory_.associated_group()),
      original_info->GetRouteID(), request_id, request,
      std::move(url_loader_client_associated_ptr_info));

Isn't that the comment in associated_group.h is requiring?

> It would be helpful to understand what precisely you're trying to solve by
> establishing ordering for loader IPCs. We might have a much simpler solution
> (https://codereview.chromium.org/2455823002) that could be used without
needing
> to spread associated interfaces everywhere. The linked CL provides a sort of
> Channel-associated barrier that can signal over a non-associated interface to
> allow for ad hoc synchronization without requiring strict FIFO everywhere.
Many existing IPC and associated mojo messages are relying on the Loading IPC
ordering. The issue is interactions with messages we don't know much. We don't
have the full list, and requiring strict FIFO is a easier way for this case I
believe.

[commit-bot: I haz the power, 2016-11-01 11:38:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 11:38:59]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-02 02:24:23]

https://codereview.chromium.org/2449933003/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2449933003/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:36: class
DelegatingURLLoader final : public mojom::URLLoader {
Please write comments about the purpose of this class.
Like: Proxy class from URLLoaderAssociated to URLLoader.

https://codereview.chromium.org/2449933003/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:56: class
DelegatingURLLoaderClient final : public mojom::URLLoaderClient {
ditto

https://codereview.chromium.org/2449933003/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:292:
dispatcher->DispatchFetchEvent(
Write comment about passing those classes.
Like:
Pass |url_loader_factory_|, |url_loader_| and |url_loader_client_| to the
callback to keep them alive while the FetchEvent is ongoing in the service
worker.

[yhirano, 2016-11-02 03:35:06]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 03:35:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-02 03:48:48]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 03:49:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-02 03:53:17]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 03:53:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-02 03:58:38]

Patchset #18 (id:500001) has been deleted

[yhirano, 2016-11-02 03:58:59]

https://codereview.chromium.org/2449933003/diff/460001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2449933003/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:36: class
DelegatingURLLoader final : public mojom::URLLoader {
On 2016/11/02 02:24:23, horo wrote:
> Please write comments about the purpose of this class.
> Like: Proxy class from URLLoaderAssociated to URLLoader.

Done.

https://codereview.chromium.org/2449933003/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:56: class
DelegatingURLLoaderClient final : public mojom::URLLoaderClient {
On 2016/11/02 02:24:23, horo wrote:
> ditto

Done.

https://codereview.chromium.org/2449933003/diff/460001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:292:
dispatcher->DispatchFetchEvent(
On 2016/11/02 02:24:23, horo wrote:
> Write comment about passing those classes.
> Like:
> Pass |url_loader_factory_|, |url_loader_| and |url_loader_client_| to the
> callback to keep them alive while the FetchEvent is ongoing in the service
> worker.

Done.

[commit-bot: I haz the power, 2016-11-02 05:01:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 05:01:42]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-02 07:39:20]

content/browser/service_worker/ lgtm with a nit.

https://codereview.chromium.org/2449933003/diff/520001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2449933003/diff/520001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:369: 
nit:

DCHECK(!url_loader_factory_);

[yhirano, 2016-11-02 07:45:52]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 07:46:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-02 07:58:12]

https://codereview.chromium.org/2449933003/diff/520001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2449933003/diff/520001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:369: 
On 2016/11/02 07:39:20, horo wrote:
> nit:
> 
> DCHECK(!url_loader_factory_);

Done.

[commit-bot: I haz the power, 2016-11-02 09:15:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 09:15:22]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-03 13:08:38]

rockot@, PTAL again.

[Ken Rockot(use gerrit already), 2016-11-03 18:24:55]

rockot@chromium.org changed reviewers:
+ yzshen@chromium.org

[Ken Rockot(use gerrit already), 2016-11-03 18:29:04]

On 2016/11/01 at 11:00:16, yhirano wrote:
> > It's also worth noting that I don't think your delegating solution solves
the
> > problem of writing to an associated interface before its request end has
been
> > transferred. You bind both of the delegating impls immediately, so it's
still
> > possible for someone to (e.g.) write to the non-associated URLLoaderPtr, and
> > have that message be dispatched by the Delegating impl to the associated
> > URLLoaderPtr, all before Fetch() has been called.
> 
> I'm confused: In my understanding, 
> 
>   mojo::GetProxy(&url_loader_factory_)
> 
> creates a pair of handles, and the following code passes the
AssociatedURLLoaderRequest to the factory before making any calls on
|url_loader_associated_ptr|.
> 
>   url_loader_factory_->CreateLoaderAndStart(
>       mojo::GetProxy(&url_loader_associated_ptr,
>                      url_loader_factory_.associated_group()),
>       original_info->GetRouteID(), request_id, request,
>       std::move(url_loader_client_associated_ptr_info));
> 
> Isn't that the comment in associated_group.h is requiring?

Yes, sorry I think I understand now. But it seems kind of pointless to proxy an
InterfacePtr through an AssociatedInterfacePtr, because messages sent on the
InterfacePtr will not be ordered at all with respect to the
AssociatedInterfacePtr's group.

> 
> > It would be helpful to understand what precisely you're trying to solve by
> > establishing ordering for loader IPCs. We might have a much simpler solution
> > (https://codereview.chromium.org/2455823002) that could be used without
needing
> > to spread associated interfaces everywhere. The linked CL provides a sort of
> > Channel-associated barrier that can signal over a non-associated interface
to
> > allow for ad hoc synchronization without requiring strict FIFO everywhere.
> Many existing IPC and associated mojo messages are relying on the Loading IPC
ordering. The issue is interactions with messages we don't know much. We don't
have the full list, and requiring strict FIFO is a easier way for this case I
believe.

Again, you're only getting ordering among URLLoaderFactory, URLLoader, and
URLLoaderClient. You are not getting FIFO on these interfaces with respect to
any other interfaces, or with respect to Chrome IPC. If you actually want strict
ordering, URLLoaderFactory needs to be a Channel-associated interface (and you
can't use the delegating impl solution either since it inherently breaks
ordering).

+yzshen@ since he may have some ideas about what to do as well

[yhirano, 2016-11-04 01:20:04]

On 2016/11/03 18:29:04, Ken Rockot wrote:
> On 2016/11/01 at 11:00:16, yhirano wrote:
> > > It's also worth noting that I don't think your delegating solution solves
> the
> > > problem of writing to an associated interface before its request end has
> been
> > > transferred. You bind both of the delegating impls immediately, so it's
> still
> > > possible for someone to (e.g.) write to the non-associated URLLoaderPtr,
and
> > > have that message be dispatched by the Delegating impl to the associated
> > > URLLoaderPtr, all before Fetch() has been called.
> > 
> > I'm confused: In my understanding, 
> > 
> >   mojo::GetProxy(&url_loader_factory_)
> > 
> > creates a pair of handles, and the following code passes the
> AssociatedURLLoaderRequest to the factory before making any calls on
> |url_loader_associated_ptr|.
> > 
> >   url_loader_factory_->CreateLoaderAndStart(
> >       mojo::GetProxy(&url_loader_associated_ptr,
> >                      url_loader_factory_.associated_group()),
> >       original_info->GetRouteID(), request_id, request,
> >       std::move(url_loader_client_associated_ptr_info));
> > 
> > Isn't that the comment in associated_group.h is requiring?
> 
> Yes, sorry I think I understand now. But it seems kind of pointless to proxy
an
> InterfacePtr through an AssociatedInterfacePtr, because messages sent on the
> InterfacePtr will not be ordered at all with respect to the
> AssociatedInterfacePtr's group.
> 
As I wrote in #39 and #65, we DO NOT NEED the ordering guarantee here (i.e., for
ServiceWorker).

> > 
> > > It would be helpful to understand what precisely you're trying to solve by
> > > establishing ordering for loader IPCs. We might have a much simpler
solution
> > > (https://codereview.chromium.org/2455823002) that could be used without
> needing
> > > to spread associated interfaces everywhere. The linked CL provides a sort
of
> > > Channel-associated barrier that can signal over a non-associated interface
> to
> > > allow for ad hoc synchronization without requiring strict FIFO everywhere.
> > Many existing IPC and associated mojo messages are relying on the Loading
IPC
> ordering. The issue is interactions with messages we don't know much. We don't
> have the full list, and requiring strict FIFO is a easier way for this case I
> believe.
> 
> Again, you're only getting ordering among URLLoaderFactory, URLLoader, and
> URLLoaderClient. You are not getting FIFO on these interfaces with respect to
> any other interfaces, or with respect to Chrome IPC. If you actually want
strict
> ordering, URLLoaderFactory needs to be a Channel-associated interface (and you
> can't use the delegating impl solution either since it inherently breaks
> ordering).
> 
> +yzshen@ since he may have some ideas about what to do as well

Again, we DO NOT NEED the ordering guarantee for ServiceWorker. As I wrote in
#65, the general loading usecase DO NEED the ordering guarantee, but it's using
an associated URLLoaderFactory via ResourceMessageFilter. Please see [1] and
[2].

1:
https://cs.chromium.org/chromium/src/content/browser/loader/resource_message_...
2:
https://cs.chromium.org/chromium/src/content/renderer/renderer_blink_platform...

[Ken Rockot(use gerrit already), 2016-11-04 01:26:20]

On 2016/11/04 at 01:20:04, yhirano wrote:
> On 2016/11/03 18:29:04, Ken Rockot wrote:
> > On 2016/11/01 at 11:00:16, yhirano wrote:
> > > > It's also worth noting that I don't think your delegating solution
solves
> > the
> > > > problem of writing to an associated interface before its request end has
> > been
> > > > transferred. You bind both of the delegating impls immediately, so it's
> > still
> > > > possible for someone to (e.g.) write to the non-associated URLLoaderPtr,
and
> > > > have that message be dispatched by the Delegating impl to the associated
> > > > URLLoaderPtr, all before Fetch() has been called.
> > > 
> > > I'm confused: In my understanding, 
> > > 
> > >   mojo::GetProxy(&url_loader_factory_)
> > > 
> > > creates a pair of handles, and the following code passes the
> > AssociatedURLLoaderRequest to the factory before making any calls on
> > |url_loader_associated_ptr|.
> > > 
> > >   url_loader_factory_->CreateLoaderAndStart(
> > >       mojo::GetProxy(&url_loader_associated_ptr,
> > >                      url_loader_factory_.associated_group()),
> > >       original_info->GetRouteID(), request_id, request,
> > >       std::move(url_loader_client_associated_ptr_info));
> > > 
> > > Isn't that the comment in associated_group.h is requiring?
> > 
> > Yes, sorry I think I understand now. But it seems kind of pointless to proxy
an
> > InterfacePtr through an AssociatedInterfacePtr, because messages sent on the
> > InterfacePtr will not be ordered at all with respect to the
> > AssociatedInterfacePtr's group.
> > 
> As I wrote in #39 and #65, we DO NOT NEED the ordering guarantee here (i.e.,
for ServiceWorker).
> 
> > > 
> > > > It would be helpful to understand what precisely you're trying to solve
by
> > > > establishing ordering for loader IPCs. We might have a much simpler
solution
> > > > (https://codereview.chromium.org/2455823002) that could be used without
> > needing
> > > > to spread associated interfaces everywhere. The linked CL provides a
sort of
> > > > Channel-associated barrier that can signal over a non-associated
interface
> > to
> > > > allow for ad hoc synchronization without requiring strict FIFO
everywhere.
> > > Many existing IPC and associated mojo messages are relying on the Loading
IPC
> > ordering. The issue is interactions with messages we don't know much. We
don't
> > have the full list, and requiring strict FIFO is a easier way for this case
I
> > believe.
> > 
> > Again, you're only getting ordering among URLLoaderFactory, URLLoader, and
> > URLLoaderClient. You are not getting FIFO on these interfaces with respect
to
> > any other interfaces, or with respect to Chrome IPC. If you actually want
strict
> > ordering, URLLoaderFactory needs to be a Channel-associated interface (and
you
> > can't use the delegating impl solution either since it inherently breaks
> > ordering).
> > 
> > +yzshen@ since he may have some ideas about what to do as well
> 
> Again, we DO NOT NEED the ordering guarantee for ServiceWorker. As I wrote in
#65, the general loading usecase DO NEED the ordering guarantee, but it's using
an associated URLLoaderFactory via ResourceMessageFilter. Please see [1] and
[2].
> 
> 1:
https://cs.chromium.org/chromium/src/content/browser/loader/resource_message_...
> 2:
https://cs.chromium.org/chromium/src/content/renderer/renderer_blink_platform...

I see. Sorry, long thread, lost track of some of the details. Lgtm.

[yhirano, 2016-11-04 01:27:47]

Thank you!

[yhirano, 2016-11-04 01:38:04]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-11-04 01:38:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2449933003/#ps540001
(title: "fix")

[commit-bot: I haz the power, 2016-11-04 01:39:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 01:50:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 01:50:28]

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...)

[yhirano, 2016-11-04 01:55:23]

yhirano@chromium.org changed reviewers:
+ dcheng@chromium.org

[yhirano, 2016-11-04 01:55:25]

+dcheng@ for content/common/url_loader_factory.mojom.

[dcheng, 2016-11-04 06:49:39]

mojom LGTM

https://codereview.chromium.org/2449933003/diff/540001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2449933003/diff/540001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:405: new
DelegatingURLLoaderClient(std::move(url_loader_client_ptr)));
Nit: auto url_loader_client = base::MakeUnique<DelegatingURLLoaderClient>(...);

[yhirano, 2016-11-04 07:09:05]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 07:09:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-04 07:14:08]

https://codereview.chromium.org/2449933003/diff/540001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2449933003/diff/540001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:405: new
DelegatingURLLoaderClient(std::move(url_loader_client_ptr)));
On 2016/11/04 06:49:39, dcheng wrote:
> Nit: auto url_loader_client =
base::MakeUnique<DelegatingURLLoaderClient>(...);

Done.

[commit-bot: I haz the power, 2016-11-04 07:20:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 07:20:24]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[yhirano, 2016-11-04 07:23:30]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 07:23:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 08:21:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 08:21:29]

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_...)

[yhirano, 2016-11-04 16:53:08]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-11-04 16:53:09]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, dcheng@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2449933003/#ps570001
(title: "fix")

[commit-bot: I haz the power, 2016-11-04 16:53:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 16:58:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 16:58:38]

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_...)

[yzshen1, 2016-11-04 17:50:09]

On 2016/11/03 18:29:04, Ken Rockot wrote:
> On 2016/11/01 at 11:00:16, yhirano wrote:
> > > It's also worth noting that I don't think your delegating solution solves
> the
> > > problem of writing to an associated interface before its request end has
> been
> > > transferred. You bind both of the delegating impls immediately, so it's
> still
> > > possible for someone to (e.g.) write to the non-associated URLLoaderPtr,
and
> > > have that message be dispatched by the Delegating impl to the associated
> > > URLLoaderPtr, all before Fetch() has been called.
> > 
> > I'm confused: In my understanding, 
> > 
> >   mojo::GetProxy(&url_loader_factory_)
> > 
> > creates a pair of handles, and the following code passes the
> AssociatedURLLoaderRequest to the factory before making any calls on
> |url_loader_associated_ptr|.
> > 
> >   url_loader_factory_->CreateLoaderAndStart(
> >       mojo::GetProxy(&url_loader_associated_ptr,
> >                      url_loader_factory_.associated_group()),
> >       original_info->GetRouteID(), request_id, request,
> >       std::move(url_loader_client_associated_ptr_info));
> > 
> > Isn't that the comment in associated_group.h is requiring?
> 
> Yes, sorry I think I understand now. But it seems kind of pointless to proxy
an
> InterfacePtr through an AssociatedInterfacePtr, because messages sent on the
> InterfacePtr will not be ordered at all with respect to the
> AssociatedInterfacePtr's group.
> 
> > 
> > > It would be helpful to understand what precisely you're trying to solve by
> > > establishing ordering for loader IPCs. We might have a much simpler
solution
> > > (https://codereview.chromium.org/2455823002) that could be used without
> needing
> > > to spread associated interfaces everywhere. The linked CL provides a sort
of
> > > Channel-associated barrier that can signal over a non-associated interface
> to
> > > allow for ad hoc synchronization without requiring strict FIFO everywhere.
> > Many existing IPC and associated mojo messages are relying on the Loading
IPC
> ordering. The issue is interactions with messages we don't know much. We don't
> have the full list, and requiring strict FIFO is a easier way for this case I
> believe.
> 
> Again, you're only getting ordering among URLLoaderFactory, URLLoader, and
> URLLoaderClient. You are not getting FIFO on these interfaces with respect to
> any other interfaces, or with respect to Chrome IPC. If you actually want
strict
> ordering, URLLoaderFactory needs to be a Channel-associated interface (and you
> can't use the delegating impl solution either since it inherently breaks
> ordering).
> 
> +yzshen@ since he may have some ideas about what to do as well

Sorry for didn't see this earlier.

I looked at the patchset 21 and the idea LGTM

[yhirano, 2016-11-07 01:19:20]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 01:19:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 04:04:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 04:04:45]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-07 04:08:21]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-11-07 04:08:21]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
rockot@chromium.org, jam@chromium.org, dcheng@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2449933003/#ps590001
(title: "fix")

[commit-bot: I haz the power, 2016-11-07 04:08:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 04:18:31]

Description was changed from

==========
Use Associated interfaces for mojo-loading

This CL makes URLLoader and URLLoaderClient associated for mojo-loading in
order to deal with the IPC ordering issue with the exising IPC messages.

BUG=603396
==========

to

==========
Use Associated interfaces for mojo-loading

This CL makes URLLoader and URLLoaderClient associated for mojo-loading in
order to deal with the IPC ordering issue with the exising IPC messages.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-07 04:18:35]

Committed patchset #22 (id:590001)

[commit-bot: I haz the power, 2016-11-07 04:20:49]

Description was changed from

==========
Use Associated interfaces for mojo-loading

This CL makes URLLoader and URLLoaderClient associated for mojo-loading in
order to deal with the IPC ordering issue with the exising IPC messages.

BUG=603396
==========

to

==========
Use Associated interfaces for mojo-loading

This CL makes URLLoader and URLLoaderClient associated for mojo-loading in
order to deal with the IPC ordering issue with the exising IPC messages.

BUG=603396

Committed: https://crrev.com/fe95d9b23ae824a85531260d87bb6f7897ea139c
Cr-Commit-Position: refs/heads/master@{#430216}
==========

[commit-bot: I haz the power, 2016-11-07 04:20:52]

Patchset 22 (id:??) landed as
https://crrev.com/fe95d9b23ae824a85531260d87bb6f7897ea139c
Cr-Commit-Position: refs/heads/master@{#430216}