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

Issue 869551 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 6
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 866708



Sign in to add a comment

Some services tests break when change Mojo C++ bindings dispatch scheduling

Project Member Reported by roc...@chromium.org, Jul 31

Issue description

When applying this CL[1], a few services_unittests tests break for unknown (not yet investigated) reasons. They are:

ProxyResolverFactoryMojoTest.CreateProxyResolver_Failed
NetworkContextCertTransparencyAuditingDisabledTest.SCTsAreNotCheckedForInclusion
NetworkServiceTestWithService.DestroyingPrimaryNetworkContextDestroysOtherContexts
NetworkContextCertTransparencyAuditingEnabledTest.SCTsAreCheckedForInclusion

These need to be fixed for the CL to be landed.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1145692
 
Cc: reillyg@chromium.org jam@chromium.org mmenke@chromium.org
+CC some network service folks

I investigated the proxy resolver one but haven't looked at the others yet. For proxy resolver the issue is that the ProxyResolverPtr and Binding<ProxyResolverFactoryRequestClient> endpoints owned by a ProxyResolverFactoryMojo are not mutually ordered, but the code is written to assume that they are. Namely, it is possible for a client to in this order:

  1. Call ProxyResolverFactoryRequestClient.ReportResult() on its Ptr
  2. Close its ProxyResolver binding

but for the service to receive a connection error on the ProxyResolverPtr before the ReportResult message is dispatched.

I am reasonably confident that the other failures are similarly timing/FIFO issues, only exposed once we try to change how dispatch works. Please see the blocked bug and linked CL from #1 for more details on what is changing and why.

Can someone more familiar with network service look at these tests? I think it's reasonably important for performance reasons that we land the Mojo scheduling change ASAP.
Cc: roc...@chromium.org
Owner: mmenke@chromium.org
I'll look into these.
Status: Started (was: Assigned)
CLs are out to fix all three - one's an actual bug in production code (Though a minor one)
Erm...All four CLs, rather.  Only 3 CLs, since two are the same issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 2

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

commit e1e19180d987d019406963d675a7aaf9af6ee53a
Author: Matt Menke <mmenke@chromium.org>
Date: Thu Aug 02 10:44:03 2018

Fix test that deleting the primary NetworkContext destroys all others.

The test wasn't waiting for the secondary NetworkContext to be created,
so it could end up that the secondary NetworkContext was created only
after the primary one was destroyed.

Bug:  869551 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Iaefbb98553175cf4e3784017adafe227888c588b
Reviewed-on: https://chromium-review.googlesource.com/1159588
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580127}
[modify] https://crrev.com/e1e19180d987d019406963d675a7aaf9af6ee53a/services/network/network_service_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 2

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

commit 2f0bee383d08b917bf82d45a4686f0c441aeec0a
Author: Matt Menke <mmenke@chromium.org>
Date: Thu Aug 02 10:45:16 2018

Fix bug in NetworkContext Certificate Transparency tests.

The tests were waiting for a request to receive headers, but then
assuming that the request had in fact completed receiving the entire
response, including the body.

Bug:  869551 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I2771bb0f786dd18e74423941364d51607fb8b7f8
Reviewed-on: https://chromium-review.googlesource.com/1159587
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580129}
[modify] https://crrev.com/2f0bee383d08b917bf82d45a4686f0c441aeec0a/services/network/network_context_cert_transparency_unittest.cc

(Note that I've run into what may or may not be a Mojo bug in my third CL - want to figure that out before landing anything, since it does fix a real bug)
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 6

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

commit 57639bd5ec9e69c5d59ca77e13d647a5f1871242
Author: Matt Menke <mmenke@chromium.org>
Date: Mon Aug 06 21:41:08 2018

ProxyResolverFactoryMojo::Job:  Only listen to errors on client pointer.

When there's an error, the ProxyResolver would invoke a method on the
ProxyResolverFactoryRequestClient pipe to pass along an error code and
close both that pipe and the ProxyResolver pipe. The Job was listening
for error on both pipes, so could see the ProxyResolver pipe close
before getting a more useful error code on the
ProxyResolverFactoryRequestClient pipe.

This CL fixes that by just listening for the client pipe to close.

Also, this CL closes the client pipe on completion, to protect against
unexpected extra messages, if the Job isn't deleted immediately.

Bug:  869551 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I2a1942d851e1d2cc4a33b0fb1278d914aac3593f
Reviewed-on: https://chromium-review.googlesource.com/1159196
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580996}
[modify] https://crrev.com/57639bd5ec9e69c5d59ca77e13d647a5f1871242/services/network/proxy_resolver_factory_mojo.cc
[modify] https://crrev.com/57639bd5ec9e69c5d59ca77e13d647a5f1871242/services/network/proxy_resolver_factory_mojo_unittest.cc

Status: Fixed (was: Started)
Thank you for looking to these and fixing them so quickly!

Sign in to add a comment