Fix persisting which resources use feature flags

File resources get into the table a different way when they haven't been
modified by the FlaggedXmlVersioner or XmlCompatVersioner and this
addresses that path.

With this change, during compile we now find if an xml resource uses
flags and store that in the XmlResource which is then persisted to and
retrieved from the proto. This means the FlaggedXmlVersioner doesn't
have to look for them in the case that the file config or minsdk is >=
baklava.

This also moved the FeatureFlagsFilter to before we version the files.
The FeatureFlagsFilter is what strips elements behind disabled read only
flags at compile time and removed the featureFlag attribute when the
element is behind an enabled read only flag. This is so that the
FlaggedXmlVersioner doesn't have to worry if the flags it sees are
read/write or readonly. It should only ever encounter read/write flags.

Test: Automation
Bug: 377974898
Flag: android.content.res.layout_readwrite_flags
Change-Id: Ia6cbf55bc9f8d594eeb5c44c143565e93684ae2c

9 files changed, 86 insertions, 57 deletions

diff --git a/tools/aapt2/ResourcesInternal.proto b/tools/aapt2/ResourcesInternal.proto
index f4735a2f6ce7..380c5f21103c 100644
--- a/tools/aapt2/ResourcesInternal.proto
+++ b/tools/aapt2/ResourcesInternal.proto
@@ -50,8 +50,11 @@ message CompiledFile {
   // Any symbols this file auto-generates/exports (eg. @+id/foo in an XML file).
   repeated Symbol exported_symbol = 5;
 
-  // The status of the flag the file is behind if any
+  // The status of the read only flag the file is behind if any
   uint32 flag_status = 6;
   bool flag_negated = 7;
   string flag_name = 8;
+
+  // Whether the file uses read/write feature flags
+  bool uses_readwrite_feature_flags = 9;
 }
diff --git a/tools/aapt2/cmd/Compile.cpp b/tools/aapt2/cmd/Compile.cpp
index a5e18d35a256..3b4f5429f254 100644
--- a/tools/aapt2/cmd/Compile.cpp
+++ b/tools/aapt2/cmd/Compile.cpp
@@ -407,6 +407,45 @@ static bool IsValidFile(IAaptContext* context, const std::string& input_path) {
   return true;
 }
 
+class FindReadWriteFlagsVisitor : public xml::Visitor {
+ public:
+  FindReadWriteFlagsVisitor(const FeatureFlagValues& feature_flag_values)
+      : feature_flag_values_(feature_flag_values) {
+  }
+
+  void Visit(xml::Element* node) override {
+    if (had_flags_) {
+      return;
+    }
+    auto* attr = node->FindAttribute(xml::kSchemaAndroid, xml::kAttrFeatureFlag);
+    if (attr != nullptr) {
+      std::string_view flag_name = util::TrimWhitespace(attr->value);
+      if (flag_name.starts_with('!')) {
+        flag_name = flag_name.substr(1);
+      }
+      if (auto it = feature_flag_values_.find(flag_name); it != feature_flag_values_.end()) {
+        if (!it->second.read_only) {
+          had_flags_ = true;
+          return;
+        }
+      } else {
+        // Flag not passed to aapt2, must evaluate at runtime
+        had_flags_ = true;
+        return;
+      }
+    }
+    VisitChildren(node);
+  }
+
+  bool HadFlags() const {
+    return had_flags_;
+  }
+
+ private:
+  bool had_flags_ = false;
+  const FeatureFlagValues& feature_flag_values_;
+};
+
 static bool CompileXml(IAaptContext* context, const CompileOptions& options,
                        const ResourcePathData& path_data, io::IFile* file, IArchiveWriter* writer,
                        const std::string& output_path) {
@@ -436,6 +475,10 @@ static bool CompileXml(IAaptContext* context, const CompileOptions& options,
   xmlres->file.type = ResourceFile::Type::kProtoXml;
   xmlres->file.flag = ParseFlag(path_data.flag_name);
 
+  FindReadWriteFlagsVisitor visitor(options.feature_flag_values);
+  xmlres->root->Accept(&visitor);
+  xmlres->file.uses_readwrite_feature_flags = visitor.HadFlags();
+
   if (xmlres->file.flag) {
     std::string error;
     auto flag_status = GetFlagStatus(xmlres->file.flag, options.feature_flag_values, &error);
diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp
index 755dbb6f8e42..0e18ee250993 100644
--- a/tools/aapt2/cmd/Link.cpp
+++ b/tools/aapt2/cmd/Link.cpp
@@ -615,6 +615,8 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
             file_op.xml_to_flatten->file.source = file_ref->GetSource();
             file_op.xml_to_flatten->file.name =
                 ResourceName(pkg->name, type->named_type, entry->name);
+            file_op.xml_to_flatten->file.uses_readwrite_feature_flags =
+                config_value->uses_readwrite_feature_flags;
           }
 
           // NOTE(adamlesinski): Explicitly construct a StringPiece here, or
@@ -647,6 +649,17 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
             }
           }
 
+          FeatureFlagsFilterOptions flags_filter_options;
+          // Don't fail on unrecognized flags or flags without values as these flags might be
+          // defined and have a value by the time they are evaluated at runtime.
+          flags_filter_options.fail_on_unrecognized_flags = false;
+          flags_filter_options.flags_must_have_value = false;
+          flags_filter_options.remove_disabled_elements = true;
+          FeatureFlagsFilter flags_filter(options_.feature_flag_values, flags_filter_options);
+          if (!flags_filter.Consume(context_, file_op.xml_to_flatten.get())) {
+            return 1;
+          }
+
           std::vector<std::unique_ptr<xml::XmlResource>> versioned_docs =
               LinkAndVersionXmlFile(table, &file_op);
           if (versioned_docs.empty()) {
@@ -673,6 +686,7 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
 
               // Update the output format of this XML file.
               file_ref->type = XmlFileTypeForOutputFormat(options_.output_format);
+
               bool result = table->AddResource(
                   NewResourceBuilder(file.name)
                       .SetValue(std::move(file_ref), file.config)
@@ -685,14 +699,6 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
               }
             }
 
-            FeatureFlagsFilterOptions flags_filter_options;
-            flags_filter_options.fail_on_unrecognized_flags = false;
-            flags_filter_options.flags_must_have_value = false;
-            FeatureFlagsFilter flags_filter(options_.feature_flag_values, flags_filter_options);
-            if (!flags_filter.Consume(context_, doc.get())) {
-              return 1;
-            }
-
             error |= !FlattenXml(context_, *doc, dst_path, options_.keep_raw_values,
                                  false /*utf16*/, options_.output_format, archive_writer);
           }
diff --git a/tools/aapt2/format/proto/ProtoDeserialize.cpp b/tools/aapt2/format/proto/ProtoDeserialize.cpp
index 91ec3485ac3b..b8936553a193 100644
--- a/tools/aapt2/format/proto/ProtoDeserialize.cpp
+++ b/tools/aapt2/format/proto/ProtoDeserialize.cpp
@@ -640,6 +640,7 @@ bool DeserializeCompiledFileFromPb(const pb::internal::CompiledFile& pb_file,
   out_file->name = name_ref.ToResourceName();
   out_file->source.path = pb_file.source_path();
   out_file->type = DeserializeFileReferenceTypeFromPb(pb_file.type());
+  out_file->uses_readwrite_feature_flags = pb_file.uses_readwrite_feature_flags();
 
   out_file->flag_status = (FlagStatus)pb_file.flag_status();
   if (!pb_file.flag_name().empty()) {
diff --git a/tools/aapt2/format/proto/ProtoSerialize.cpp b/tools/aapt2/format/proto/ProtoSerialize.cpp
index fcc77d5a9d6d..da99c4f5917c 100644
--- a/tools/aapt2/format/proto/ProtoSerialize.cpp
+++ b/tools/aapt2/format/proto/ProtoSerialize.cpp
@@ -767,6 +767,7 @@ void SerializeCompiledFileToPb(const ResourceFile& file, pb::internal::CompiledF
     out_file->set_flag_negated(file.flag->negated);
     out_file->set_flag_name(file.flag->name);
   }
+  out_file->set_uses_readwrite_feature_flags(file.uses_readwrite_feature_flags);
 
   for (const SourcedResourceName& exported : file.exported_symbols) {
     pb::internal::CompiledFile_Symbol* pb_symbol = out_file->add_exported_symbol();
diff --git a/tools/aapt2/link/FlaggedResources_test.cpp b/tools/aapt2/link/FlaggedResources_test.cpp
index 47a71fe36e9f..4dcb8507fa45 100644
--- a/tools/aapt2/link/FlaggedResources_test.cpp
+++ b/tools/aapt2/link/FlaggedResources_test.cpp
@@ -226,9 +226,11 @@ TEST_F(FlaggedResourcesTest, ReadWriteFlagInXmlGetsFlagged) {
       }
     }
   }
+
   ASSERT_TRUE(found) << "No entry for layout1 at v36 with FLAG_USES_FEATURE_FLAGS bit set";
-  // There should only be 1 entry that has the FLAG_USES_FEATURE_FLAGS bit of flags set to 1
-  ASSERT_EQ(fields_flagged, 1);
+  // There should only be 2 entry that has the FLAG_USES_FEATURE_FLAGS bit of flags set to 1, the
+  // three versions of the layout file that has flags
+  ASSERT_EQ(fields_flagged, 3);
 }
 
 }  // namespace aapt
diff --git a/tools/aapt2/link/FlaggedXmlVersioner.cpp b/tools/aapt2/link/FlaggedXmlVersioner.cpp
index 8a3337c446cb..626cae73bfa2 100644
--- a/tools/aapt2/link/FlaggedXmlVersioner.cpp
+++ b/tools/aapt2/link/FlaggedXmlVersioner.cpp
@@ -35,10 +35,6 @@ class AllDisabledFlagsVisitor : public xml::Visitor {
     VisitChildren(node);
   }
 
-  bool HadFlags() const {
-    return had_flags_;
-  }
-
  private:
   bool FixupOrShouldRemove(const std::unique_ptr<xml::Node>& node) {
     if (auto* el = NodeCast<Element>(node.get())) {
@@ -47,7 +43,6 @@ class AllDisabledFlagsVisitor : public xml::Visitor {
         return false;
       }
 
-      had_flags_ = true;
       // This class assumes all flags are disabled so we want to remove any elements behind flags
       // unless the flag specification is negated. In the negated case we remove the featureFlag
       // attribute because we have already determined whether we are keeping the element or not.
@@ -62,56 +57,27 @@ class AllDisabledFlagsVisitor : public xml::Visitor {
 
     return false;
   }
-
-  bool had_flags_ = false;
-};
-
-// An xml visitor that goes through the a doc and determines if any elements are behind a flag.
-class FindFlagsVisitor : public xml::Visitor {
- public:
-  void Visit(xml::Element* node) override {
-    if (had_flags_) {
-      return;
-    }
-    auto* attr = node->FindAttribute(xml::kSchemaAndroid, xml::kAttrFeatureFlag);
-    if (attr != nullptr) {
-      had_flags_ = true;
-      return;
-    }
-    VisitChildren(node);
-  }
-
-  bool HadFlags() const {
-    return had_flags_;
-  }
-
-  bool had_flags_ = false;
 };
 
 std::vector<std::unique_ptr<xml::XmlResource>> FlaggedXmlVersioner::Process(IAaptContext* context,
                                                                             xml::XmlResource* doc) {
   std::vector<std::unique_ptr<xml::XmlResource>> docs;
-  if ((static_cast<ApiVersion>(doc->file.config.sdkVersion) >= SDK_BAKLAVA) ||
-      (static_cast<ApiVersion>(context->GetMinSdkVersion()) >= SDK_BAKLAVA)) {
+  if (!doc->file.uses_readwrite_feature_flags) {
+    docs.push_back(doc->Clone());
+  } else if ((static_cast<ApiVersion>(doc->file.config.sdkVersion) >= SDK_BAKLAVA) ||
+             (static_cast<ApiVersion>(context->GetMinSdkVersion()) >= SDK_BAKLAVA)) {
     // Support for read/write flags was added in baklava so if the doc will only get used on
     // baklava or later we can just return the original doc.
     docs.push_back(doc->Clone());
-    FindFlagsVisitor visitor;
-    doc->root->Accept(&visitor);
-    docs.back()->file.uses_readwrite_feature_flags = visitor.HadFlags();
   } else {
     auto preBaklavaVersion = doc->Clone();
     AllDisabledFlagsVisitor visitor;
     preBaklavaVersion->root->Accept(&visitor);
-    preBaklavaVersion->file.uses_readwrite_feature_flags = false;
     docs.push_back(std::move(preBaklavaVersion));
 
-    if (visitor.HadFlags()) {
-      auto baklavaVersion = doc->Clone();
-      baklavaVersion->file.config.sdkVersion = SDK_BAKLAVA;
-      baklavaVersion->file.uses_readwrite_feature_flags = true;
-      docs.push_back(std::move(baklavaVersion));
-    }
+    auto baklavaVersion = doc->Clone();
+    baklavaVersion->file.config.sdkVersion = SDK_BAKLAVA;
+    docs.push_back(std::move(baklavaVersion));
   }
   return docs;
 }
diff --git a/tools/aapt2/link/FlaggedXmlVersioner_test.cpp b/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
index 0c1314f165cc..0dc464253385 100644
--- a/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
+++ b/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
@@ -101,6 +101,7 @@ TEST_F(FlaggedXmlVersionerTest, PreBaklavaGetsSplit) {
         <TextView android:featureFlag="package.flag" /><TextView /><TextView />
       </LinearLayout>)");
   doc->file.config.sdkVersion = SDK_GINGERBREAD;
+  doc->file.uses_readwrite_feature_flags = true;
 
   FlaggedXmlVersioner versioner;
   auto results = versioner.Process(context_.get(), doc.get());
@@ -131,6 +132,7 @@ TEST_F(FlaggedXmlVersionerTest, NoVersionGetsSplit) {
       <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android">
         <TextView android:featureFlag="package.flag" /><TextView /><TextView />
       </LinearLayout>)");
+  doc->file.uses_readwrite_feature_flags = true;
 
   FlaggedXmlVersioner versioner;
   auto results = versioner.Process(context_.get(), doc.get());
@@ -162,6 +164,7 @@ TEST_F(FlaggedXmlVersionerTest, NegatedFlagAttributeRemoved) {
         <TextView android:featureFlag="!package.flag" /><TextView /><TextView />
       </LinearLayout>)");
   doc->file.config.sdkVersion = SDK_GINGERBREAD;
+  doc->file.uses_readwrite_feature_flags = true;
 
   FlaggedXmlVersioner versioner;
   auto results = versioner.Process(context_.get(), doc.get());
@@ -192,6 +195,7 @@ TEST_F(FlaggedXmlVersionerTest, NegatedFlagAttributeRemovedNoSpecifiedVersion) {
       <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android">
         <TextView android:featureFlag="!package.flag" /><TextView /><TextView />
       </LinearLayout>)");
+  doc->file.uses_readwrite_feature_flags = true;
 
   FlaggedXmlVersioner versioner;
   auto results = versioner.Process(context_.get(), doc.get());
diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp
index 1d4adc4a57d8..17f332397317 100644
--- a/tools/aapt2/link/TableMerger.cpp
+++ b/tools/aapt2/link/TableMerger.cpp
@@ -295,6 +295,8 @@ bool TableMerger::DoMerge(const android::Source& src, ResourceTablePackage* src_
           dst_config_value =
               dst_entry->FindOrCreateValue(src_config_value->config, src_config_value->product);
         }
+        dst_config_value->uses_readwrite_feature_flags |=
+            src_config_value->uses_readwrite_feature_flags;
 
         // Continue if we're taking the new resource.
         CloningValueTransformer cloner(&main_table_->string_pool);
@@ -378,12 +380,13 @@ bool TableMerger::MergeFile(const ResourceFile& file_desc, bool overlay, io::IFi
   file_ref->file = file;
   file_ref->SetFlagStatus(file_desc.flag_status);
   file_ref->SetFlag(file_desc.flag);
-
   ResourceTablePackage* pkg = table.FindOrCreatePackage(file_desc.name.package);
-  pkg->FindOrCreateType(file_desc.name.type)
-      ->FindOrCreateEntry(file_desc.name.entry)
-      ->FindOrCreateValue(file_desc.config, {})
-      ->value = std::move(file_ref);
+  ResourceConfigValue* config_value = pkg->FindOrCreateType(file_desc.name.type)
+                                          ->FindOrCreateEntry(file_desc.name.entry)
+                                          ->FindOrCreateValue(file_desc.config, {});
+
+  config_value->value = std::move(file_ref);
+  config_value->uses_readwrite_feature_flags = file_desc.uses_readwrite_feature_flags;
 
   return DoMerge(file->GetSource(), pkg, false /*mangle*/, overlay /*overlay*/, true /*allow_new*/);
 }