[Cronet] Test the libcronet that's shipped, not libcronet_test

components/cronet/android/test/cronet_test_util.cc

Index: components/cronet/android/test/cronet_test_util.cc
diff --git a/components/cronet/android/test/cronet_test_util.cc b/components/cronet/android/test/cronet_test_util.cc
index a44e484fab27ca7aae259ac3f37eddc2360e8b90..7433fb747ff78236992f1adebc07130d423d67cb 100644
--- a/components/cronet/android/test/cronet_test_util.cc
+++ b/components/cronet/android/test/cronet_test_util.cc
@@ -6,7 +6,10 @@
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
+#include "base/message_loop/message_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "components/cronet/android/cronet_url_request_adapter.h"
+#include "components/cronet/android/cronet_url_request_context_adapter.h"
 #include "components/cronet/android/test/native_test_server.h"
 #include "jni/CronetTestUtil_jni.h"
 #include "net/url_request/url_request.h"
@@ -17,13 +20,76 @@ namespace cronet {
 
 jint GetLoadFlags(JNIEnv* env,
                   const JavaParamRef<jclass>& jcaller,
-                  const jlong urlRequest) {
-  return reinterpret_cast<CronetURLRequestAdapter*>(urlRequest)
-      ->GetURLRequestForTesting()
-      ->load_flags();
+                  const jlong jurl_request_adapter) {
+  return TestUtil::getURLRequest(jurl_request_adapter)->load_flags();

[kapishnikov, 2016/10/24 21:17:34]

Is it safe to call load_flags() here? As I understand this works because
load_flags() is inline and the definition of URLRequest is the same in libcronet
& libcronet_test. But what will happen if the method is not inline for some
reason; or libcronettest is compiled with different flags which cause different
field alignment? 

[pauljensen, 2016/10/25 11:55:38]

If the method isn't inline, as I said, it's fine because load_flags() is in
net/, not Cronet, and net/ is built into libcronet_test.so so it should be fine.
 We get a linker unresolved symbol error if we try to use anything in Cronet
from the tests.

If libcronet and libcronet_test are compiled with different flags we're in
trouble, but the tests will definitely catch this.

 }
 
-bool RegisterCronetTestUtil(JNIEnv* env) {
+// static
+scoped_refptr<base::SingleThreadTaskRunner> TestUtil::getTaskRunner(
+    jlong jcontext_adapter) {
+  CronetURLRequestContextAdapter* context_adapter =
+      (CronetURLRequestContextAdapter*)jcontext_adapter;
+  return context_adapter->network_thread_->task_runner();

[kapishnikov, 2016/10/24 21:17:34]

The task_runner() is inline but if DCHECK is enabled, it calls a regular
non-virtual method:
id_event_.IsSignaled()

How will it find the address of that function if DEBUG is enabled?

[pauljensen, 2016/10/25 11:55:38]

As I said, this is all calling stuff in net/, not Cronet, and net/ is built into
libcronet_test.so so it should be fine.  We get a linker unresolved symbol error
if we try to use anything in Cronet from the tests.

+}
+
+// static
+net::URLRequestContext* TestUtil::getURLRequestContext(jlong jcontext_adapter) {
+  CronetURLRequestContextAdapter* context_adapter =
+      (CronetURLRequestContextAdapter*)jcontext_adapter;
+  return context_adapter->context_.get();
+}
+
+// static
+void TestUtil::runAfterContextInit(jlong jcontext_adapter,
+                                   const base::Closure& task) {
+  CronetURLRequestContextAdapter* context_adapter =
+      (CronetURLRequestContextAdapter*)jcontext_adapter;

[xunjieli, 2016/10/25 17:51:38]

reinterpret_cast<CronetURLRequestContextAdapter*> ?

[pauljensen, 2016/10/26 17:55:18]

Done.

+  if (context_adapter->is_context_initialized_) {
+    task.Run();
+  } else {
+    context_adapter->tasks_waiting_for_context_.push(task);
+  }
+}
+
+// static
+net::URLRequest* TestUtil::getURLRequest(jlong jrequest_adapter) {
+  CronetURLRequestAdapter* request_adapter =
+      (CronetURLRequestAdapter*)jrequest_adapter;
+  return request_adapter->url_request_.get();
+}
+
+static void PrepareNetworkThreadOnNetworkThread(jlong jcontext_adapter) {

[xunjieli, 2016/10/25 17:51:38]

Can this be a file local function (in an anonymous space) ?

[pauljensen, 2016/10/26 17:55:18]

It's static, which means file local.  I'd rather not hoist it up into an
anonymous namespace, as that would move it far away from its pair,
PrepareNetworkThread, and I'd prefer to have it reside beside its pair.

+  (new base::MessageLoopForIO())
+      ->SetTaskRunner(TestUtil::getTaskRunner(jcontext_adapter));
+}
+
+// Tests need to call into libcronet.so code on libcronet.so threads.
+// libcronet.so's threads are registered with static tables for MessageLoops
+// and SingleThreadTaskRunners in libcronet.so, so libcronet_test.so
+// functions that try and access these tables will find missing entries in
+// the corresponding static tables in libcronet_test.so.  Fix this by
+// initializing a MessageLoop and SingleThreadTaskRunner in libcronet_test.so
+// for these threads.
+void PrepareNetworkThread(JNIEnv* env,
+                          const JavaParamRef<jclass>& jcaller,
+                          jlong jcontext_adapter) {
+  TestUtil::getTaskRunner(jcontext_adapter)
+      ->PostTask(FROM_HERE, base::Bind(&PrepareNetworkThreadOnNetworkThread,
+                                       jcontext_adapter));
+}
+
+static void CleanupNetorkThreadOnNetworkThread() {

[xunjieli, 2016/10/25 17:51:38]

file local function? Also there is a typo in Network.

[pauljensen, 2016/10/26 17:55:18]

Same comment about file local.  I fixed the typo in Network.

+  delete base::MessageLoop::current();
+}
+
+void CleanupNetorkThread(JNIEnv* env,
+                         const JavaParamRef<jclass>& jcaller,
+                         jlong jcontext_adapter) {
+  TestUtil::getTaskRunner(jcontext_adapter)
+      ->PostTask(FROM_HERE, base::Bind(&CleanupNetorkThreadOnNetworkThread));
+}
+
+bool TestUtil::Register(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }