Fix P2PSocketHostUdp to handle SetOptions() in error state

Fix P2PSocketHostUdp to handle SetOptions() in error state

Previously P2PSocketHostUdp was crashing when SetOptions is called
in error state.

BUG=714951
Review-Url: https://codereview.chromium.org/2843823002
Cr-Commit-Position: refs/heads/master@{#467395}
Committed: https://chromium.googlesource.com/chromium/src/+/2523efeb9a9d25f67c6d4799d0ca6687b674517f

[Sergey Ulanov, 2017-04-25 21:17:00]

sergeyu@chromium.org changed reviewers:
+ pthatcher@chromium.org

[pthatcher2, 2017-04-25 22:13:07]

lgtm

[Sergey Ulanov, 2017-04-25 22:49:12]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2017-04-25 22:49:32]

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, 2017-04-25 22:49:33]

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

[commit-bot: I haz the power, 2017-04-25 22:49:34]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[Sergey Ulanov, 2017-04-25 22:58:25]

sergeyu@chromium.org changed reviewers:
+ grunell@chromium.org

[Sergey Ulanov, 2017-04-25 22:58:25]

+grunell

[Henrik Grunell, 2017-04-26 03:12:53]

I'm not familiar enough with the code and usage to say if the assertion was
indeed incorrect, and that this is not an error elsewhere. I'm relying on that
you're sure about that Sergey.

Code change in itself looks fine.

lgtm

[Henrik Grunell, 2017-04-26 03:18:07]

On 2017/04/26 03:12:53, Henrik Grunell wrote:
> I'm not familiar enough with the code and usage to say if the assertion was
> indeed incorrect, and that this is not an error elsewhere. I'm relying on that
> you're sure about that Sergey.
> 
> Code change in itself looks fine.
> 
> lgtm

Actually, the description mentions handling the case of error state, but the
code handles all states. Is this function expected to be called in any state or
just open and error?

[Sergey Ulanov, 2017-04-26 17:43:06]

On 2017/04/26 03:18:07, Henrik Grunell wrote:
> On 2017/04/26 03:12:53, Henrik Grunell wrote:
> > I'm not familiar enough with the code and usage to say if the assertion was
> > indeed incorrect, and that this is not an error elsewhere. I'm relying on
that
> > you're sure about that Sergey.
> > 
> > Code change in itself looks fine.
> > 
> > lgtm
> 
> Actually, the description mentions handling the case of error state, but the
> code handles all states. Is this function expected to be called in any state
or
> just open and error?

The renderer doesn't get the socket ID until the socket is in OPEN state, so it
can only send 
P2PHostMsg_SetOption message when the socket is either in OPEN or ERROR state.
I've added a DCHECK to verify that it's in ERROR state when it isn't OPEN. We
still want to handle the case when state_ is not OPEN or ERROR in release builds
because the browser shouldn't trust the renderer to send only valid messages
(e.g. in case the renderer process is compromised and the attacker can guess
socket_id).

[Sergey Ulanov, 2017-04-26 17:44:10]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2017-04-26 17:44:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
grunell@chromium.org, pthatcher@chromium.org
Link to the patchset: https://codereview.chromium.org/2843823002/#ps20001
(title: "dcheck")

[commit-bot: I haz the power, 2017-04-26 17:45:01]

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, 2017-04-26 18:49:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493228650759040,
"parent_rev": "f677ae05d0b2e54b4876b3921a0dc1b9d66b4a21", "commit_rev":
"2523efeb9a9d25f67c6d4799d0ca6687b674517f"}

[commit-bot: I haz the power, 2017-04-26 18:49:47]

Description was changed from

==========
Fix P2PSocketHostUdp to handle SetOptions() in error state

Previously P2PSocketHostUdp was crashing when SetOptions is called
in error state.

BUG=714951
==========

to

==========
Fix P2PSocketHostUdp to handle SetOptions() in error state

Previously P2PSocketHostUdp was crashing when SetOptions is called
in error state.

BUG=714951

Review-Url: https://codereview.chromium.org/2843823002
Cr-Commit-Position: refs/heads/master@{#467395}
Committed:
https://chromium.googlesource.com/chromium/src/+/2523efeb9a9d25f67c6d4799d0ca...
==========

[commit-bot: I haz the power, 2017-04-26 18:49:48]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2523efeb9a9d25f67c6d4799d0ca...

[Henrik Grunell, 2017-04-27 15:29:33]

On 2017/04/26 17:43:06, Sergey Ulanov wrote:
> On 2017/04/26 03:18:07, Henrik Grunell wrote:
> > On 2017/04/26 03:12:53, Henrik Grunell wrote:
> > > I'm not familiar enough with the code and usage to say if the assertion
was
> > > indeed incorrect, and that this is not an error elsewhere. I'm relying on
> that
> > > you're sure about that Sergey.
> > > 
> > > Code change in itself looks fine.
> > > 
> > > lgtm
> > 
> > Actually, the description mentions handling the case of error state, but the
> > code handles all states. Is this function expected to be called in any state
> or
> > just open and error?
> 
> The renderer doesn't get the socket ID until the socket is in OPEN state, so
it
> can only send 
> P2PHostMsg_SetOption message when the socket is either in OPEN or ERROR state.
> I've added a DCHECK to verify that it's in ERROR state when it isn't OPEN. We
> still want to handle the case when state_ is not OPEN or ERROR in release
builds
> because the browser shouldn't trust the renderer to send only valid messages
> (e.g. in case the renderer process is compromised and the attacker can guess
> socket_id).

Ack.