[concurrency] fix UI thread models

Summary: Clean up UI thread modelling.

Reviewed By: jeremydubreil

Differential Revision: D9800251

fbshipit-source-id: 45ed60802

infer/src/concurrency/ConcurrencyModels.ml

@@ -158,11 +158,109 @@ let find_annotated_or_overriden_annotated_method is_annot pname tenv =
     tenv pname
 
 
-(* we don't want to warn on methods that run on the UI thread because they should always be
-      single-threaded *)
+let is_call_of_class ?(search_superclasses = true) ?(method_prefix = false)
+    ?(actuals_pred = fun _ -> true) clazz methods =
+  let method_matcher =
+    if method_prefix then fun current_method target_method ->
+      String.is_prefix current_method ~prefix:target_method
+    else fun current_method target_method -> String.equal current_method target_method
+  in
+  let class_matcher =
+    let is_target_class =
+      let target = Typ.Name.Java.from_string clazz in
+      fun tname -> Typ.Name.equal tname target
+    in
+    if search_superclasses then fun tenv classname ->
+      let is_target_struct tname _ = is_target_class tname in
+      PatternMatch.supertype_exists tenv is_target_struct classname
+    else fun _ classname -> is_target_class classname
+  in
+  (fun tenv pn actuals ->
+    actuals_pred actuals
+    &&
+    match pn with
+    | Typ.Procname.Java java_pname ->
+        let mthd = Typ.Procname.Java.get_method java_pname in
+        List.exists methods ~f:(method_matcher mthd)
+        &&
+        let classname = Typ.Procname.Java.get_class_type_name java_pname in
+        class_matcher tenv classname
+    | _ ->
+        false )
+  |> Staged.stage
+
+
+let is_ui_method =
+  let is_activity_ui_method =
+    (* lifecycle methods of Activity *)
+    is_call_of_class ~search_superclasses:true "android.app.Activity"
+      (* sort police: this is in lifecycle order *)
+      ["onCreate"; "onStart"; "onRestart"; "onResume"; "onPause"; "onStop"; "onDestroy"]
+    |> Staged.unstage
+  in
+  let is_fragment_ui_method =
+    (* lifecycle methods of Fragment *)
+    let fragment_methods =
+      (* sort police: this is in lifecycle order *)
+      [ "onAttach"
+      ; "onCreate"
+      ; "onCreateView"
+      ; "onActivityCreated"
+      ; "onStart"
+      ; "onResume"
+      ; "onPause"
+      ; "onStop"
+      ; "onDestroyView"
+      ; "onDestroy"
+      ; "onDetach" ]
+    in
+    let fragment_v4_matcher =
+      is_call_of_class ~search_superclasses:true "android.support.v4.app.Fragment" fragment_methods
+      |> Staged.unstage
+    in
+    let fragment_matcher =
+      is_call_of_class ~search_superclasses:true "android.app.Fragment" fragment_methods
+      |> Staged.unstage
+    in
+    fun tenv pname actuals ->
+      fragment_v4_matcher tenv pname actuals || fragment_matcher tenv pname actuals
+  in
+  let is_service_ui_method =
+    (* lifecycle methods of Service *)
+    is_call_of_class ~search_superclasses:true "android.app.Service"
+      ["onBind"; "onCreate"; "onDestroy"; "onStartCommand"]
+    |> Staged.unstage
+  in
+  let is_content_provider_ui_method =
+    is_call_of_class ~search_superclasses:true "android.content.ContentProvider" ["onCreate"]
+    |> Staged.unstage
+  in
+  let is_broadcast_receiver_ui_method =
+    is_call_of_class ~search_superclasses:true "android.content.BroadcastReceiver" ["onReceive"]
+    |> Staged.unstage
+  in
+  let is_view_ui_method =
+    (* according to Android documentation, *all* methods of the View class run on UI thread, but
+     let's be a bit conservative and catch all methods that start with "on".
+     https://developer.android.com/reference/android/view/View.html *)
+    is_call_of_class ~search_superclasses:true ~method_prefix:true "android.view.View" ["on"]
+    |> Staged.unstage
+  in
+  let matchers =
+    [ is_activity_ui_method
+    ; is_fragment_ui_method
+    ; is_service_ui_method
+    ; is_content_provider_ui_method
+    ; is_broadcast_receiver_ui_method
+    ; is_view_ui_method ]
+  in
+  (* we pass an empty actuals list because all matching is done on class and method name here *)
+  fun tenv pname -> List.exists matchers ~f:(fun m -> m tenv pname [])
+
+
 let runs_on_ui_thread tenv proc_desc =
   (* assume that methods annotated with @UiThread, @OnEvent, @OnBind, @OnMount, @OnUnbind,
-        @OnUnmount always run on the UI thread *)
+          @OnUnmount always run on the UI thread *)
   let is_annot annot =
     Annotations.ia_is_ui_thread annot || Annotations.ia_is_on_bind annot
     || Annotations.ia_is_on_event annot || Annotations.ia_is_on_mount annot
@@ -170,6 +268,7 @@ let runs_on_ui_thread tenv proc_desc =
     || Annotations.ia_is_on_unmount annot
   in
   let pname = Procdesc.get_proc_name proc_desc in
+  let mono_pname = MF.wrap_monospaced Typ.Procname.pp in
   if
     Annotations.pdesc_has_return_annot proc_desc Annotations.ia_is_worker_thread
     || find_annotated_or_overriden_annotated_method Annotations.ia_is_worker_thread pname tenv
@@ -177,34 +276,23 @@ let runs_on_ui_thread tenv proc_desc =
     || get_current_class_and_annotated_superclasses Annotations.ia_is_worker_thread tenv pname
        |> Option.value_map ~default:false ~f:(function _, [] -> false | _ -> true)
   then None
+  else if is_ui_method tenv pname then
+    Some (F.asprintf "%a is a standard UI-thread method" mono_pname pname)
   else if Annotations.pdesc_has_return_annot proc_desc is_annot then
     Some
-      (F.asprintf "%a is annotated %s"
-         (MF.wrap_monospaced Typ.Procname.pp)
-         pname
+      (F.asprintf "%a is annotated %s" mono_pname pname
          (MF.monospaced_to_string Annotations.ui_thread))
   else
     match find_annotated_or_overriden_annotated_method is_annot pname tenv with
     | Some override_pname ->
         Some
-          (F.asprintf "class %a overrides %a, which is annotated %s"
-             (MF.wrap_monospaced Typ.Procname.pp)
-             pname
-             (MF.wrap_monospaced Typ.Procname.pp)
+          (F.asprintf "class %a overrides %a, which is annotated %s" mono_pname pname mono_pname
              override_pname
              (MF.monospaced_to_string Annotations.ui_thread))
     | None -> (
       match
         get_current_class_and_annotated_superclasses Annotations.ia_is_ui_thread tenv pname
       with
-      | Some (current_class, _)
-        when let open PatternMatch in
-             is_subtype_of_str tenv current_class "android.app.Service"
-             && not (is_subtype_of_str tenv current_class "android.app.IntentService") ->
-          Some
-            (F.asprintf "class %s extends %s"
-               (MF.monospaced_to_string (Typ.Name.name current_class))
-               (MF.monospaced_to_string "android.app.Service"))
       | Some (current_class, (super_class :: _ as super_classes)) ->
           let middle =
             if List.exists super_classes ~f:(Typ.Name.equal current_class) then ""
@@ -217,35 +305,3 @@ let runs_on_ui_thread tenv proc_desc =
                (MF.monospaced_to_string Annotations.ui_thread))
       | _ ->
           None )
-
-
-let is_call_of_class ?(search_superclasses = true) ?(method_prefix = false)
-    ?(actuals_pred = fun _ -> true) clazz methods =
-  let method_matcher =
-    if method_prefix then fun current_method target_method ->
-      String.is_prefix current_method ~prefix:target_method
-    else fun current_method target_method -> String.equal current_method target_method
-  in
-  let class_matcher =
-    let is_target_class =
-      let target = Typ.Name.Java.from_string clazz in
-      fun tname -> Typ.Name.equal tname target
-    in
-    if search_superclasses then fun tenv classname ->
-      let is_target_struct tname _ = is_target_class tname in
-      PatternMatch.supertype_exists tenv is_target_struct classname
-    else fun _ classname -> is_target_class classname
-  in
-  (fun tenv pn actuals ->
-    actuals_pred actuals
-    &&
-    match pn with
-    | Typ.Procname.Java java_pname ->
-        let mthd = Typ.Procname.Java.get_method java_pname in
-        List.exists methods ~f:(method_matcher mthd)
-        &&
-        let classname = Typ.Procname.Java.get_class_type_name java_pname in
-        class_matcher tenv classname
-    | _ ->
-        false )
-  |> Staged.stage

infer/tests/codetoanalyze/java/starvation/MyActivity.java

@@ -0,0 +1,55 @@
+/*
+ * 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.app.Activity;
+import android.accounts.AccountManager;
+import android.os.Bundle;
+
+// test is for recognizing Activity lifecycle methods
+class MyActivity extends Activity {
+  AccountManager am;
+  private void bad() {
+    am.setUserData(null, null, null);
+  }
+
+  // overrides so no Bad suffixes
+
+  @Override
+  protected void onCreate (Bundle savedInstanceState) {
+    bad();
+  }
+
+  @Override
+  public void onStart() {
+    bad();
+  }
+
+  @Override
+  public void onRestart() {
+    bad();
+  }
+
+  @Override
+  public void onResume() {
+    bad();
+  }
+
+  @Override
+  public void onPause() {
+    bad();
+  }
+
+  @Override
+  public void onStop() {
+    bad();
+  }
+
+  @Override
+  public void onDestroy() {
+    bad();
+  }
+}

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

@@ -26,6 +26,13 @@ codetoanalyze/java/starvation/Interclass.java, Interclass.interclass1Bad(Intercl
 codetoanalyze/java/starvation/Interproc.java, Interproc.interproc1Bad(InterprocA):void, 9, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void Interproc.interproc1Bad(InterprocA)`,locks `this` in class `Interproc*`,Method call: `void Interproc.interproc2Bad(InterprocA)`,locks `b` in class `InterprocA*`,[Trace 2] `void InterprocA.interproc1Bad(Interproc)`,locks `this` in class `InterprocA*`,Method call: `void InterprocA.interproc2Bad(Interproc)`,locks `d` in class `Interproc*`]
 codetoanalyze/java/starvation/Intraproc.java, Intraproc.intraBad(IntraprocA):void, 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, LegacySync.onUiThreadOpBad():java.lang.Object, 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()`,[Trace 1 on UI thread] `Object LegacySync.onUiThreadOpBad()`,`Object LegacySync.onUiThreadOpBad()` is annotated `UiThread`]
+codetoanalyze/java/starvation/MyActivity.java, MyActivity.onCreate(android.os.Bundle):void, 23, STARVATION, no_bucket, ERROR, [`void MyActivity.onCreate(Bundle)`,Method call: `void MyActivity.bad()`,calls `void AccountManager.setUserData(Account,String,String)` from `void MyActivity.bad()`,[Trace on UI thread] `void MyActivity.onCreate(Bundle)`,`void MyActivity.onCreate(Bundle)` is a standard UI-thread method]
+codetoanalyze/java/starvation/MyActivity.java, MyActivity.onDestroy():void, 53, STARVATION, no_bucket, ERROR, [`void MyActivity.onDestroy()`,Method call: `void MyActivity.bad()`,calls `void AccountManager.setUserData(Account,String,String)` from `void MyActivity.bad()`,[Trace on UI thread] `void MyActivity.onDestroy()`,`void MyActivity.onDestroy()` is a standard UI-thread method]
+codetoanalyze/java/starvation/MyActivity.java, MyActivity.onPause():void, 43, STARVATION, no_bucket, ERROR, [`void MyActivity.onPause()`,Method call: `void MyActivity.bad()`,calls `void AccountManager.setUserData(Account,String,String)` from `void MyActivity.bad()`,[Trace on UI thread] `void MyActivity.onPause()`,`void MyActivity.onPause()` is a standard UI-thread method]
+codetoanalyze/java/starvation/MyActivity.java, MyActivity.onRestart():void, 33, STARVATION, no_bucket, ERROR, [`void MyActivity.onRestart()`,Method call: `void MyActivity.bad()`,calls `void AccountManager.setUserData(Account,String,String)` from `void MyActivity.bad()`,[Trace on UI thread] `void MyActivity.onRestart()`,`void MyActivity.onRestart()` is a standard UI-thread method]
+codetoanalyze/java/starvation/MyActivity.java, MyActivity.onResume():void, 38, STARVATION, no_bucket, ERROR, [`void MyActivity.onResume()`,Method call: `void MyActivity.bad()`,calls `void AccountManager.setUserData(Account,String,String)` from `void MyActivity.bad()`,[Trace on UI thread] `void MyActivity.onResume()`,`void MyActivity.onResume()` is a standard UI-thread method]
+codetoanalyze/java/starvation/MyActivity.java, MyActivity.onStart():void, 28, STARVATION, no_bucket, ERROR, [`void MyActivity.onStart()`,Method call: `void MyActivity.bad()`,calls `void AccountManager.setUserData(Account,String,String)` from `void MyActivity.bad()`,[Trace on UI thread] `void MyActivity.onStart()`,`void MyActivity.onStart()` is a standard UI-thread method]
+codetoanalyze/java/starvation/MyActivity.java, MyActivity.onStop():void, 48, STARVATION, no_bucket, ERROR, [`void MyActivity.onStop()`,Method call: `void MyActivity.bad()`,calls `void AccountManager.setUserData(Account,String,String)` from `void MyActivity.bad()`,[Trace on UI thread] `void MyActivity.onStop()`,`void MyActivity.onStop()` is a standard UI-thread method]
 codetoanalyze/java/starvation/NonBlk.java, NonBlk.deadlockABBad():void, 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, ObjWait.indirectWaitOnMainWithoutTimeoutBad():void, 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()`,[Trace 1 on UI thread] `void ObjWait.indirectWaitOnMainWithoutTimeoutBad()`,`void ObjWait.indirectWaitOnMainWithoutTimeoutBad()` is annotated `UiThread`]
 codetoanalyze/java/starvation/ObjWait.java, ObjWait.waitOnMainWithExcessiveTimeout1Bad():void, 31, STARVATION, no_bucket, ERROR, [`void ObjWait.waitOnMainWithExcessiveTimeout1Bad()`,calls `void Object.wait(long)` from `void ObjWait.waitOnMainWithExcessiveTimeout1Bad()`,[Trace on UI thread] `void ObjWait.waitOnMainWithExcessiveTimeout1Bad()`,`void ObjWait.waitOnMainWithExcessiveTimeout1Bad()` is annotated `UiThread`]
@@ -34,8 +41,7 @@ codetoanalyze/java/starvation/ObjWait.java, ObjWait.waitOnMainWithoutTimeoutBad(
 codetoanalyze/java/starvation/PubPriv.java, PubPriv.alsoBad():void, 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()`,[Trace on UI thread] `void PubPriv.alsoBad()`,Method call: `void PubPriv.transactBad()`,Method call: `void PubPriv.doTransactOk()`,`void PubPriv.doTransactOk()` is annotated `UiThread`]
 codetoanalyze/java/starvation/PubPriv.java, PubPriv.callOneWayBad():void, 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, PubPriv.transactBad():void, 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()`,[Trace on UI thread] `void PubPriv.transactBad()`,Method call: `void PubPriv.doTransactOk()`,`void PubPriv.doTransactOk()` is annotated `UiThread`]
-codetoanalyze/java/starvation/ServiceOnUIThread.java, ServiceOnUIThread.onBind(android.content.Intent):android.os.IBinder, 19, STARVATION, no_bucket, ERROR, [`IBinder ServiceOnUIThread.onBind(Intent)`,Method call: `void ServiceOnUIThread.transactBad()`,calls `boolean IBinder.transact(int,Parcel,Parcel,int)` from `void ServiceOnUIThread.transactBad()`,[Trace on UI thread] `IBinder ServiceOnUIThread.onBind(Intent)`,class `ServiceOnUIThread` extends `android.app.Service`]
-codetoanalyze/java/starvation/ServiceOnUIThread.java, ServiceOnUIThread.transactBad():void, 25, STARVATION, no_bucket, ERROR, [`void ServiceOnUIThread.transactBad()`,calls `boolean IBinder.transact(int,Parcel,Parcel,int)` from `void ServiceOnUIThread.transactBad()`,[Trace on UI thread] `void ServiceOnUIThread.transactBad()`,class `ServiceOnUIThread` extends `android.app.Service`]
+codetoanalyze/java/starvation/ServiceOnUIThread.java, ServiceOnUIThread.onBind(android.content.Intent):android.os.IBinder, 19, STARVATION, no_bucket, ERROR, [`IBinder ServiceOnUIThread.onBind(Intent)`,Method call: `void ServiceOnUIThread.transactBad()`,calls `boolean IBinder.transact(int,Parcel,Parcel,int)` from `void ServiceOnUIThread.transactBad()`,[Trace on UI thread] `IBinder ServiceOnUIThread.onBind(Intent)`,`IBinder ServiceOnUIThread.onBind(Intent)` is a standard UI-thread method]
 codetoanalyze/java/starvation/StaticLock.java, StaticLock.lockOtherClassOneWayBad():void, 23, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void StaticLock.lockOtherClassOneWayBad()`,locks `StaticLock$0` in class `java.lang.Class*`,locks `this` in class `StaticLock*`,[Trace 2] `void StaticLock.lockOtherClassAnotherWayNad()`,locks `this` in class `StaticLock*`,Method call: `void StaticLock.staticSynced()`,locks `StaticLock$0` in class `java.lang.Class*`]
 codetoanalyze/java/starvation/SuppLint.java, SuppLint.onUiThreadBad():void, 26, STARVATION, no_bucket, ERROR, [`void SuppLint.onUiThreadBad()`,calls `Object Future.get()` from `void SuppLint.onUiThreadBad()`,[Trace on UI thread] `void SuppLint.onUiThreadBad()`,`void SuppLint.onUiThreadBad()` is annotated `UiThread`]
 codetoanalyze/java/starvation/ThreadSleep.java, ThreadSleep.indirectSleepOnUIThreadBad():void, 25, STARVATION, no_bucket, ERROR, [[Trace 1] `void ThreadSleep.indirectSleepOnUIThreadBad()`,locks `this.ThreadSleep.lock` in class `ThreadSleep*`,[Trace 2] `void ThreadSleep.lockAndSleepOnNonUIThread()`,locks `this.ThreadSleep.lock` in class `ThreadSleep*`,Method call: `void ThreadSleep.sleepOnAnyThreadOk()`,calls `void Thread.sleep(long)` from `void ThreadSleep.sleepOnAnyThreadOk()`,[Trace 1 on UI thread] `void ThreadSleep.indirectSleepOnUIThreadBad()`,`void ThreadSleep.indirectSleepOnUIThreadBad()` is annotated `UiThread`]