Convert a bunch of resource IPCs to be control messages. They didn't need to be routed.

Convert a bunch of resource IPCs to be control messages. They didn't need to be routed.

BUG=286074
R=ajwong@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221751

[awong, 2013-09-06 17:04:11]

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:849: handled = true;
If I read this correctly, any ResourceHostMsg will be considered handled now.
That feels overly broad.  Can this be restricted to just the set of ack messages
that we expect to be racy?

Also, is this a problem for all _ACK messages?  If not, then I think the comment
could use a little more detail.

[jam, 2013-09-06 17:12:04]

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:849: handled = true;
On 2013/09/06 17:04:11, awong wrote:
> If I read this correctly, any ResourceHostMsg will be considered handled now.

right

> That feels overly broad.  Can this be restricted to just the set of ack
messages
> that we expect to be racy?

I didn't want to play whac-a-mole with different resource IPCs triggering that
dcheck. I couldn't think of any issues of handling all IPCs here. can you?

Another alternative is to remove that dcheck in RenderProcessHost. It seems odd
that we dcheck for control IPCs, but not routed.

> 
> Also, is this a problem for all _ACK messages?  If not, then I think the
comment
> could use a little more detail.

I saw this for ResourceHostMsg_DataReceived_ACK, but it wasn't clear to me what
other IPCs could trigger this.

[jam, 2013-09-06 17:22:17]

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:849: handled = true;
On 2013/09/06 17:12:04, jam wrote:
> On 2013/09/06 17:04:11, awong wrote:
> > If I read this correctly, any ResourceHostMsg will be considered handled
now.
> 
> right
> 
> > That feels overly broad.  Can this be restricted to just the set of ack
> messages
> > that we expect to be racy?
> 
> I didn't want to play whac-a-mole with different resource IPCs triggering that
> dcheck. I couldn't think of any issues of handling all IPCs here. can you?
> 
> Another alternative is to remove that dcheck in RenderProcessHost. It seems
odd
> that we dcheck for control IPCs, but not routed.

btw I've uploaded a new patchset that does this now. the more I think about it,
the more that seems better. wdyt?

> 
> > 
> > Also, is this a problem for all _ACK messages?  If not, then I think the
> comment
> > could use a little more detail.
> 
> I saw this for ResourceHostMsg_DataReceived_ACK, but it wasn't clear to me
what
> other IPCs could trigger this.

[awong, 2013-09-06 17:37:34]

LGTM (with large comment)

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:849: handled = true;
On 2013/09/06 17:22:17, jam wrote:
> On 2013/09/06 17:12:04, jam wrote:
> > On 2013/09/06 17:04:11, awong wrote:
> > > If I read this correctly, any ResourceHostMsg will be considered handled
> now.
> > 
> > right
> > 
> > > That feels overly broad.  Can this be restricted to just the set of ack
> > messages
> > > that we expect to be racy?
> > 
> > I didn't want to play whac-a-mole with different resource IPCs triggering
that
> > dcheck. I couldn't think of any issues of handling all IPCs here. can you?
> > 
> > Another alternative is to remove that dcheck in RenderProcessHost. It seems
> odd
> > that we dcheck for control IPCs, but not routed.
> 
> btw I've uploaded a new patchset that does this now. the more I think about
it,
> the more that seems better. wdyt?

It's consistent now so that bit is better.

However, I was personally would have expected that we should go the other
direction -- all unhandled IPCs result in an error rather than just black
holing.  With this change the failure mode becomes a timeout/hang instead of a
crash which is more annoying to debug.  It also seems like it might mask people
making having non-benign cross-process synchronization and races (even though
the case you cited may be benign, but I worry others might not).

I was actually hoping to error on unhandled messages for RenderFrameHost's
OnMessageReceived, but maybe that policy to too strict for this level of the
stack?

Having said all that, I don't have a super strong opinion on this so whatever
you think is best.




> 
> > 
> > > 
> > > Also, is this a problem for all _ACK messages?  If not, then I think the
> > comment
> > > could use a little more detail.
> > 
> > I saw this for ResourceHostMsg_DataReceived_ACK, but it wasn't clear to me
> what
> > other IPCs could trigger this.
>

[jam, 2013-09-06 18:00:20]

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:849: handled = true;
On 2013/09/06 17:37:34, awong wrote:
> On 2013/09/06 17:22:17, jam wrote:
> > On 2013/09/06 17:12:04, jam wrote:
> > > On 2013/09/06 17:04:11, awong wrote:
> > > > If I read this correctly, any ResourceHostMsg will be considered handled
> > now.
> > > 
> > > right
> > > 
> > > > That feels overly broad.  Can this be restricted to just the set of ack
> > > messages
> > > > that we expect to be racy?
> > > 
> > > I didn't want to play whac-a-mole with different resource IPCs triggering
> that
> > > dcheck. I couldn't think of any issues of handling all IPCs here. can you?
> > > 
> > > Another alternative is to remove that dcheck in RenderProcessHost. It
seems
> > odd
> > > that we dcheck for control IPCs, but not routed.
> > 
> > btw I've uploaded a new patchset that does this now. the more I think about
> it,
> > the more that seems better. wdyt?
> 
> It's consistent now so that bit is better.
> 
> However, I was personally would have expected that we should go the other
> direction -- all unhandled IPCs result in an error rather than just black
> holing.  With this change the failure mode becomes a timeout/hang instead of a
> crash which is more annoying to debug.  It also seems like it might mask
people
> making having non-benign cross-process synchronization and races (even though
> the case you cited may be benign, but I worry others might not).

Given the asynchronous nature of our code base, I think we naturally have a lot
of handled IPCs around teardown. We've been ignoring them all for now, other
than control IPCs. I guess we can come up with alternate ways of ignoring them
while not ignoring the non-teardown case, perhaps by telling the channel to
ignore route X from now on every time an object is deleted. Before we go down
that extra complexity, I'd prefer to see examples of bugs that happened because
IPCs were dropped silently.

> 
> I was actually hoping to error on unhandled messages for RenderFrameHost's
> OnMessageReceived, but maybe that policy to too strict for this level of the
> stack?
> 
> Having said all that, I don't have a super strong opinion on this so whatever
> you think is best.
> 
> 
> 
> 
> > 
> > > 
> > > > 
> > > > Also, is this a problem for all _ACK messages?  If not, then I think the
> > > comment
> > > > could use a little more detail.
> > > 
> > > I saw this for ResourceHostMsg_DataReceived_ACK, but it wasn't clear to me
> > what
> > > other IPCs could trigger this.
> >

[awong, 2013-09-06 18:01:58]

On Fri, Sep 6, 2013 at 11:00 AM, <jam@chromium.org> wrote:

>
> https://codereview.chromium.**org/23583039/diff/35001/**
>
content/browser/loader/**resource_dispatcher_host_impl.**cc<https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/resource_dispatcher_host_impl.cc>
> File content/browser/loader/**resource_dispatcher_host_impl.**cc (right):
>
> https://codereview.chromium.**org/23583039/diff/35001/**
>
content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode849<https://codereview.chromium.org/23583039/diff/35001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode849>
> content/browser/loader/**resource_dispatcher_host_impl.**cc:849: handled =
> true;
> On 2013/09/06 17:37:34, awong wrote:
>
>> On 2013/09/06 17:22:17, jam wrote:
>> > On 2013/09/06 17:12:04, jam wrote:
>> > > On 2013/09/06 17:04:11, awong wrote:
>> > > > If I read this correctly, any ResourceHostMsg will be considered
>>
> handled
>
>> > now.
>> > >
>> > > right
>> > >
>> > > > That feels overly broad.  Can this be restricted to just the set
>>
> of ack
>
>> > > messages
>> > > > that we expect to be racy?
>> > >
>> > > I didn't want to play whac-a-mole with different resource IPCs
>>
> triggering
>
>> that
>> > > dcheck. I couldn't think of any issues of handling all IPCs here.
>>
> can you?
>
>> > >
>> > > Another alternative is to remove that dcheck in RenderProcessHost.
>>
> It seems
>
>> > odd
>> > > that we dcheck for control IPCs, but not routed.
>> >
>> > btw I've uploaded a new patchset that does this now. the more I
>>
> think about
>
>> it,
>> > the more that seems better. wdyt?
>>
>
>  It's consistent now so that bit is better.
>>
>
>  However, I was personally would have expected that we should go the
>>
> other
>
>> direction -- all unhandled IPCs result in an error rather than just
>>
> black
>
>> holing.  With this change the failure mode becomes a timeout/hang
>>
> instead of a
>
>> crash which is more annoying to debug.  It also seems like it might
>>
> mask people
>
>> making having non-benign cross-process synchronization and races (even
>>
> though
>
>> the case you cited may be benign, but I worry others might not).
>>
>
> Given the asynchronous nature of our code base, I think we naturally
> have a lot of handled IPCs around teardown. We've been ignoring them all
> for now, other than control IPCs. I guess we can come up with alternate
> ways of ignoring them while not ignoring the non-teardown case, perhaps
> by telling the channel to ignore route X from now on every time an
> object is deleted. Before we go down that extra complexity, I'd prefer
> to see examples of bugs that happened because IPCs were dropped
> silently.


SGTM. :)

Thanks for the context.


>
>
>
>  I was actually hoping to error on unhandled messages for
>>
> RenderFrameHost's
>
>> OnMessageReceived, but maybe that policy to too strict for this level
>>
> of the
>
>> stack?
>>
>
>  Having said all that, I don't have a super strong opinion on this so
>>
> whatever
>
>> you think is best.
>>
>
>
>
>
>  >
>> > >
>> > > >
>> > > > Also, is this a problem for all _ACK messages?  If not, then I
>>
> think the
>
>> > > comment
>> > > > could use a little more detail.
>> > >
>> > > I saw this for ResourceHostMsg_DataReceived_**ACK, but it wasn't
>>
> clear to me
>
>> > what
>> > > other IPCs could trigger this.
>> >
>>
>
>
https://codereview.chromium.**org/23583039/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sjkenny69, 2015-09-06 22:56:44]

sjkenny69@gmail.com changed reviewers:
+ sjkenny69@gmail.com

[sjkenny69, 2015-09-06 22:56:48]

lgtm

[sjkenny69, 2015-09-06 22:56:50]

lgtm

lgtm

[sjkenny69, 2015-09-06 22:59:07]

lgtm

[sjkenny69, 2015-09-06 22:59:08]

lgtm

lgtm

[missmanno2626, 2016-07-11 05:37:19]

missmanno2626@gmail.com changed reviewers:
+ missmanno2626@gmail.com

[missmanno2626, 2016-07-11 05:37:20]

lgtm

https://codereview.chromium.org/23583039/

[onedeep3331, 2016-12-06 16:44:05]

onedeep3331@gmail.com changed reviewers:
+ onedeep3331@gmail.com

[onedeep3331, 2016-12-06 16:44:09]

lgtm

[onedeep3331, 2016-12-06 17:01:39]

lgtm

[daniellepalmer1993, 2017-06-23 04:02:02]

daniellepalmer1993@gmail.com changed reviewers:
+ daniellepalmer1993@gmail.com

[daniellepalmer1993, 2017-06-23 04:02:06]

lgtm

[daniellepalmer1993, 2017-06-23 04:02:09]

lgtm

lgtm

[daniellepalmer1993, 2017-06-23 04:07:10]

Hey hey