Cronet: Fix CronetUploadTest.testDestroyAdapterBefore* tests.

Cronet:  Fix CronetUploadTest.testDestroyAdapterBefore* tests.

The tests weren't actually destroying the adapter when they claimed.
As a result, the final read/rewind call could happen on the "network"
thread between starting the destroy task in Shutdown and actually
tearing down the network thread, at which point the
TestUploadDataStreamHandler network thread pointer is NULL, resulting in
a crash.

The fix is just to destroy the adapters when they should be, at which
point there should be no messages pending on the network thread.  May be
worth revising how thread checks are done if more tests are added.

BUG=480623
Committed: https://crrev.com/ea69e72231b085781569d037bd61a583d16a5912
Cr-Commit-Position: refs/heads/master@{#326852}

[mmenke, 2015-04-24 17:16:56]

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

[xunjieli, 2015-04-24 18:01:10]

I didn't realize that we passed the ownership of the adapter to the handler. I
somehow thought that java owns the object.
Thanks for the fix!

https://codereview.chromium.org/1098253005/diff/20001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java
(right):

https://codereview.chromium.org/1098253005/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java:244:
// destroy the CronetUploadDataStreamAdapter.
nit: onAdapterDestroyed() destroys the CronetUploadDataStreamDelegate (not the
CronetUploadDataStreamAdapter).

Maybe also mention the fact that CronetUploadDataStreamAdapter is owned by the
native TestUploadDataStreamHandler, if we destroy the native
TestUploadDataStreamHandler, the CronetUploadDataStreamAdapter will also be
destroyed(.. which will invoke CronetUploadDataStream's onAdapterDestroyed()
method which will then destroy CronetUploadDataStreamDelegate).

Now I think about it, our naming scheme is confusing. 
We probably should rename cronet_upload_data_stream_delegate.cc as
cronet_upload_data_stream_adapter.cc, and cronet_upload_data_stream_adapter.cc
as cronet_upload_data_stream.cc (since cronet_upload_data_stream_adapter is
really an upload_data_stream).

[mmenke, 2015-04-24 18:22:17]

Thanks for the feedback - as always, well thought out.

https://codereview.chromium.org/1098253005/diff/20001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java
(right):

https://codereview.chromium.org/1098253005/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUploadTest.java:244:
// destroy the CronetUploadDataStreamAdapter.
On 2015/04/24 18:01:10, xunjieli wrote:
> nit: onAdapterDestroyed() destroys the CronetUploadDataStreamDelegate (not the
> CronetUploadDataStreamAdapter).

Fixed.

> Maybe also mention the fact that CronetUploadDataStreamAdapter is owned by the
> native TestUploadDataStreamHandler, if we destroy the native
> TestUploadDataStreamHandler, the CronetUploadDataStreamAdapter will also be
> destroyed(.. which will invoke CronetUploadDataStream's onAdapterDestroyed()
> method which will then destroy CronetUploadDataStreamDelegate).

Done.

> Now I think about it, our naming scheme is confusing. 
> We probably should rename cronet_upload_data_stream_delegate.cc as
> cronet_upload_data_stream_adapter.cc, and cronet_upload_data_stream_adapter.cc
> as cronet_upload_data_stream.cc (since cronet_upload_data_stream_adapter is
> really an upload_data_stream).

You're absolutely right about this.  My fault.  I'm happy to fix this after my
week off (Or this afternoon, if I have time).

[mmenke, 2015-04-24 18:22:38]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-04-24 18:23:59]

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

[commit-bot: I haz the power, 2015-04-24 18:24:01]

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

[commit-bot: I haz the power, 2015-04-24 18:24:01]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mmenke, 2015-04-24 18:25:31]

Oops...thought you signed off.  :)

[xunjieli, 2015-04-24 18:40:20]

On 2015/04/24 18:25:31, mmenke (Out 4-27 to 5-1) wrote:
> Oops...thought you signed off.  :)

LGTM! Just wanna double check in case I had it wrong :)

[mmenke, 2015-04-24 18:43:04]

On 2015/04/24 18:40:20, xunjieli wrote:
> On 2015/04/24 18:25:31, mmenke (Out 4-27 to 5-1) wrote:
> > Oops...thought you signed off.  :)
> 
> LGTM! Just wanna double check in case I had it wrong :)

No problem!  When there's any question, you absolutely should hold off on your
LGTM.  And you should ignore any impatience on the part of the author (Not that
I always live by that, myself).

[mmenke, 2015-04-24 18:43:11]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-04-24 18:44:21]

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

[commit-bot: I haz the power, 2015-04-24 19:06:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-24 19:07:49]

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