[infer][starvation] Report locks on UI thread

Reviewed By: ngorogiannis Differential Revision: D66294273 fbshipit-source-id: f03f7a319690da4374e109165ef27853d83f4f52
facebook · Dec 9, 2024 · c3ef09a · c3ef09a
1 parent 8076f64
commit c3ef09a
Show file tree

Hide file tree

Showing 11 changed files with 132 additions and 27 deletions.
diff --git a/infer/documentation/issues/LOCK_ON_UI_THREAD.md b/infer/documentation/issues/LOCK_ON_UI_THREAD.md
@@ -0,0 +1,13 @@
+A method annoted as being on UIThread acquires a lock. This could be a potential performance issue
+
+Example:
+
+```java
+class Example {
+    @UiThread
+    void foo() {
+        synchronized(this) {
+        }
+    }
+}
+```
diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt
@@ -667,6 +667,7 @@ OPTIONS
              LINEAGE_FLOW (enabled by default),
              LOCKLESS_VIOLATION (enabled by default),
              LOCK_CONSISTENCY_VIOLATION (enabled by default),
+             LOCK_ON_UI_THREAD (disabled by default),
              Leak_after_array_abstraction (enabled by default),
              Leak_in_footprint (enabled by default),
              Leak_unknown_origin (disabled by default),

diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt
@@ -182,6 +182,7 @@ OPTIONS
              LINEAGE_FLOW (enabled by default),
              LOCKLESS_VIOLATION (enabled by default),
              LOCK_CONSISTENCY_VIOLATION (enabled by default),
+             LOCK_ON_UI_THREAD (disabled by default),
              Leak_after_array_abstraction (enabled by default),
              Leak_in_footprint (enabled by default),
              Leak_unknown_origin (disabled by default),

diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt
@@ -667,6 +667,7 @@ OPTIONS
              LINEAGE_FLOW (enabled by default),
              LOCKLESS_VIOLATION (enabled by default),
              LOCK_CONSISTENCY_VIOLATION (enabled by default),
+             LOCK_ON_UI_THREAD (disabled by default),
              Leak_after_array_abstraction (enabled by default),
              Leak_in_footprint (enabled by default),
              Leak_unknown_origin (disabled by default),

diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml
@@ -719,6 +719,11 @@ let lock_consistency_violation =
     ~user_documentation:[%blob "./documentation/issues/LOCK_CONSISTENCY_VIOLATION.md"]
 
 
+let lock_on_ui_thread =
+  register ~hum:"Lock on UI Thread" ~id:"LOCK_ON_UI_THREAD" ~category:PerfRegression ~enabled:false
+    Warning Starvation ~user_documentation:[%blob "./documentation/issues/LOCK_ON_UI_THREAD.md"]
+
+
 let lockless_violation =
   register ~category:NoCategory ~id:"LOCKLESS_VIOLATION" Error Starvation
     ~user_documentation:[%blob "./documentation/issues/LOCKLESS_VIOLATION.md"]

diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli
@@ -247,6 +247,8 @@ val lockless_violation : t
 
 val lock_consistency_violation : t
 
+val lock_on_ui_thread : t
+
 val expensive_loop_invariant_call : t
 
 val memory_leak : t

diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml
@@ -564,6 +564,8 @@ module ReportMap : sig
 
   val add_starvation : report_add_t
 
+  val add_lock_on_ui_thread : report_add_t
+
   val add_strict_mode_violation : report_add_t
 
   val issue_log_of : t -> IssueLog.t
@@ -623,6 +625,10 @@ end = struct
     add tenv pattrs loc ltr message IssueType.lockless_violation map
 
 
+  let add_lock_on_ui_thread tenv pattrs loc ltr message map =
+    add tenv pattrs loc ltr message IssueType.lock_on_ui_thread map
+
+
   let add_arbitrary_code_execution_under_lock tenv pattrs loc ltr message map =
     add tenv pattrs loc ltr message IssueType.arbitrary_code_execution_under_lock map
 
@@ -632,6 +638,7 @@ end = struct
       [ deadlock
       ; lockless_violation
       ; ipc_on_ui_thread
+      ; lock_on_ui_thread
       ; regex_op_on_ui_thread
       ; starvation
       ; strict_mode_violation
@@ -908,33 +915,42 @@ let report_on_pair ~analyze_ondemand tenv pattrs (pair : Domain.CriticalPair.t)
       let loc = CriticalPair.get_earliest_lock_or_call_loc ~procname:pname pair in
       let ltr = CriticalPair.make_trace pname pair in
       ReportMap.add_lockless_violation tenv pattrs loc ltr error_message report_map
-  | LockAcquire {locks} when is_not_private -> (
-    match
-      List.find locks ~f:(fun lock -> Acquisitions.lock_is_held lock pair.elem.acquisitions)
-    with
-    | Some lock ->
-        let error_message =
-          Format.asprintf "Potential self deadlock. %a%a twice." pname_pp pname Lock.pp_locks lock
-        in
-        let loc = CriticalPair.get_earliest_lock_or_call_loc ~procname:pname pair in
-        let ltr = CriticalPair.make_trace ~header:"In method " pname pair in
-        ReportMap.add_deadlock tenv pattrs loc ltr error_message report_map
-    | None when Config.starvation_whole_program ->
-        report_map
-    | None ->
-        List.fold locks ~init:report_map ~f:(fun acc lock ->
-            Lock.root_class lock
-            |> Option.value_map ~default:acc ~f:(fun other_class ->
-                   (* get the class of the root variable of the lock in the lock acquisition
-                      and retrieve all the summaries of the methods of that class;
-                      then, report on the parallel composition of the current pair and any pair in these
-                      summaries that can indeed run in parallel *)
-                   fold_reportable_summaries analyze_ondemand tenv other_class ~init:acc
-                     ~f:(fun acc (other_pname, summary) ->
-                       Domain.fold_critical_pairs_of_summary
-                         (report_on_parallel_composition ~should_report_starvation tenv pattrs pair
-                            lock other_pname )
-                         summary acc ) ) ) )
+  | LockAcquire {locks; thread} when is_not_private -> (
+      let report_map =
+        if ThreadDomain.is_uithread thread && should_report_starvation then
+          let error_message =
+            Format.asprintf "%a acquired in UI Thread!" (Format.pp_print_list Lock.pp_locks) locks
+          in
+          let ltr, loc = make_trace_and_loc () in
+          ReportMap.add_lock_on_ui_thread tenv pattrs loc ltr error_message report_map
+        else report_map
+      in
+      match
+        List.find locks ~f:(fun lock -> Acquisitions.lock_is_held lock pair.elem.acquisitions)
+      with
+      | Some lock ->
+          let error_message =
+            Format.asprintf "Potential self deadlock. %a%a twice." pname_pp pname Lock.pp_locks lock
+          in
+          let loc = CriticalPair.get_earliest_lock_or_call_loc ~procname:pname pair in
+          let ltr = CriticalPair.make_trace ~header:"In method " pname pair in
+          ReportMap.add_deadlock tenv pattrs loc ltr error_message report_map
+      | None when Config.starvation_whole_program ->
+          report_map
+      | None ->
+          List.fold locks ~init:report_map ~f:(fun acc lock ->
+              Lock.root_class lock
+              |> Option.value_map ~default:acc ~f:(fun other_class ->
+                     (* get the class of the root variable of the lock in the lock acquisition
+                        and retrieve all the summaries of the methods of that class;
+                        then, report on the parallel composition of the current pair and any pair in these
+                        summaries that can indeed run in parallel *)
+                     fold_reportable_summaries analyze_ondemand tenv other_class ~init:acc
+                       ~f:(fun acc (other_pname, summary) ->
+                         Domain.fold_critical_pairs_of_summary
+                           (report_on_parallel_composition ~should_report_starvation tenv pattrs
+                              pair lock other_pname )
+                           summary acc ) ) ) )
   | _ ->
       report_map
 

diff --git a/infer/src/concurrency/starvationDomain.mli b/infer/src/concurrency/starvationDomain.mli
@@ -20,6 +20,8 @@ module F = Format
 module ThreadDomain : sig
   type t = UnknownThread | UIThread | BGThread | AnyThread | NamedThread of string
 
+  val is_uithread : t -> bool
+
   include AbstractDomain.WithBottom with type t := t
 end