Use WeakPtrFactory in net::BidirectionalStream when posting task

Use WeakPtrFactory in net::BidirectionalStream when posting task

This CL uses WeakPtrFactory to post task in
net::BidirectionalStream.

BUG=606394
Committed: https://crrev.com/3b6ac70b2c8b7085bade85f46577677a45732005
Cr-Commit-Position: refs/heads/master@{#398132}

[xunjieli, 2016-06-06 19:15:33]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org

[xunjieli, 2016-06-06 19:15:33]

Hi Matt,

Sorry for burdening you yet again with another CL review! I wonder if this is
the correct use of WeakPtrFactory, and whether there is any life-time issues
that I've missed. Some context: I have fixed the underlying
BidirectionalStreamImpl to not call BidirectionalStreamImpl::OnFailed twice in
crrev.com/2032733002.

Thanks a lot in advance!

[mmenke, 2016-06-06 19:27:53]

On 2016/06/06 19:15:33, xunjieli wrote:
> Hi Matt,
> 
> Sorry for burdening you yet again with another CL review! I wonder if this is
> the correct use of WeakPtrFactory, and whether there is any life-time issues
> that I've missed. Some context: I have fixed the underlying
> BidirectionalStreamImpl to not call BidirectionalStreamImpl::OnFailed twice in
> crrev.com/2032733002.
> 
> Thanks a lot in advance!

Is there a filed bug for this?

[xunjieli, 2016-06-06 19:29:24]

On 2016/06/06 19:27:53, mmenke wrote:
> On 2016/06/06 19:15:33, xunjieli wrote:
> > Hi Matt,
> > 
> > Sorry for burdening you yet again with another CL review! I wonder if this
is
> > the correct use of WeakPtrFactory, and whether there is any life-time issues
> > that I've missed. Some context: I have fixed the underlying
> > BidirectionalStreamImpl to not call BidirectionalStreamImpl::OnFailed twice
in
> > crrev.com/2032733002.
> > 
> > Thanks a lot in advance!
> 
> Is there a filed bug for this?

No, there is not. For the previous CL, the crbug is crbug.com/606394

[mmenke, 2016-06-06 19:34:00]

LGTM

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.cc:109: weak_factory_.GetWeakPtr(),
ERR_DISALLOWED_URL_SCHEME));
You may want a test for this, since this could cause actual crashes in
production code - create a bidirectional stream for a non-HTTPS URL, then delete
the BidirectionalStream without waiting, and the spin the message loop.  Could
also do one where you wait for the response, if you don't have one already.

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.cc:352: void BidirectionalStream::NotifyError(int
error) {
nit:  May want to call this NotifyFailed instead, to match the name of the
delegate's method.

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.h:14: #include "base/memory/ref_counted.h"
include base/memory/weak_ptr.h.

[xunjieli, 2016-06-06 19:50:48]

Description was changed from

==========
Use WeakPtrFactory in net::BidirectionalStream when posting task

This CL uses WeakPtrFactory to post task in net::BidirectionalStream.
==========

to

==========
Use WeakPtrFactory in net::BidirectionalStream when posting task

This CL uses WeakPtrFactory to post task in 
net::BidirectionalStream.

BUG=606394
==========

[xunjieli, 2016-06-06 19:52:59]

Thanks a lot for the review! This is very helpful.

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
File net/http/bidirectional_stream.cc (right):

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.cc:109: weak_factory_.GetWeakPtr(),
ERR_DISALLOWED_URL_SCHEME));
On 2016/06/06 19:34:00, mmenke wrote:
> You may want a test for this, since this could cause actual crashes in
> production code - create a bidirectional stream for a non-HTTPS URL, then
delete
> the BidirectionalStream without waiting, and the spin the message loop.  Could
> also do one where you wait for the response, if you don't have one already.

Done. I have a unit where I wait for the response. I added a new one where we
don't wait, and this crashes without the patch. Could you take another look?
Thanks!

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.cc:352: void BidirectionalStream::NotifyError(int
error) {
On 2016/06/06 19:34:00, mmenke wrote:
> nit:  May want to call this NotifyFailed instead, to match the name of the
> delegate's method.

Done.

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/2043863002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.h:14: #include "base/memory/ref_counted.h"
On 2016/06/06 19:34:00, mmenke wrote:
> include base/memory/weak_ptr.h.

Done.

[mmenke, 2016-06-06 19:58:25]

The new test LGTM

[xunjieli, 2016-06-06 19:59:11]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-06-06 19:59:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043863002/20001

[commit-bot: I haz the power, 2016-06-06 21:36:42]

Description was changed from

==========
Use WeakPtrFactory in net::BidirectionalStream when posting task

This CL uses WeakPtrFactory to post task in 
net::BidirectionalStream.

BUG=606394
==========

to

==========
Use WeakPtrFactory in net::BidirectionalStream when posting task

This CL uses WeakPtrFactory to post task in 
net::BidirectionalStream.

BUG=606394
==========

[commit-bot: I haz the power, 2016-06-06 21:36:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-06 21:38:03]

Description was changed from

==========
Use WeakPtrFactory in net::BidirectionalStream when posting task

This CL uses WeakPtrFactory to post task in 
net::BidirectionalStream.

BUG=606394
==========

to

==========
Use WeakPtrFactory in net::BidirectionalStream when posting task

This CL uses WeakPtrFactory to post task in
net::BidirectionalStream.

BUG=606394

Committed: https://crrev.com/3b6ac70b2c8b7085bade85f46577677a45732005
Cr-Commit-Position: refs/heads/master@{#398132}
==========

[commit-bot: I haz the power, 2016-06-06 21:38:05]

Patchset 2 (id:??) landed as
https://crrev.com/3b6ac70b2c8b7085bade85f46577677a45732005
Cr-Commit-Position: refs/heads/master@{#398132}