Checker that complains when Fragments don't nullify their Views

Reviewed By: jeremydubreil

Differential Revision: D2905842

fb-gh-sync-id: 08b8b58

infer/src/checkers/callbackChecker.ml

@@ -16,22 +16,6 @@ module IdSet = Ident.IdentSet
 module FldSet = Ident.FieldSet
 open Utils
 
-(** Automatically create a harness method to exercise code under test *)
-
-(** return the set of instance fields that are assigned to a null literal in [procdesc] *)
-let get_fields_nullified procdesc =
-  (* walk through the instructions and look for instance fields that are assigned to null *)
-  let collect_nullified_flds (nullified_flds, this_ids) _ = function
-    | Sil.Set (Sil.Lfield (Sil.Var lhs, fld, _), typ, rhs, loc)
-      when Sil.exp_is_null_literal rhs && IdSet.mem lhs this_ids ->
-        (FldSet.add fld nullified_flds, this_ids)
-    | Sil.Letderef (id, rhs, _, _) when Sil.exp_is_this rhs ->
-        (nullified_flds, IdSet.add id this_ids)
-    | _ -> (nullified_flds, this_ids) in
-  let (nullified_flds, _) =
-    Cfg.Procdesc.fold_instrs collect_nullified_flds (FldSet.empty, IdSet.empty) procdesc in
-  nullified_flds
-
 (** set of instance fields belonging to the current file that are assigned to null literals *)
 let fields_nullified = ref FldSet.empty
 
@@ -112,7 +96,8 @@ let callback_checker_main all_procs get_procdesc idenv tenv proc_name proc_desc
         let _ = if AndroidFramework.is_destroy_method proc_name then
             (* compute the set of fields nullified by this procedure *)
             (* TODO (t4959422): get fields that are nullified in callees of the destroy method *)
-            fields_nullified := FldSet.union (get_fields_nullified proc_desc) !fields_nullified in
+            fields_nullified :=
+              FldSet.union (PatternMatch.get_fields_nullified proc_desc) !fields_nullified in
         if done_checking (IList.length def_methods) then
           do_eradicate_check all_procs get_procdesc idenv tenv
   | _ -> ()

infer/src/checkers/callbackChecker.mli

@@ -9,7 +9,4 @@
 
 (** Make sure callbacks are always unregistered. drive the point home by reporting possible NPE's *)
 
-(** return the set of instance fields that are assigned to a null literal in [procdesc] *)
-val get_fields_nullified : Cfg.Procdesc.t -> Ident.FieldSet.t
-
 val callback_checker_main : Callbacks.proc_callback_t

infer/src/checkers/fragmentRetainsViewChecker.ml

@@ -0,0 +1,62 @@
+(*
+ * Copyright (c) 2013 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *)
+
+(** Make sure callbacks are always unregistered. drive the point home by reporting possible NPE's *)
+
+module P = Printf
+open Utils
+
+let report_error fld fld_typ pname pdesc =
+  let retained_view = "CHECKERS_FRAGMENT_RETAINS_VIEW" in
+  let fld_decl_class, fld_name =
+    Ident.java_fieldname_get_class fld, Ident.java_fieldname_get_field fld in
+  let description =
+    P.sprintf
+      "Fragment %s does not nullify View field %s (type %s) in onDestroyView. If the Fragment is placed on the back stack, a reference to the View may be retained."
+      fld_decl_class
+      fld_name
+      (Sil.typ_to_string (Sil.typ_strip_ptr fld_typ)) in
+  let loc = Cfg.Procdesc.get_loc pdesc in
+  let exn = Exceptions.Checkers (retained_view, Localise.verbatim_desc description) in
+  Reporting.log_error pname ~loc:(Some loc) exn
+
+let callback_fragment_retains_view all_procs get_procdesc idenv tenv pname pdesc =
+  (* TODO: complain if onDestroyView is not defined, yet the Fragment has View fields *)
+  (* TODO: handle fields nullified in callees in the same file *)
+  let is_on_destroy_view = Procname.java_get_method pname = "onDestroyView" in
+  (* this is needlessly complicated because field types are Tvars instead of Tstructs *)
+  let fld_typ_is_view = function
+    | Sil.Tptr (Sil.Tvar tname, _) ->
+        begin
+          match Sil.tenv_lookup tenv tname with
+          | Some typ -> AndroidFramework.is_view typ tenv
+          | None -> false
+        end
+    | _ -> false in
+  (* is [fldname] a View type declared by [class_typename]? *)
+  let is_declared_view_typ class_typename (fldname, fld_typ, _) =
+    let fld_classname = Typename.Java.from_string (Ident.java_fieldname_get_class fldname) in
+    Typename.equal fld_classname class_typename && fld_typ_is_view fld_typ in
+  if is_on_destroy_view then
+    begin
+      let class_typename = Typename.Java.from_string (Procname.java_get_class pname) in
+      match Sil.tenv_lookup tenv class_typename with
+      | Some (Sil.Tstruct { Sil.csu; struct_name = Some class_name; def_methods; instance_fields }
+              as typ) when AndroidFramework.is_fragment typ tenv ->
+          let declared_view_fields =
+            IList.filter (is_declared_view_typ class_typename) instance_fields in
+          let fields_nullified = PatternMatch.get_fields_nullified pdesc in
+          (* report if a field is declared by C, but not nulled out in C.onDestroyView *)
+          IList.iter
+            (fun (fname, typ, _) ->
+               if not (Ident.FieldSet.mem fname fields_nullified) then
+                 report_error fname typ pname pdesc)
+            declared_view_fields
+      | _ -> ()
+    end

infer/src/checkers/patternMatch.ml

@@ -328,3 +328,18 @@ let proc_iter_overridden_methods f tenv proc_name =
     | Some curr_type ->
         IList.iter (do_super_type tenv) (type_get_direct_supertypes curr_type)
     | None -> ()
+
+(** return the set of instance fields that are assigned to a null literal in [procdesc] *)
+let get_fields_nullified procdesc =
+  (* walk through the instructions and look for instance fields that are assigned to null *)
+  let collect_nullified_flds (nullified_flds, this_ids) _ = function
+    | Sil.Set (Sil.Lfield (Sil.Var lhs, fld, _), typ, rhs, loc)
+      when Sil.exp_is_null_literal rhs && Ident.IdentSet.mem lhs this_ids ->
+        (Ident.FieldSet.add fld nullified_flds, this_ids)
+    | Sil.Letderef (id, rhs, _, _) when Sil.exp_is_this rhs ->
+        (nullified_flds, Ident.IdentSet.add id this_ids)
+    | _ -> (nullified_flds, this_ids) in
+  let (nullified_flds, _) =
+    Cfg.Procdesc.fold_instrs
+      collect_nullified_flds (Ident.FieldSet.empty, Ident.IdentSet.empty) procdesc in
+  nullified_flds

infer/src/checkers/patternMatch.mli

@@ -80,3 +80,6 @@ val type_is_nested_in_type : Sil.typ -> Mangled.t -> bool
 
 (** Is the type java.lang.Object *)
 val type_is_object : Sil.typ -> bool
+
+(** return the set of instance fields that are assigned to a null literal in [procdesc] *)
+val get_fields_nullified : Cfg.Procdesc.t -> Ident.FieldSet.t

infer/src/checkers/registerCheckers.ml

@@ -28,6 +28,7 @@ let active_procedure_checkers () =
         Checkers.callback_check_write_to_parcel, false;
         Checkers.callback_find_deserialization, false;
         Dataflow.callback_test_dataflow, false;
+        FragmentRetainsViewChecker.callback_fragment_retains_view, checkers_enabled;
         SqlChecker.callback_sql, false;
         Eradicate.callback_eradicate, !Config.eradicate;
         CodeQuery.code_query_callback, !CodeQuery.query <> None;

infer/src/harness/androidFramework.ml

@@ -289,6 +289,10 @@ let is_activity typ tenv =
 let is_view typ tenv =
   is_subtype_package_class typ "android.view" "View" tenv
 
+let is_fragment typ tenv =
+  is_subtype_package_class typ "android.app" "Fragment" tenv ||
+  is_subtype_package_class typ "android.support.v4.app" "Fragment" 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
 

infer/src/harness/androidFramework.mli

@@ -32,6 +32,8 @@ val is_activity : Sil.typ -> Sil.tenv -> bool
 (** return true if [typ] <: android.view.View *)
 val is_view : Sil.typ -> Sil.tenv -> bool
 
+val is_fragment : Sil.typ -> Sil.tenv -> bool
+
 (** return true if [procname] is a special lifecycle cleanup method *)
 val is_destroy_method : Procname.t -> bool
 

infer/tests/codetoanalyze/java/checkers/FragmentDoesNotRetainViewExample.java

@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2016 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package codetoanalyze.java.checkers;
+
+import android.content.Context;
+import android.os.Bundle;
+import android.support.v4.app.Fragment;
+import android.view.LayoutInflater;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.ListView;
+
+public class FragmentDoesNotRetainViewExample extends Fragment {
+
+  class CustomView extends ListView {
+
+    public CustomView(Context c) {
+      super(c);
+    }
+
+  }
+
+  View mView1;
+  View mView2;
+  ViewGroup mViewSubclass;
+  CustomView mCustomView;
+
+  boolean b;
+
+  @Override
+  public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle bundle) {
+    mView1 = inflater.inflate(-1, container, false);
+    mView2 = inflater.inflate(-1, container, false);
+    mViewSubclass = (ViewGroup) inflater.inflate(-1, container, false);
+    mCustomView = (CustomView) inflater.inflate(-1, container, false);
+    return container;
+  }
+
+  @Override
+  public void onDestroyView() {
+    mView1 = null;
+    if (b) {
+      mView2 = null; // conditional nulling is still ok
+    }
+    mCustomView = null;
+    mViewSubclass = null;
+  }
+
+}

infer/tests/codetoanalyze/java/checkers/FragmentRetainsViewExample.java

@@ -0,0 +1,47 @@
+/*
+ * Copyright (c) 2016 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package codetoanalyze.java.checkers;
+
+import android.content.Context;
+import android.os.Bundle;
+import android.support.v4.app.Fragment;
+import android.view.LayoutInflater;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.ListView;
+
+public class FragmentRetainsViewExample extends Fragment {
+
+  class CustomView extends ListView {
+
+    public CustomView(Context c) {
+      super(c);
+    }
+
+  }
+
+  View mView;
+  ViewGroup mViewSubclass;
+  CustomView mCustomView;
+
+  @Override
+  public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle bundle) {
+    mView = inflater.inflate(-1, container, false);
+    mViewSubclass = (ViewGroup) inflater.inflate(-1, container, false);
+    mCustomView = (CustomView) inflater.inflate(-1, container, false);
+    return container;
+  }
+
+  @Override
+  public void onDestroyView() {
+    // not nulling out anything
+  }
+
+}

infer/tests/endtoend/java/checkers/FragmentDoesNotRetainViewTest.java

@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) 2015 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package endtoend.java.checkers;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static utils.matchers.ResultContainsNoErrorInMethod.doesNotContain;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import utils.InferException;
+import utils.InferResults;
+
+public class FragmentDoesNotRetainViewTest {
+
+  public static final String SOURCE_FILE =
+    "infer/tests/codetoanalyze/java/checkers/FragmentDoesNotRetainViewExample.java";
+
+  public static final String FRAGMENT_RETAINS_VIEW =
+    "CHECKERS_FRAGMENT_RETAINS_VIEW";
+
+  private static InferResults inferResults;
+
+  @BeforeClass
+  public static void loadResults() throws InterruptedException, IOException {
+    inferResults =
+      InferResults.loadCheckersResults(FragmentDoesNotRetainViewTest.class, SOURCE_FILE);
+  }
+
+  @Test
+  public void matchNumberOfErrors()
+      throws IOException, InterruptedException, InferException {
+    assertThat(
+        "Results should contain 0 retained View errors",
+        inferResults,
+        doesNotContain(
+            FRAGMENT_RETAINS_VIEW,
+            SOURCE_FILE,
+            "onDestroyView"
+        )
+    );
+  }
+
+}

infer/tests/endtoend/java/checkers/FragmentRetainsViewTest.java

@@ -0,0 +1,54 @@
+/*
+ * Copyright (c) 2015 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package endtoend.java.checkers;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static utils.matchers.ResultContainsNumberOfErrorsInMethod.containsNumberOfErrors;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import utils.InferException;
+import utils.InferResults;
+
+public class FragmentRetainsViewTest {
+
+  public static final String SOURCE_FILE =
+    "infer/tests/codetoanalyze/java/checkers/FragmentRetainsViewExample.java";
+
+  public static final String FRAGMENT_RETAINS_VIEW =
+    "CHECKERS_FRAGMENT_RETAINS_VIEW";
+
+  private static InferResults inferResults;
+
+  @BeforeClass
+  public static void loadResults() throws InterruptedException, IOException {
+    inferResults =
+      InferResults.loadCheckersResults(FragmentRetainsViewTest.class, SOURCE_FILE);
+  }
+
+  @Test
+  public void matchNumberOfErrors()
+      throws IOException, InterruptedException, InferException {
+    assertThat(
+        "Results should contain 3 retained View errors",
+        inferResults,
+        containsNumberOfErrors(
+            FRAGMENT_RETAINS_VIEW,
+            SOURCE_FILE,
+            "onDestroyView",
+            3
+        )
+    );
+  }
+
+}