From 881d6172845937bb0006341b7b46d275cf22791e Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Thu, 28 May 2020 07:18:11 -0700 Subject: [PATCH] [concurrency] android methods starting with "on" are on UI thread Summary: Android OS calls certain overridden class methods always on the UI thread. The function changed here attempted to build a list of all these methods, one by one. It's much more complete to simply consider a method as callable on the UI thread if it's an override of an Android library method, and it starts with "on". Only a single instance is known not to obey this pattern, so it's easier to blacklist than to whitelist. Also clarify the name to `is_android_lifecycle_method`. Reviewed By: ezgicicek Differential Revision: D21703365 fbshipit-source-id: 41ca3e998 --- infer/src/absint/ConcurrencyModels.ml | 76 ++++++++----------- infer/src/absint/ConcurrencyModels.mli | 2 +- infer/src/backend/StarvationGlobalAnalysis.ml | 2 +- .../MyServiceConnection.java | 1 + .../java/starvation-whole-program/issues.exp | 6 +- 5 files changed, 35 insertions(+), 52 deletions(-) diff --git a/infer/src/absint/ConcurrencyModels.ml b/infer/src/absint/ConcurrencyModels.ml index 1f35e4d5b..360cbadfd 100644 --- a/infer/src/absint/ConcurrencyModels.ml +++ b/infer/src/absint/ConcurrencyModels.ml @@ -307,52 +307,36 @@ let get_current_class_and_annotated_superclasses is_annot tenv pname = None -let ui_matcher_records = - let open MethodMatcher in - let fragment_methods = - (* sort police: this is in lifecycle order *) - [ "onAttach" - ; "onCreate" - ; "onCreateView" - ; "onActivityCreated" - ; "onStart" - ; "onResume" - ; "onPause" - ; "onStop" - ; "onDestroyView" - ; "onDestroy" - ; "onDetach" ] +let is_android_lifecycle_method tenv pname = + let package_starts_with_android procname = + Procname.get_class_type_name procname + |> Option.exists ~f:(fun typename -> + match (typename : Typ.Name.t) with + | CUnion _ | CStruct _ | CppClass _ | ObjcClass _ | ObjcProtocol _ -> + false + | JavaClass java_class_name -> + JavaClassName.package java_class_name + |> Option.exists ~f:(String.is_prefix ~prefix:"android") ) in - (* search_superclasses is true by default in how [default] is treated *) - [ {default with classname= "android.support.v4.app.Fragment"; methods= fragment_methods} - ; {default with classname= "android.app.Fragment"; methods= fragment_methods} - ; {default with classname= "androidx.fragment.app.Fragment"; methods= fragment_methods} - ; {default with classname= "android.content.ContentProvider"; methods= ["onCreate"]} - ; {default with classname= "android.content.BroadcastReceiver"; methods= ["onReceive"]} - ; { default with - classname= "android.app.Service" - ; methods= ["onBind"; "onCreate"; "onDestroy"; "onStartCommand"] } - ; {default with classname= "android.app.Application"; methods= ["onCreate"]} - ; { default with - classname= "android.app.Activity" - ; methods= ["onCreate"; "onStart"; "onRestart"; "onResume"; "onPause"; "onStop"; "onDestroy"] } - ; { default with - (* 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 *) - method_prefix= true - ; classname= "android.view.View" - ; methods= ["on"] } - ; { default with - classname= "android.content.ServiceConnection" - ; methods= ["onBindingDied"; "onNullBinding"; "onServiceConnected"; "onServiceDisconnected"] } - ] - - -let is_modeled_ui_method = - let matchers = List.map ui_matcher_records ~f:MethodMatcher.of_record in - (* we pass an empty actuals list because all matching is done on class and method name here *) - fun tenv pname -> MethodMatcher.of_list matchers tenv pname [] + let overrides_android_method tenv pname = + PatternMatch.override_exists package_starts_with_android tenv pname + in + let method_starts_with_on pname = Procname.get_method pname |> String.is_prefix ~prefix:"on" in + let is_whitelisted pname = + match Procname.get_method pname with + (* [IntentService.onHandleIntent] is an exception *) + | "onHandleIntent" -> + true + | _ -> + false + in + match (pname : Procname.t) with + | C _ | Linters_dummy_method | Block _ | ObjC_Cpp _ | WithBlockParameters _ -> + false + | Java _ -> + method_starts_with_on pname + && (not (is_whitelisted pname)) + && overrides_android_method tenv pname type annotation_trail = DirectlyAnnotated | Override of Procname.t | SuperClass of Typ.name @@ -392,7 +376,7 @@ let annotated_as_uithread_equivalent tenv pname = let runs_on_ui_thread tenv pname = - is_modeled_ui_method tenv pname || annotated_as_uithread_equivalent tenv pname + is_android_lifecycle_method tenv pname || annotated_as_uithread_equivalent tenv pname let is_recursive_lock_type = function diff --git a/infer/src/absint/ConcurrencyModels.mli b/infer/src/absint/ConcurrencyModels.mli index 27c7830bc..1e09056e5 100644 --- a/infer/src/absint/ConcurrencyModels.mli +++ b/infer/src/absint/ConcurrencyModels.mli @@ -55,5 +55,5 @@ val runs_on_ui_thread : Tenv.t -> Procname.t -> bool (** is method not transitively annotated [@WorkerThread] and is modeled or annotated [@UIThread] or equivalent? *) -val is_modeled_ui_method : Tenv.t -> Procname.t -> bool +val is_android_lifecycle_method : Tenv.t -> Procname.t -> bool (** is method a known Android UI thread callback (eg [Activity.onCreate]) *) diff --git a/infer/src/backend/StarvationGlobalAnalysis.ml b/infer/src/backend/StarvationGlobalAnalysis.ml index 7db45e77a..0dbf0b2fa 100644 --- a/infer/src/backend/StarvationGlobalAnalysis.ml +++ b/infer/src/backend/StarvationGlobalAnalysis.ml @@ -29,7 +29,7 @@ let iter_summary ~f exe_env (summary : Summary.t) = |> Option.iter ~f:(fun ({scheduled_work; critical_pairs} : summary) -> let pname = Summary.get_proc_name summary in let tenv = Exe_env.get_tenv exe_env pname in - if ConcurrencyModels.is_modeled_ui_method tenv pname then f pname critical_pairs ; + if ConcurrencyModels.is_android_lifecycle_method tenv pname then f pname critical_pairs ; ScheduledWorkDomain.iter (fun work -> get_summary_of_scheduled_work work |> Option.iter ~f:(f pname)) scheduled_work ) diff --git a/infer/tests/codetoanalyze/java/starvation-whole-program/MyServiceConnection.java b/infer/tests/codetoanalyze/java/starvation-whole-program/MyServiceConnection.java index 59c0b2dd8..068595832 100644 --- a/infer/tests/codetoanalyze/java/starvation-whole-program/MyServiceConnection.java +++ b/infer/tests/codetoanalyze/java/starvation-whole-program/MyServiceConnection.java @@ -23,6 +23,7 @@ class MyServiceConnection implements ServiceConnection { // implemented/overrides so no Bad suffixes + // following two methods are FNs due to a separate issue with default interface methods void onBindingDied(ComponentName name) { bad(); } diff --git a/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp b/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp index 25635920e..9e3f1afa8 100644 --- a/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation-whole-program/issues.exp @@ -29,10 +29,8 @@ codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onResume codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onResume():void, 100, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void MyActivity.onResume()`,Method call: `void MyActivity$3.run()`, locks `this.this$0.FP_monitorE` in `class MyActivity$3`, locks `this.this$0.FP_monitorD` in `class MyActivity$3`,[Trace 2] `void MyActivity.onResume()`, locks `this.FP_monitorD` in `class MyActivity`, locks `this.FP_monitorE` in `class MyActivity`] codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onStart():void, 33, STARVATION, no_bucket, ERROR, [`void MyActivity.onStart()`,Method call: `void MyActivity.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/MyActivity.java, MyActivity.onStop():void, 48, STARVATION, no_bucket, ERROR, [[Trace 1] `void MyActivity.onStop()`, locks `this.monitorA` in `class MyActivity`,[Trace 2] `void MyActivity.onStop()`,Method call: `void MyActivity$1.run()`, locks `this.this$0.monitorA` in `class MyActivity$1`,Method call: `void MyActivity.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] -codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onBindingDied(android.content.ComponentName):void, 27, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onBindingDied(ComponentName)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] -codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onNullBinding(android.content.ComponentName):void, 31, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onNullBinding(ComponentName)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] -codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onServiceConnected(android.content.ComponentName,android.os.IBinder):void, 36, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onServiceConnected(ComponentName,IBinder)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] -codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onServiceDisconnected(android.content.ComponentName):void, 41, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onServiceDisconnected(ComponentName)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] +codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onServiceConnected(android.content.ComponentName,android.os.IBinder):void, 37, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onServiceConnected(ComponentName,IBinder)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] +codetoanalyze/java/starvation-whole-program/MyServiceConnection.java, MyServiceConnection.onServiceDisconnected(android.content.ComponentName):void, 42, STARVATION, no_bucket, ERROR, [`void MyServiceConnection.onServiceDisconnected(ComponentName)`,Method call: `void MyServiceConnection.bad()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/StaticInitAttributes.java, StaticInitAttributes.postBlockingCallToUIExecutorBad():void, 52, STARVATION, no_bucket, ERROR, [`void StaticInitAttributes.postBlockingCallToUIExecutorBad()`,Method call: `void StaticInitAttributes$1.run()`,Method call: `void StaticInitAttributes.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/StaticInitAttributes.java, StaticInitAttributes.postBlockingCallToUIHandlerBad():void, 64, STARVATION, no_bucket, ERROR, [`void StaticInitAttributes.postBlockingCallToUIHandlerBad()`,Method call: `void StaticInitAttributes$1.run()`,Method call: `void StaticInitAttributes.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation-whole-program/ThreadScheduling.java, ThreadScheduling.scheduleBlockingCallOnContendedLockBad():void, 36, STARVATION, no_bucket, ERROR, [[Trace 1] `void ThreadScheduling.scheduleBlockingCallOnContendedLockBad()`,Method call: `void ThreadScheduling$2.run()`, locks `this.this$0.monitorA` in `class ThreadScheduling$2`,[Trace 2] `void ThreadScheduling.scheduleBlockingCallOnContendedLockBad()`,Method call: `void ThreadScheduling$1.run()`, locks `this.this$0.monitorA` in `class ThreadScheduling$1`,Method call: `void ThreadScheduling.doTransact()`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`]