Revert "Revert of Moving gRPC support interfaces out of cronet and into a new component. (patchset …

Reland refactor of gRPC. Previous CL was reverted because it used a fixed port for testing, which was sometimes occupied. The test now uses dynamically bound ports for Cronet as well as the grpc_support unittest.

Revert "Revert of Moving gRPC support interfaces out of cronet and into a new component. (patchset #67 id:1300001 of https://codereview.chromium.org/2273403003/ )"

This reverts commit 3b72ebe609598fa876ddd2af0b61636a225486c1.

TBR=blundell@chromium.org
BUG=650462
Committed: https://crrev.com/47cd6404f5dbcf436a4dd0e1685498da2ec3f4fe
Cr-Commit-Position: refs/heads/master@{#430995}

[Garrett Casto, 2016-11-01 22:00:02]

Patchset #1 (id:1) has been deleted

[Garrett Casto, 2016-11-02 21:08:02]

Description was changed from

==========
Revert "Revert of Moving gRPC support interfaces out of cronet and into a new
component. (patchset #67 id:1300001 of
https://codereview.chromium.org/2273403003/ )"

This reverts commit 3b72ebe609598fa876ddd2af0b61636a225486c1.

BUG=650462
==========

to

==========
Revert "Revert of Moving gRPC support interfaces out of cronet and into a new
component. (patchset #67 id:1300001 of
https://codereview.chromium.org/2273403003/ )"

This reverts commit 3b72ebe609598fa876ddd2af0b61636a225486c1.

TBR=blundell@chromium.org
BUG=650462
==========

[Garrett Casto, 2016-11-02 21:08:03]

gcasto@chromium.org changed reviewers:
+ mef@chromium.org

[Garrett Casto, 2016-11-02 21:09:42]

Description was changed from

==========
Revert "Revert of Moving gRPC support interfaces out of cronet and into a new
component. (patchset #67 id:1300001 of
https://codereview.chromium.org/2273403003/ )"

This reverts commit 3b72ebe609598fa876ddd2af0b61636a225486c1.

TBR=blundell@chromium.org
BUG=650462
==========

to

==========
Reland refactor of gRPC. Previous CL was reverted because it used a fixed port
for testing, which was sometimes occupied. The test now uses dynamically bound
ports for Cronet as well as the grpc_support unittest.

Revert "Revert of Moving gRPC support interfaces out of cronet and into a new
component. (patchset #67 id:1300001 of
https://codereview.chromium.org/2273403003/ )"

This reverts commit 3b72ebe609598fa876ddd2af0b61636a225486c1.

TBR=blundell@chromium.org
BUG=650462
==========

[Garrett Casto, 2016-11-02 21:10:07]

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

[commit-bot: I haz the power, 2016-11-02 21:10: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-02 22:37:26]

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

[commit-bot: I haz the power, 2016-11-02 22:37:27]

Dry run: This issue passed the CQ dry run.

[Garrett Casto, 2016-11-07 14:43:36]

Ping? I would like to re-land this soon.

[blundell, 2016-11-07 14:47:58]

On 2016/11/07 14:43:36, Garrett Casto wrote:
> Ping? I would like to re-land this soon.

I know you're not asking me to review, but it's not clear whether PS1 is the
original CL with the later patchsets being the new delta. That's always useful
in order to see what exactly has to be reviewed.

[mef, 2016-11-07 16:33:27]

Sorry for the delay, it took me a little bit to understand the difference.

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:296:
mapped_host_resolver->SetRulesFromString(host_resolver_rules_);
would those be always empty?

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:399: void
CronetEnvironment::SetHostResolverRules(const std::string& rules) {
Just to confirm my understanding, do we need to change host resolver rules in
order to specify actual port for the mapped host?

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:124: std::string
host_resolver_rules_;
I think |host_resolver_rules_| is no longer needed as we just set it, but never
use it.

[Garrett Casto, 2016-11-07 17:06:43]

Ah, yes sorry I didn't clarify that. The first patch set is the original
CL, and the later two are the necessary changes to make the tests work
correctly on dynamic ports.

On Mon, Nov 7, 2016 at 6:47 AM, <blundell@chromium.org> wrote:

> On 2016/11/07 14:43:36, Garrett Casto wrote:
> > Ping? I would like to re-land this soon.
>
> I know you're not asking me to review, but it's not clear whether PS1 is
> the
> original CL with the later patchsets being the new delta. That's always
> useful
> in order to see what exactly has to be reviewed.
>
> https://codereview.chromium.org/2470923002/
>

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

[Garrett Casto, 2016-11-07 17:23:11]

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:296:
mapped_host_resolver->SetRulesFromString(host_resolver_rules_);
On 2016/11/07 16:33:26, mef wrote:
> would those be always empty?

Not always. Currently the restriction in place is that you need to call [Cronet
setHostResolveRulesForTesting] after [Cronet start], but it's possible that the
former happens before CronetEnvironment::InitializeOnNetworkThread() is run. In
that case, SetHostResolverRules() will just store the rules and we will
initialize the mapped_host_resolver_ here. The alternative would be to have some
way to wait until initialization has been completed on the IO thread, but I
thought that this was cleaner.

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:399: void
CronetEnvironment::SetHostResolverRules(const std::string& rules) {
On 2016/11/07 16:33:26, mef wrote:
> Just to confirm my understanding, do we need to change host resolver rules in
> order to specify actual port for the mapped host?

Yeah. The problem is that we need to specify two things for the
URLRequestContext, the fact that a particular hostname speaks QUIC, and the fact
that we want a particular hostname to resolve to a certain port. If it were just
the latter constraint, we could have the test specify the url as
"test.example.com:%d/path" and just fill in the port dynamically. However
because of the QUIC constraint this will fail because the net stack will try to
talk TCP to the port and fail. There doesn't seem to be a way to say that you
want all communication to a particular domain to be QUIC, only for a specific
origin. 

So I think that we need to update the URLRequestContexts, and doing it this way
seem to be the easiest route since MappedHostResolver is thread safe so we don't
need to jump threads.

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:124: std::string
host_resolver_rules_;
On 2016/11/07 16:33:26, mef wrote:
> I think |host_resolver_rules_| is no longer needed as we just set it, but
never
> use it.

See my other comment. I think this is necessary to prevent a race condition on
initialization.

[mef, 2016-11-07 17:26:40]

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:401: if (main_context_) {
I think this is racy. :( 

It is possible that SetHostResolverRules is executed on the application thread
before CronetEnvironment::InitializeOnNetworkThread() gets executed on the
network thread.

Generally speaking, we shouldn't access |main_context_| and its members outside
of the network thread. Would it be better to post this to network thread?

[mef, 2016-11-07 17:49:54]

On 2016/11/07 17:26:40, mef wrote:
>
https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
> File components/cronet/ios/cronet_environment.cc (right):
> 
>
https://codereview.chromium.org/2470923002/diff/60001/components/cronet/ios/c...
> components/cronet/ios/cronet_environment.cc:401: if (main_context_) {
> I think this is racy. :( 
> 
> It is possible that SetHostResolverRules is executed on the application thread
> before CronetEnvironment::InitializeOnNetworkThread() gets executed on the
> network thread.
> 
> Generally speaking, we shouldn't access |main_context_| and its members
outside
> of the network thread. Would it be better to post this to network thread?

I've missed your explanation, I think posting
SetHostResolverRulesOnNetworkThread will be more consistent with the rest of the
code and safe regardless of implementation of MappedHostResolver.

I'll be ok to keep current implementation and just adding comment in
SetHostResolverRules though.

[chromium-reviews, 2016-11-07 18:09:10]

On Mon, Nov 7, 2016 at 9:49 AM, <mef@chromium.org> wrote:

> On 2016/11/07 17:26:40, mef wrote:
> >
> https://codereview.chromium.org/2470923002/diff/60001/
> components/cronet/ios/cronet_environment.cc
> > File components/cronet/ios/cronet_environment.cc (right):
> >
> >
> https://codereview.chromium.org/2470923002/diff/60001/
> components/cronet/ios/cronet_environment.cc#newcode401
> > components/cronet/ios/cronet_environment.cc:401: if (main_context_) {
> > I think this is racy. :(
> >
> > It is possible that SetHostResolverRules is executed on the application
> thread
> > before CronetEnvironment::InitializeOnNetworkThread() gets executed on
> the
> > network thread.
> >
> > Generally speaking, we shouldn't access |main_context_| and its members
> outside
> > of the network thread. Would it be better to post this to network thread?
>
> I've missed your explanation, I think posting
> SetHostResolverRulesOnNetworkThread will be more consistent with the rest
> of the
> code and safe regardless of implementation of MappedHostResolver.
>
>
I'll be ok to keep current implementation and just adding comment in
> SetHostResolverRules though.
>
>
No, you found an actual bug. |main_context_| could be non-null, but
main_context_->host_resolver() could still be null.  This works in the
grpc_support case because it holds on to an actual pointer to the
MappedHostResolver directly, but it could fail here. Changed to thread jump.


> https://codereview.chromium.org/2470923002/
>

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

[mef, 2016-11-07 18:15:21]

Thanks for changes!

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:404: event_.Wait();
Can event be allocated on the stack and passed as a bind argument? It should
remain valid until SetHostResolverRulesInternal completes.

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:122: void
SetHostResolverRulesInternal(const std::string& rules);
nit: Could you rename it to SetHostResolverRulesOnNetworkThread for consistency
with other methods?

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:131: base::WaitableEvent event_;
Does it have to be a member variable? If yes, can it have more descriptive name?

[Garrett Casto, 2016-11-07 18:37:26]

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.cc:404: event_.Wait();
On 2016/11/07 18:15:20, mef wrote:
> Can event be allocated on the stack and passed as a bind argument? It should
> remain valid until SetHostResolverRulesInternal completes.

Done.

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:122: void
SetHostResolverRulesInternal(const std::string& rules);
On 2016/11/07 18:15:21, mef wrote:
> nit: Could you rename it to SetHostResolverRulesOnNetworkThread for
consistency
> with other methods?

Done.

https://codereview.chromium.org/2470923002/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:131: base::WaitableEvent event_;
On 2016/11/07 18:15:21, mef wrote:
> Does it have to be a member variable? If yes, can it have more descriptive
name?

Removed.

[mef, 2016-11-07 18:44:51]

lgtm

https://codereview.chromium.org/2470923002/diff/100001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/2470923002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.h:121: // Sets host resolver rules on
the network_io_thread_.
nit: |pipes| around variable names. Maybe add explanation for |event|.

[Garrett Casto, 2016-11-09 17:43:01]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2016-11-09 17:43:20]

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-09 19:10:08]

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

[commit-bot: I haz the power, 2016-11-09 19:10:19]

Failed to apply patch for components/grpc_support/DEPS:
While running git apply --index -3 -p1;
  error: repository lacks the necessary blob to fall back on 3-way merge.
  error: chromecast/net/DEPS: patch does not apply

Patch:   NR  chromecast/net/DEPS->components/grpc_support/DEPS
Index: components/grpc_support/DEPS
diff --git a/chromecast/net/DEPS b/components/grpc_support/DEPS
similarity index 100%
copy from chromecast/net/DEPS
copy to components/grpc_support/DEPS

[commit-bot: I haz the power, 2016-11-09 19:17:10]

Description was changed from

==========
Reland refactor of gRPC. Previous CL was reverted because it used a fixed port
for testing, which was sometimes occupied. The test now uses dynamically bound
ports for Cronet as well as the grpc_support unittest.

Revert "Revert of Moving gRPC support interfaces out of cronet and into a new
component. (patchset #67 id:1300001 of
https://codereview.chromium.org/2273403003/ )"

This reverts commit 3b72ebe609598fa876ddd2af0b61636a225486c1.

TBR=blundell@chromium.org
BUG=650462
==========

to

==========
Reland refactor of gRPC. Previous CL was reverted because it used a fixed port
for testing, which was sometimes occupied. The test now uses dynamically bound
ports for Cronet as well as the grpc_support unittest.

Revert "Revert of Moving gRPC support interfaces out of cronet and into a new
component. (patchset #67 id:1300001 of
https://codereview.chromium.org/2273403003/ )"

This reverts commit 3b72ebe609598fa876ddd2af0b61636a225486c1.

TBR=blundell@chromium.org
BUG=650462

Committed: https://crrev.com/47cd6404f5dbcf436a4dd0e1685498da2ec3f4fe
Cr-Commit-Position: refs/heads/master@{#430995}
==========

[commit-bot: I haz the power, 2016-11-09 19:17:11]

Patchset 5 (id:??) landed as
https://crrev.com/47cd6404f5dbcf436a4dd0e1685498da2ec3f4fe
Cr-Commit-Position: refs/heads/master@{#430995}

[Dirk Pranke, 2016-11-09 19:48:23]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2484263005/ by dpranke@chromium.org.

The reason for reverting is: Looks like this broke component_unittests on
Android and Windows:

https://build.chromium.org/p/chromium.android/builders/Android%20N5X%20Swarm%...
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui...

so I'm going to revert this. Sorry!.

[Lei Zhang, 2016-11-09 21:37:37]

On 2016/11/09 19:48:23, Dirk Pranke wrote:
> A revert of this CL (patchset #5 id:100001) has been created in
> https://codereview.chromium.org/2484263005/ by mailto:dpranke@chromium.org.
> 
> The reason for reverting is: Looks like this broke component_unittests on
> Android and Windows:
> 
>
https://build.chromium.org/p/chromium.android/builders/Android%20N5X%20Swarm%...
>
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui...
> 
> so I'm going to revert this. Sorry!.

And here's a broken Linux bot too:
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes...

Not sure why it passed on all the trybots. Maybe try locally with
--single-process-tests, and maybe also --gtest_shuffle for more fun?

[chromium-reviews, 2016-11-09 21:47:27]

I can't get this to reproduce running locally either with
--single-process-tests and/or --gtest_shuffle, and obviously it never
showed up on the trybots. Is there anything else that the waterfall does
differently from the trybots that I could use to help pinpoint what is
going on here?

On Wed, Nov 9, 2016 at 1:37 PM, <thestig@chromium.org> wrote:

> On 2016/11/09 19:48:23, Dirk Pranke wrote:
> > A revert of this CL (patchset #5 id:100001) has been created in
> > https://codereview.chromium.org/2484263005/ by mailto:
> dpranke@chromium.org.
> >
> > The reason for reverting is: Looks like this broke component_unittests on
> > Android and Windows:
> >
> >
> https://build.chromium.org/p/chromium.android/builders/
> Android%20N5X%20Swarm%20Builder/builds/6152
> >
> https://build.chromium.org/p/chromium.win/builders/Win7%
> 20Tests%20%281%29/builds/59755
> >
> > so I'm going to revert this. Sorry!.
>
> And here's a broken Linux bot too:
> https://build.chromium.org/p/chromium.memory.full/builders/
> Linux%20MSan%20Tests/builds/3106
>
> Not sure why it passed on all the trybots. Maybe try locally with
> --single-process-tests, and maybe also --gtest_shuffle for more fun?
>
> https://codereview.chromium.org/2470923002/
>

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

[mikecase (-- gone --), 2016-11-09 23:45:08]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2487863003/ by mikecase@chromium.org.

The reason for reverting is: Causing components_unittests failure on 

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test....