From a49b0965ef5a83e3569364708c62f37d52621164 Mon Sep 17 00:00:00 2001 From: jrm Date: Mon, 30 Nov 2015 21:52:49 -0800 Subject: [PATCH] Model `android.view.View.findViewById` as an expensive method Summary: public The method `android.view.View.findViewById` and should not be run performance critical parts of the code like scrolling. Reviewed By: sblackshear Differential Revision: D2698196 fb-gh-sync-id: 2716ad7 --- infer/src/backend/interproc.ml | 3 +- infer/src/backend/mangled.ml | 3 +- infer/src/backend/procname.ml | 10 ++++- infer/src/backend/procname.mli | 3 ++ infer/src/checkers/performanceCritical.ml | 40 ++++++++++++++----- infer/src/harness/androidFramework.ml | 24 +++++++---- infer/src/harness/androidFramework.mli | 9 +++++ infer/tests/codetoanalyze/java/checkers/BUCK | 1 + .../java/checkers/ExpensiveCallExample.java | 14 +++++++ .../java/checkers/ExpensiveCallTest.java | 2 + 10 files changed, 86 insertions(+), 23 deletions(-) diff --git a/infer/src/backend/interproc.ml b/infer/src/backend/interproc.ml index 3008f06ff..67695198a 100644 --- a/infer/src/backend/interproc.ml +++ b/infer/src/backend/interproc.ml @@ -631,7 +631,8 @@ let report_context_leaks pname sigma tenv = IList.fold_left (fun exps hpred -> match hpred with | Sil.Hpointsto (_, Sil.Eexp (exp, _), Sil.Sizeof (Sil.Tptr (typ, _), _)) - when AndroidFramework.is_context typ tenv -> + when AndroidFramework.is_context typ tenv + && not (AndroidFramework.is_application typ tenv) -> (exp, typ) :: exps | _ -> exps) [] diff --git a/infer/src/backend/mangled.ml b/infer/src/backend/mangled.ml index 3f2869bcd..cd9249a55 100644 --- a/infer/src/backend/mangled.ml +++ b/infer/src/backend/mangled.ml @@ -57,7 +57,8 @@ let get_mangled pn = match pn.mangled with (** Create a mangled type name from a package name and a class name *) let from_package_class package_name class_name = - from_string (package_name ^ "." ^ class_name) + if package_name = "" then from_string class_name + else from_string (package_name ^ "." ^ class_name) (** Pretty print a mangled name *) let pp f pn = diff --git a/infer/src/backend/procname.ml b/infer/src/backend/procname.ml index a7d0f46cb..bd711bd21 100644 --- a/infer/src/backend/procname.ml +++ b/infer/src/backend/procname.ml @@ -220,8 +220,14 @@ let java_get_class_components proc_name = Str.split (Str.regexp (Str.quote ".")) (java_get_class proc_name) (** Return the class name of a java procedure name. *) -let java_get_simple_class proc_name = - IList.hd (IList.rev (java_get_class_components proc_name)) +let java_get_simple_class = function + | JAVA j -> snd j.classname + | _ -> assert false + +(** Return the package of a java procname. *) +let java_get_package = function + | JAVA j -> fst j.classname + | _ -> assert false (** Return the method of a java procname. *) let java_get_method = function diff --git a/infer/src/backend/procname.mli b/infer/src/backend/procname.mli index 370863a52..f798af77c 100644 --- a/infer/src/backend/procname.mli +++ b/infer/src/backend/procname.mli @@ -84,6 +84,9 @@ val java_get_class : t -> string (** Return the simple class name of a java procedure name. *) val java_get_simple_class : t -> string +(** Return the package name of a java procedure name. *) +val java_get_package : t -> string option + (** Return the method name of a java procedure name. *) val java_get_method : t -> string diff --git a/infer/src/checkers/performanceCritical.ml b/infer/src/checkers/performanceCritical.ml index 8219e497d..f6f54b9a5 100644 --- a/infer/src/checkers/performanceCritical.ml +++ b/infer/src/checkers/performanceCritical.ml @@ -59,8 +59,26 @@ let method_overrides_performance_critical tenv pname = || overrides () -let method_is_expensive pname = - check_method Annotations.ia_is_expensive pname +let is_modeled_expensive tenv pname = + if SymExec.function_is_builtin pname then false + else if Procname.java_get_method pname <> "findViewById" then false + else + let package = + match Procname.java_get_package pname with + | None -> "" + | Some p -> p in + let classname = + Mangled.from_package_class package (Procname.java_get_simple_class pname) in + match Sil.get_typ classname (Some Sil.Class) tenv with + | None -> false + | Some typ -> + AndroidFramework.is_view typ tenv + || AndroidFramework.is_activity typ tenv + + +let method_is_expensive tenv pname = + is_modeled_expensive tenv pname + || check_method Annotations.ia_is_expensive pname let lookup_call_trees pname = @@ -74,13 +92,13 @@ let lookup_call_trees pname = end -let collect_expensive_call caller_pdesc checked_pnames call_trees (pname, _) = +let collect_expensive_call tenv caller_pdesc checked_pnames call_trees (pname, _) = if Procname.Set.mem pname !checked_pnames then call_trees else begin Ondemand.do_analysis caller_pdesc pname; checked_pnames := Procname.Set.add pname !checked_pnames; - if method_is_expensive pname then + if method_is_expensive tenv pname then (CallTree.Direct pname) :: call_trees else match lookup_call_trees pname with @@ -134,7 +152,7 @@ let check_one_procedure tenv pname pdesc = and performance_critical = is_performance_critical attributes in let check_expensive_subtyping_rules overridden_pname = - if not (method_is_expensive overridden_pname) then + if not (method_is_expensive tenv overridden_pname) then let description = Printf.sprintf "Method `%s` overrides unannotated method `%s` and cannot be annotated with `@%s`" @@ -165,21 +183,21 @@ let check_one_procedure tenv pname pdesc = let expensive_call_trees = let checked_pnames = ref Procname.Set.empty in Cfg.Procdesc.fold_calls - (collect_expensive_call pdesc checked_pnames) [] pdesc in + (collect_expensive_call tenv pdesc checked_pnames) [] pdesc in update_summary expensive_call_trees pname; match expensive_call_trees with | [] -> () + | call_trees when performance_critical -> + report_expensive_calls pname pdesc loc call_trees | call_trees when infer_performance_critical_methods -> if method_overrides_performance_critical tenv pname then report_expensive_calls pname pdesc loc call_trees - | call_trees when performance_critical -> - report_expensive_calls pname pdesc loc call_trees | _ -> () -let callback_performance_checker _ get_proc_desc _ tenv pname proc_desc = +let callback_performance_checker _ get_proc_desc _ tenv pname pdesc = let callbacks = let analyze_ondemand pn = match get_proc_desc pn with @@ -187,10 +205,10 @@ let callback_performance_checker _ get_proc_desc _ tenv pname proc_desc = | Some pd -> check_one_procedure tenv pn pd in { Ondemand.analyze_ondemand; get_proc_desc; } in if !Config.ondemand_enabled - || Ondemand.procedure_should_be_analyzed proc_desc pname + || Ondemand.procedure_should_be_analyzed pdesc pname then begin Ondemand.set_callbacks callbacks; - check_one_procedure tenv pname proc_desc; + check_one_procedure tenv pname pdesc; Ondemand.unset_callbacks () end diff --git a/infer/src/harness/androidFramework.ml b/infer/src/harness/androidFramework.ml index ad3e9a57b..106b21dcc 100644 --- a/infer/src/harness/androidFramework.ml +++ b/infer/src/harness/androidFramework.ml @@ -284,15 +284,23 @@ let get_all_supertypes typ tenv = let is_subtype (typ0 : Sil.typ) (typ1 : Sil.typ) tenv = TypSet.mem typ1 (get_all_supertypes typ0 tenv) -(** return true if [typ] <: android.content.Context *) +let is_subtype_package_class typ package classname tenv = + let classname = Mangled.from_package_class package classname in + match Sil.get_typ classname (Some Sil.Class) tenv with + | Some found_typ -> is_subtype typ found_typ tenv + | _ -> false + let is_context typ tenv = - let context_classname = Mangled.from_package_class "android.content" "Context" - and application_classname = Mangled.from_package_class "android.app" "Application" in - let subclass_of classname = - match Sil.get_typ classname (Some Sil.Class) tenv with - | Some class_typ -> is_subtype typ class_typ tenv - | None -> false in - subclass_of context_classname && not (subclass_of application_classname) + is_subtype_package_class typ "android.content" "Context" tenv + +let is_application typ tenv = + is_subtype_package_class typ "android.app" "Application" tenv + +let is_activity typ tenv = + is_subtype_package_class typ "android.app" "Activity" tenv + +let is_view typ tenv = + is_subtype_package_class typ "android.view" "View" tenv (** return true if [class_name] is a known callback class name *) let is_callback_class_name class_name = Mangled.MangledSet.mem class_name android_callbacks diff --git a/infer/src/harness/androidFramework.mli b/infer/src/harness/androidFramework.mli index c3b608719..79fa91c5e 100644 --- a/infer/src/harness/androidFramework.mli +++ b/infer/src/harness/androidFramework.mli @@ -33,6 +33,15 @@ val is_callback_class : Sil.typ -> Sil.tenv -> bool (** return true if [typ] <: android.content.Context *) val is_context : Sil.typ -> Sil.tenv -> bool +(** return true if [typ] <: android.app.Application *) +val is_application : Sil.typ -> Sil.tenv -> bool + +(** return true if [typ] <: android.app.Activity *) +val is_activity : Sil.typ -> Sil.tenv -> bool + +(** return true if [typ] <: android.view.View *) +val is_view : Sil.typ -> Sil.tenv -> bool + (** return true if [procname] is a special lifecycle cleanup method *) val is_destroy_method : Procname.t -> bool diff --git a/infer/tests/codetoanalyze/java/checkers/BUCK b/infer/tests/codetoanalyze/java/checkers/BUCK index bbc4de4c2..8f58640b0 100644 --- a/infer/tests/codetoanalyze/java/checkers/BUCK +++ b/infer/tests/codetoanalyze/java/checkers/BUCK @@ -1,6 +1,7 @@ sources = glob(['**/*.java']) dependencies = [ + '//dependencies/java/android/support/v4:android-support-v4', '//infer/annotations:annotations', '//infer/lib/java/android:android', ] diff --git a/infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java b/infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java index 9268c93f3..4e34f8a92 100644 --- a/infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java +++ b/infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java @@ -9,6 +9,10 @@ package codetoanalyze.java.checkers; +import android.support.v4.app.FragmentActivity; +import android.widget.ImageView; +import android.view.View; + import com.facebook.infer.annotation.Expensive; import com.facebook.infer.annotation.PerformanceCritical; @@ -70,4 +74,14 @@ public class ExpensiveCallExample { callsExpensive2(); } + @PerformanceCritical + View callsFindViewByIdFromView(ImageView view, int id) { + return view.findViewById(id); + } + + @PerformanceCritical + View callsFindViewByIdFromActivity(FragmentActivity activity, int id) { + return activity.findViewById(id); + } + } diff --git a/infer/tests/endtoend/java/checkers/ExpensiveCallTest.java b/infer/tests/endtoend/java/checkers/ExpensiveCallTest.java index dfb6b92ed..948d70920 100644 --- a/infer/tests/endtoend/java/checkers/ExpensiveCallTest.java +++ b/infer/tests/endtoend/java/checkers/ExpensiveCallTest.java @@ -44,6 +44,8 @@ public class ExpensiveCallTest { "indirectlyCallingExpensiveMethod", "callingExpensiveMethodFromInterface", "longerCallStackToExpensive", + "callsFindViewByIdFromView", + "callsFindViewByIdFromActivity", }; assertThat( "Results should contain " + CALLS_EXPENSIVE_METHOD,