[concurrency] fix over-eager modeling of thread status

Summary:
At some point it was thought that we can assume that any annotation starting with "On" means the method is on the UI thread.
That's too imprecise and has led to false positives and negatives.  Restrict to a well-known safe set.

Reviewed By: ezgicicek

Differential Revision: D17769376

fbshipit-source-id: 0f8fee059

infer/src/checkers/annotations.ml

@@ -67,6 +67,16 @@ let notnull = "NotNull"
 
 let not_thread_safe = "NotThreadSafe"
 
+let on_bind = "OnBind"
+
+let on_event = "OnEvent"
+
+let on_mount = "OnMount"
+
+let on_unbind = "OnUnbind"
+
+let on_unmount = "OnUnmount"
+
 let performance_critical = "PerformanceCritical"
 
 let prop = "Prop"
@@ -111,10 +121,6 @@ let ma_has_annotation_with ({return; params} : Annot.Method.t) (predicate : Anno
   has_annot return || List.exists ~f:has_annot params
 
 
-let get_annot_ending ({class_name} : Annot.t) =
-  String.rsplit2 class_name ~on:'.' |> Option.value_map ~default:class_name ~f:snd
-
-
 (** [annot_ends_with annot ann_name] returns true if the class name of [annot], without the package,
     is equal to [ann_name] *)
 let annot_ends_with ({class_name} : Annot.t) ann_name =
@@ -232,3 +238,8 @@ let ia_is_suppress_lint ia = ia_ends_with ia suppress_lint
 let ia_is_thread_confined ia = ia_ends_with ia thread_confined
 
 let ia_is_worker_thread ia = ia_ends_with ia worker_thread
+
+(* methods annotated with the annotations below always run on the UI thread. *)
+let ia_is_uithread_equivalent =
+  let annotations = [mainthread; ui_thread; on_bind; on_event; on_mount; on_unbind; on_unmount] in
+  fun ia -> List.exists annotations ~f:(ia_ends_with ia)

infer/src/checkers/annotations.mli

@@ -51,9 +51,6 @@ val visibleForTesting : string
 
 val generated_graphql : string
 
-val get_annot_ending : Annot.t -> string
-(** get the '.'-last component of an annotation *)
-
 val annot_ends_with : Annot.t -> string -> bool
 (** [annot_ends_with annot ann_name] returns true if the class name of [annot], without the package,
     is equal to [ann_name] *)
@@ -115,6 +112,8 @@ val ia_is_volatile : Annot.Item.t -> bool
 
 val ia_is_worker_thread : Annot.Item.t -> bool
 
+val ia_is_uithread_equivalent : Annot.Item.t -> bool
+
 val pdesc_get_return_annot : Procdesc.t -> Annot.Item.t
 (** get the list of annotations on the return value of [pdesc] *)
 

infer/src/concurrency/ConcurrencyModels.ml

@@ -376,22 +376,10 @@ let if_pred_evalopt ~pred ~f x =
   IOption.if_none_evalopt x ~f:(fun () -> if pred () then Some (f ()) else None)
 
 
-(* assume that methods annotated with @MainThread, @UiThread,
-   and any string starting with "On" always run on the UI thread.
-   We do the latter because there are too many to precisely list. *)
-let is_uithread annots =
-  let f (annot, _) =
-    let ending = Annotations.get_annot_ending annot in
-    String.equal ending Annotations.mainthread
-    || String.equal ending Annotations.ui_thread
-    || String.is_prefix ~prefix:"On" ending
-  in
-  List.exists annots ~f
-
-
 let mono_pname = MF.wrap_monospaced Typ.Procname.pp
 
 let runs_on_ui_thread ~attrs_of_pname tenv proc_desc =
+  let is_uithread = Annotations.ia_is_uithread_equivalent in
   let pname = Procdesc.get_proc_name proc_desc in
   if
     is_method_or_override_annotated ~attrs_of_pname Annotations.ia_is_worker_thread pname tenv

infer/tests/codetoanalyze/java/racerd/Annotations.java

@@ -440,7 +440,7 @@ class ExtendsClassOnUiThread extends AllMethodsOnUiThread {
   }
 }
 
-// All annotations that start with "On" are assumed to be on the main thread
+// NOT All annotations that start with "On" are on the main thread
 @Target(ElementType.METHOD)
 @Retention(RetentionPolicy.CLASS)
 @interface OnXYZ {}
@@ -450,7 +450,7 @@ class WeirdAnnotation {
   int f;
 
   @OnXYZ
-  void fooOk() {
+  void fooBad() {
     f = 0;
   }
 }

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

@@ -12,6 +12,7 @@ codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.Annotati
 codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.Annotations.read_off_UI_thread_Bad():void, 190, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [<Read trace>,access to `this.f`,<Write trace>,access to `this.f`]
 codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.ThreadSafeAlias.threadSafeAliasBad1():void, 82, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `this.field`]
 codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.ThreadSafeAlias.threadSafeAliasBad2():void, 87, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `this.field`]
+codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.WeirdAnnotation.fooBad():void, 454, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `this.f`]
 codetoanalyze/java/racerd/Arrays.java, Arrays.arrayParameterWriteBad(int[]):void, 26, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `name1.[_]`]
 codetoanalyze/java/racerd/Arrays.java, Arrays.readWriteRaceBad(java.lang.String):java.lang.String, 47, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [<Read trace>,access to `this.strArr1.[_]`,<Write trace>,access to `this.strArr1.[_]`]
 codetoanalyze/java/racerd/Arrays.java, Arrays.writeWriteRaceBad(java.lang.String):void, 39, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `this.strArr1.[_]`]