[starvation] catch Object.wait calls on main thread

Summary: Consider calls to Object.wait as blocking.  Treat overloaded versions with timeout as well.

Reviewed By: mbouaziz

Differential Revision: D8989377

fbshipit-source-id: 084c1b0bd

infer/src/concurrency/RacerDConfig.ml

@@ -618,8 +618,8 @@ module Models = struct
   (** magical value from https://developer.android.com/topic/performance/vitals/anr *)
   let android_anr_time_limit = 5.0
 
-  (** can this timeout cause an anr? *)
-  let is_excessive_timeout =
+  (* get time unit in seconds *)
+  let timeunit_of_exp =
     let time_units =
       String.Map.of_alist_exn
         [ ("NANOSECONDS", 0.000_000_001)
@@ -630,45 +630,68 @@ module Models = struct
         ; ("HOURS", 3_600.0)
         ; ("DAYS", 86_400.0) ]
     in
-    fun duration_exp timeunit_exp ->
-      match (duration_exp, timeunit_exp) with
-      | HilExp.Constant (Const.Cint duration_lit), HilExp.AccessExpression timeunit_acc_exp -> (
-        match AccessExpression.to_access_path timeunit_acc_exp with
-        | _, [AccessPath.FieldAccess field]
-          when String.equal "java.util.concurrent.TimeUnit" (Typ.Fieldname.Java.get_class field) ->
-            let fieldname = Typ.Fieldname.Java.get_field field in
-            let duration = IntLit.to_float duration_lit in
-            String.Map.find time_units fieldname
-            |> Option.value_map ~default:false ~f:(fun unit_in_secs ->
-                   unit_in_secs *. duration >. android_anr_time_limit )
-        | _ ->
-            false )
+    let str_of_access_path = function
+      | _, [AccessPath.FieldAccess field]
+        when String.equal "java.util.concurrent.TimeUnit" (Typ.Fieldname.Java.get_class field) ->
+          Some (Typ.Fieldname.Java.get_field field)
       | _ ->
-          false
-
-
-  (** It's surprisingly difficult making any sense of the official documentation on java.io classes
-                as to whether methods may block.  We approximate these by calls in traditionally blocking
-                classes (non-channel based I/O), to methods `(Reader|InputStream).read*`.
-                Initially, (Writer|OutputStream).(flush|close) were also matched, but this generated too
-                many reports *)
+          None
+    in
+    let str_of_exp = function
+      | HilExp.AccessExpression timeunit_acc_exp ->
+          AccessExpression.to_access_path timeunit_acc_exp |> str_of_access_path
+      | _ ->
+          None
+    in
+    fun timeunit_exp -> str_of_exp timeunit_exp |> Option.bind ~f:(String.Map.find time_units)
 
-  (* let is_blocking_java_io =
-    is_call_of_class ["java.io.Reader"; "java.io.InputStream"] ~method_prefix:true
-      "read" *)
 
-  let actuals_are_empty_or_timeout = function
+  (** check whether actuals of a method call either empty, denoting indefinite timeout,
+      or evaluate to a finite timeout greater than the android anr limit *)
+  let empty_or_excessive_timeout actuals =
+    let duration_of_exp = function
+      | HilExp.Constant (Const.Cint duration_lit) ->
+          Some (IntLit.to_float duration_lit)
+      | _ ->
+          None
+    in
+    (* all arguments in seconds *)
+    let is_excessive_secs duration = duration >. android_anr_time_limit in
+    match actuals with
     | [_] ->
+        (* this is a wait or lock call without timeout, thus it can block indefinitely *)
         true
-    | [_; duration; timeunit] ->
-        is_excessive_timeout duration timeunit
+    | [_; snd_arg] ->
+        (* this is an Object.wait(_) call, second argument should be a duration in milliseconds *)
+        duration_of_exp snd_arg
+        |> Option.value_map ~default:false ~f:(fun duration ->
+               is_excessive_secs (0.001 *. duration) )
+    | [_; snd_arg; third_arg] ->
+        (* this is either a call to Object.wait(_, _) or to a java.util.concurent.lock(_, _) method.
+           In the first case the arguments are a duration in milliseconds and an extra duration in
+           nanoseconds; in the second case, the arguments are a duration and a time unit. *)
+        duration_of_exp snd_arg
+        |> Option.value_map ~default:false ~f:(fun duration ->
+               match timeunit_of_exp third_arg with
+               | Some timeunit ->
+                   is_excessive_secs (timeunit *. duration)
+               | None ->
+                   duration_of_exp third_arg
+                   |> Option.value_map ~default:false ~f:(fun extra ->
+                          is_excessive_secs (0.001 *. (duration +. (0.000_001 *. extra))) ) )
     | _ ->
         false
 
 
+  (** is the method called Object.wait or on subclass, without timeout or with excessive timeout ? *)
+  let is_object_wait =
+    is_call_of_class ~actuals_pred:empty_or_excessive_timeout ["java.lang.Object"] "wait"
+    |> Staged.unstage
+
+
   (** is the method called CountDownLath.await or on subclass? *)
   let is_countdownlatch_await =
-    is_call_of_class ~actuals_pred:actuals_are_empty_or_timeout
+    is_call_of_class ~actuals_pred:empty_or_excessive_timeout
       ["java.util.concurrent.CountDownLatch"] "await"
     |> Staged.unstage
 
@@ -693,7 +716,7 @@ module Models = struct
 
   (** is it a call to Future.get()? *)
   let is_future_get =
-    is_call_of_class ~search_superclasses:false ~actuals_pred:actuals_are_empty_or_timeout
+    is_call_of_class ~search_superclasses:false ~actuals_pred:empty_or_excessive_timeout
       ["java.util.concurrent.Future"] "get"
     |> Staged.unstage
 
@@ -704,7 +727,7 @@ module Models = struct
 
 
   let is_asyncTask_get =
-    is_call_of_class ~actuals_pred:actuals_are_empty_or_timeout ["android.os.AsyncTask"] "get"
+    is_call_of_class ~actuals_pred:empty_or_excessive_timeout ["android.os.AsyncTask"] "get"
     |> Staged.unstage
 
 
@@ -715,6 +738,7 @@ module Models = struct
       [ (is_accountManager_setUserData, High)
       ; (is_two_way_binder_transact, High)
       ; (is_countdownlatch_await, High)
+      ; (is_object_wait, High)
       ; (is_getWindowVisibleDisplayFrame, Medium)
       ; (is_asyncTask_get, Low)
       ; (is_future_get, Low) ]

infer/tests/codetoanalyze/java/starvation/ObjWait.java

@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2018-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import android.support.annotation.UiThread;
+
+class ObjWait {
+  Object z;
+
+  void waitOnAnyWithoutTimeoutOk() throws InterruptedException {
+    synchronized(z) {
+      z.wait();
+    }
+  }
+
+  Object o;
+
+  @UiThread
+  void waitOnMainWithoutTimeoutBad() throws InterruptedException {
+    synchronized(o) {
+      o.wait();
+    }
+  }
+
+  @UiThread
+  void waitOnMainWithExcessiveTimeout1Bad() throws InterruptedException {
+    synchronized(o) {
+      o.wait(5001);
+    }
+  }
+
+  @UiThread
+  void waitOnMainWithExcessiveTimeout2Bad() throws InterruptedException {
+    synchronized(o) {
+      o.wait(4000, 2000000000);
+    }
+  }
+
+  Object lock, x;
+
+  @UiThread
+  void indirectWaitOnMainWithoutTimeoutBad() throws InterruptedException {
+    synchronized(lock) {}
+  }
+
+  void lockAndWaitOnAnyWithoutTimeoutBad() throws InterruptedException {
+    synchronized(lock) {
+      synchronized(x) {
+        x.wait();
+      }
+    }
+  }
+}

infer/tests/codetoanalyze/java/starvation/issues.exp

@@ -27,6 +27,10 @@ codetoanalyze/java/starvation/Interproc.java, void Interproc.interproc1Bad(Inter
 codetoanalyze/java/starvation/Intraproc.java, void Intraproc.intraBad(IntraprocA), 10, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void Intraproc.intraBad(IntraprocA)`,locks `this` in class `Intraproc*`,locks `o` in class `IntraprocA*`,[Trace 2] `void IntraprocA.intraBad(Intraproc)`,locks `this` in class `IntraprocA*`,locks `o` in class `Intraproc*`]
 codetoanalyze/java/starvation/LegacySync.java, Object LegacySync.onUiThreadOpBad(), 25, STARVATION, no_bucket, ERROR, [[Trace 1] `Object LegacySync.onUiThreadOpBad()`,locks `this.LegacySync.table` in class `LegacySync*`,[Trace 2] `void LegacySync.notOnUiThreadSyncedBad()`,locks `this.LegacySync.table` in class `LegacySync*`,calls `Object Future.get()` from `void LegacySync.notOnUiThreadSyncedBad()`]
 codetoanalyze/java/starvation/NonBlk.java, void NonBlk.deadlockABBad(), 34, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void NonBlk.deadlockABBad()`,locks `this` in class `NonBlk*`,locks `this.NonBlk.future` in class `NonBlk*`,[Trace 2] `void NonBlk.deadlockBABad()`,locks `this.NonBlk.future` in class `NonBlk*`,locks `this` in class `NonBlk*`]
+codetoanalyze/java/starvation/ObjWait.java, void ObjWait.indirectWaitOnMainWithoutTimeoutBad(), 46, STARVATION, no_bucket, ERROR, [[Trace 1] `void ObjWait.indirectWaitOnMainWithoutTimeoutBad()`,locks `this.ObjWait.lock` in class `ObjWait*`,[Trace 2] `void ObjWait.lockAndWaitOnAnyWithoutTimeoutBad()`,locks `this.ObjWait.lock` in class `ObjWait*`,calls `void Object.wait()` from `void ObjWait.lockAndWaitOnAnyWithoutTimeoutBad()`]
+codetoanalyze/java/starvation/ObjWait.java, void ObjWait.waitOnMainWithExcessiveTimeout1Bad(), 31, STARVATION, no_bucket, ERROR, [`void ObjWait.waitOnMainWithExcessiveTimeout1Bad()`,calls `void Object.wait(long)` from `void ObjWait.waitOnMainWithExcessiveTimeout1Bad()`]
+codetoanalyze/java/starvation/ObjWait.java, void ObjWait.waitOnMainWithExcessiveTimeout2Bad(), 38, STARVATION, no_bucket, ERROR, [`void ObjWait.waitOnMainWithExcessiveTimeout2Bad()`,calls `void Object.wait(long,int)` from `void ObjWait.waitOnMainWithExcessiveTimeout2Bad()`]
+codetoanalyze/java/starvation/ObjWait.java, void ObjWait.waitOnMainWithoutTimeoutBad(), 24, STARVATION, no_bucket, ERROR, [`void ObjWait.waitOnMainWithoutTimeoutBad()`,calls `void Object.wait()` from `void ObjWait.waitOnMainWithoutTimeoutBad()`]
 codetoanalyze/java/starvation/PubPriv.java, void PubPriv.alsoBad(), 25, STARVATION, no_bucket, ERROR, [`void PubPriv.alsoBad()`,Method call: `void PubPriv.transactBad()`,Method call: `void PubPriv.doTransactOk()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)` from `void PubPriv.doTransactOk()`]
 codetoanalyze/java/starvation/PubPriv.java, void PubPriv.callOneWayBad(), 47, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void PubPriv.callOneWayBad()`,Method call: `void PubPriv.oneWayOk()`,locks `this.PubPriv.lockA` in class `PubPriv*`,locks `this.PubPriv.lockB` in class `PubPriv*`,[Trace 2] `void PubPriv.callAnotherWayBad()`,Method call: `void PubPriv.anotherWayOk()`,locks `this.PubPriv.lockB` in class `PubPriv*`,locks `this.PubPriv.lockA` in class `PubPriv*`]
 codetoanalyze/java/starvation/PubPriv.java, void PubPriv.transactBad(), 21, STARVATION, no_bucket, ERROR, [`void PubPriv.transactBad()`,Method call: `void PubPriv.doTransactOk()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)` from `void PubPriv.doTransactOk()`]