Change StunMessage::AddAttribute return type from bool to void.

Change StunMessage::AddAttribute return type from bool to void.

Proper error handling was missing, using VERIFY to crash in debug
builds, while release builds would ignore the error and leak the
attribute memory. The check of attribute type consistency was changed
to a RTC_DCHECK.

Also removes a large number of uses of the deprecated VERIFY macro.

BUG=webrtc:6424
Review-Url: https://codereview.webrtc.org/2665343002
Cr-Commit-Position: refs/heads/master@{#16413}
Committed: https://chromium.googlesource.com/external/webrtc/+/cc99bc25d8bc8acc944b31d7ebcb5cb54b8fd2c7

[nisse-webrtc, 2017-02-01 10:04:12]

nisse@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, pthatcher@webrtc.org

[kwiberg-webrtc, 2017-02-01 10:40:17]

Looks good, but I have a question.

https://codereview.webrtc.org/2665343002/diff/1/webrtc/p2p/base/stun.cc
File webrtc/p2p/base/stun.cc (right):

https://codereview.webrtc.org/2665343002/diff/1/webrtc/p2p/base/stun.cc#newco...
webrtc/p2p/base/stun.cc:79: 
Old non-debug behavior for wrong attr type was to return early, leaking attr and
having the caller ignore the error. New non-debug behavior is to go ahead
anyway. What effect will this have?

[pthatcher2, 2017-02-01 22:15:05]

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

[pthatcher2, 2017-02-01 22:15:06]

LGTM 

I'd prefer fixing the leak, but I'd be fine if that's in another CL (or just
done later).

https://codereview.webrtc.org/2665343002/diff/1/webrtc/p2p/base/stun.cc
File webrtc/p2p/base/stun.cc (right):

https://codereview.webrtc.org/2665343002/diff/1/webrtc/p2p/base/stun.cc#newco...
webrtc/p2p/base/stun.cc:79: 
On 2017/02/01 10:40:17, kwiberg-webrtc wrote:
> Old non-debug behavior for wrong attr type was to return early, leaking attr
and
> having the caller ignore the error. New non-debug behavior is to go ahead
> anyway. What effect will this have?

I think a DCHECK is sufficient.  I think there is no code which is passing in
the wrong type combination.  

It might be a good idea to use a std::unique_ptr to avoid the leak, even if
changing all calls sites would be a pain.

[nisse-webrtc, 2017-02-02 07:59:46]

On 2017/02/01 22:15:06, pthatcher2 wrote:
> LGTM 

Thanks for the review. I'm not at all familiar with this part of webrtc.

> I'd prefer fixing the leak, but I'd be fine if that's in another CL (or just
> done later).

I think this change does fix the leak; the StunMessage always takes over
ownership.

> I think a DCHECK is sufficient.  I think there is no code which is passing in
> the wrong type combination.  

That's my gut feeling too, types passed in should be static at each call site,
and hence not likely to pass tests but break badly in the field.

> It might be a good idea to use a std::unique_ptr to avoid the leak, even if
> changing all calls sites would be a pain.

Using unique_ptr would improve clarity, but that's an orthogonal change I'm not
planning to do. Even with unique_ptr, I think it simplifies the code to
eliminate the return value and treat any problems in AddAttribute as fatal
errors.

[kwiberg-webrtc, 2017-02-02 08:39:01]

lgtm

https://codereview.webrtc.org/2665343002/diff/1/webrtc/p2p/base/stun.cc
File webrtc/p2p/base/stun.cc (right):

https://codereview.webrtc.org/2665343002/diff/1/webrtc/p2p/base/stun.cc#newco...
webrtc/p2p/base/stun.cc:79: 
On 2017/02/01 22:15:06, pthatcher2 wrote:
> On 2017/02/01 10:40:17, kwiberg-webrtc wrote:
> > Old non-debug behavior for wrong attr type was to return early, leaking attr
> and
> > having the caller ignore the error. New non-debug behavior is to go ahead
> > anyway. What effect will this have?
> 
> I think a DCHECK is sufficient.  I think there is no code which is passing in
> the wrong type combination.  
> 
> It might be a good idea to use a std::unique_ptr to avoid the leak, even if
> changing all calls sites would be a pain. 

Well then. If no caller does the wrong thing, we're A-OK!

[nisse-webrtc, 2017-02-02 08:49:48]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-02-02 08:49:58]

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

[commit-bot: I haz the power, 2017-02-02 09:31:31]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486025388053430, "parent_rev":
"f7826d668a7777f8d85c8444278a4c72c45ee8ef", "commit_rev":
"cc99bc25d8bc8acc944b31d7ebcb5cb54b8fd2c7"}

[commit-bot: I haz the power, 2017-02-02 09:31:34]

Description was changed from

==========
Change StunMessage::AddAttribute return type from bool to void.

Proper error handling was missing, using VERIFY to crash in debug
builds, while release builds would ignore the error and leak the
attribute memory. The check of attribute type consistency was changed
to a RTC_DCHECK.

Also removes a large number of uses of the deprecated VERIFY macro.

BUG=webrtc:6424
==========

to

==========
Change StunMessage::AddAttribute return type from bool to void.

Proper error handling was missing, using VERIFY to crash in debug
builds, while release builds would ignore the error and leak the
attribute memory. The check of attribute type consistency was changed
to a RTC_DCHECK.

Also removes a large number of uses of the deprecated VERIFY macro.

BUG=webrtc:6424

Review-Url: https://codereview.webrtc.org/2665343002
Cr-Commit-Position: refs/heads/master@{#16413}
Committed:
https://chromium.googlesource.com/external/webrtc/+/cc99bc25d8bc8acc944b31d7e...
==========

[commit-bot: I haz the power, 2017-02-02 09:31:35]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/cc99bc25d8bc8acc944b31d7e...