Don't visit removed classes

Summary:
When we find a `DexClass` via `type_class(DexType::get_type(name))`, then we must be careful! Since we never actually remove classes from the index referenced by `type_class`, the class might be 1) external, 2) internal (in scope), or... 3) formerly internal, but meanwhile removed, and no longer in scope!

In RMU, this has been happening forever, and it's usually benign. Except... if the removed class used to have annotation, which we have meanwhile also removed; and if the annotation had fields, then we actually released the field's memory. And attempting to access that later blows up.

And this has recently become a problem, as some of the removed classes had indeed (Kotlin) annotations.

This fixes the problem by making sure that RMU never inspects already removed classes.

You might ask how we can possibly be referencing a removed class --- if there's a reference, then it shouldn't have been removed! However, there are many diverse ways of removing a class (the *RMUs can be, and are (!) configured with different ignore sets; RMU is not the only optimization that removes classes), and referencing a class (some optimization might introduce a new string that happens to match the name of removed class). So that's an orthogonal issue (that we still should look into).

Reviewed By: ssj933

Differential Revision: D51837577

fbshipit-source-id: cd4d1002bb1d592e13f13681a0869df72a25d41e

libredex/Reachability.cpp

@@ -378,6 +378,15 @@ void TransitiveClosureMarkerWorker::push(const Parent* parent,
   if (!cls) {
     return;
   }
+  if (!m_shared_state->scope_set.count(cls)) {
+    if (!cls->is_external()) {
+      TRACE(REACH, 2,
+            "Warning: push([%s], class [%s]) where child class is not external "
+            "and not in scope!",
+            SHOW(parent), SHOW(cls));
+    }
+    return;
+  }
   record_reachability(parent, cls);
   if (!m_shared_state->reachable_objects->mark(cls)) {
     return;
@@ -1307,6 +1316,7 @@ void TransitiveClosureMarkerWorker::push_typelike_strings(
 }
 
 void TransitiveClosureMarkerWorker::visit_cls(const DexClass* cls) {
+  always_assert(m_shared_state->scope_set.count(cls));
   TRACE(REACH, 4, "Visiting class: %s", SHOW(cls));
   auto is_interface_instantiable = [](const DexClass* interface) {
     if (is_annotation(interface) || root(interface) || !can_rename(interface)) {
@@ -1909,6 +1919,7 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects(
     bool should_mark_all_as_seed,
     bool remove_no_argument_constructors) {
   Timer t("Marking");
+  std::unordered_set<const DexClass*> scope_set(scope.begin(), scope.end());
   auto reachable_objects = std::make_unique<ReachableObjects>();
   ConditionallyMarked cond_marked;
 
@@ -1927,6 +1938,7 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects(
   size_t num_threads = redex_parallel::default_num_threads();
   Stats stats;
   TransitiveClosureMarkerSharedState shared_state{
+      std::move(scope_set),
       &ignore_sets,
       &method_override_graph,
       record_reachability,

libredex/Reachability.h

@@ -504,6 +504,7 @@ class RootSetMarker {
 class TransitiveClosureMarkerWorker;
 
 struct TransitiveClosureMarkerSharedState {
+  std::unordered_set<const DexClass*> scope_set;
   const IgnoreSets* ignore_sets;
   const method_override_graph::Graph* method_override_graph;
   bool record_reachability;

opt/reachable-natives/ReachableNatives.cpp

@@ -69,6 +69,7 @@ void ReachableNativesPass::run_pass(DexStoresVector& stores,
       ("ReachableNativesPass Run "s + std::to_string(m_run_number)).c_str());
 
   auto scope = build_class_scope(stores);
+  std::unordered_set<const DexClass*> scope_set(scope.begin(), scope.end());
   auto reachable_objects = std::make_unique<reachability::ReachableObjects>();
   reachability::ReachableAspects reachable_aspects;
   reachability::ConditionallyMarked cond_marked;
@@ -94,6 +95,7 @@ void ReachableNativesPass::run_pass(DexStoresVector& stores,
   reachability::IgnoreSets ignore_sets;
   reachability::Stats stats;
   reachability::TransitiveClosureMarkerSharedState shared_state{
+      std::move(scope_set),
       &ignore_sets,
       method_override_graph.get(),
       false,

opt/remove-unreachable/TypeAnalysisAwareRemoveUnreachable.cpp

@@ -356,6 +356,7 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
     int* num_unreachable_invokes,
     int* num_null_invokes) {
   Timer t("Marking");
+  std::unordered_set<const DexClass*> scope_set(scope.begin(), scope.end());
   walk::parallel::code(scope, [](DexMethod*, IRCode& code) {
     code.cfg().calculate_exit_block();
   });
@@ -373,8 +374,8 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
   size_t num_threads = redex_parallel::default_num_threads();
   Stats stats;
   TypeAnalysisAwareClosureMarkerSharedState shared_state{
-      {&ignore_sets, &method_override_graph, record_reachability,
-       relaxed_keep_class_members, relaxed_keep_interfaces,
+      {std::move(scope_set), &ignore_sets, &method_override_graph,
+       record_reachability, relaxed_keep_class_members, relaxed_keep_interfaces,
        cfg_gathering_check_instantiable, cfg_gathering_check_instance_callable,
        cfg_gathering_check_returning, &cond_marked, reachable_objects.get(),
        reachable_aspects, &stats},