Revert "[Traffic Annotation] Remove function_context from extractor output"

This reverts commit 17ccf43e.

Reason for revert: suspecting that this makes the presubmit check not run on most CLs (crbug/949382)

Original change's description:
> [Traffic Annotation] Remove function_context from extractor output
> 
> The new extractor can't trivially determine the name of the enclosing
> function, and it didn't bring much value anyways.
> 
> Bug: 966883
> Change-Id: I7b52050730186c6972754d3b9e577d4c4740d5dc
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1686621
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
> Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#675223}

TBR=thakis@chromium.org,rhalavati@chromium.org,nicolaso@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 966883
Change-Id: I0c326313df18b7b05f7a41cd58e6d1e491277ae5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715423Reviewed-by: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680626}

tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp

@@ -47,6 +47,17 @@ namespace {
 struct Location {
   std::string file_path;
   int line_number = -1;
+
+  // Name of the function including this line. E.g., in the following code,
+  // |function_name| will be 'foo' for all |line_number| values 101-103.
+  //
+  // 100 void foo() {
+  // 101   NetworkTrafficAnnotationTag baz =
+  // 102       net::DefineNetworkTrafficAnnotation(...); }
+  // 103   bar(baz);
+  // 104 }
+  // If no function is found, 'Global Namespace' will be returned.
+  std::string function_name;
 };
 
 // An instance of a call to either of the 4 network traffic annotation
@@ -154,6 +165,13 @@ class NetworkAnnotationTagCallback : public MatchFinder::MatchCallback {
     location->line_number =
         result.SourceManager->getSpellingLineNumber(source_location);
 
+    const clang::FunctionDecl* ancestor =
+        result.Nodes.getNodeAs<clang::FunctionDecl>("function_context");
+    if (ancestor)
+      location->function_name = ancestor->getQualifiedNameAsString();
+    else
+      location->function_name = "Global Namespace";
+
     std::replace(location->file_path.begin(), location->file_path.end(), '\\',
                  '/');
 
@@ -178,20 +196,45 @@ class NetworkAnnotationTagCallback : public MatchFinder::MatchCallback {
     collector_->calls.push_back(instance);
   }
 
+  // Tests if the given function name belongs to the network traffic annotation
+  // API. These functions are all defined in
+  // 'net/traffic_annotation/network_traffic_annotation.h'.
+  bool IsAPIFunction(const std::string& function_name) {
+    return function_name == "net::NetworkTrafficAnnotationTag::NotReached" ||
+           function_name == "net::DefineNetworkTrafficAnnotation" ||
+           function_name == "net::DefinePartialNetworkTrafficAnnotation" ||
+           function_name == "net::CompleteNetworkTrafficAnnotation" ||
+           function_name == "net::BranchedCompleteNetworkTrafficAnnotation" ||
+           function_name ==
+               "net::MutableNetworkTrafficAnnotationTag::operator "
+               "NetworkTrafficAnnotationTag" ||
+           function_name ==
+               "net::MutablePartialNetworkTrafficAnnotationTag::operator "
+               "PartialNetworkTrafficAnnotationTag";
+  }
+
   // Stores an annotation constructor called with list expression.
   void AddConstructor(const clang::CXXConstructExpr* constructor_expr,
                       const MatchFinder::MatchResult& result) {
     Location instance;
+
     GetInstanceLocation(result, constructor_expr, &instance);
-    collector_->assignments.push_back(instance);
+    // Only report if the constructor is not in one of the API functions for
+    // network traffic annotations.
+    if (!IsAPIFunction(instance.function_name))
+      collector_->assignments.push_back(instance);
   }
 
   // Stores a value assignment to |unique_id_hash_code| of a mutable annotaton.
   void AddAssignment(const clang::MemberExpr* member_expr,
                      const MatchFinder::MatchResult& result) {
     Location instance;
+
     GetInstanceLocation(result, member_expr, &instance);
-    collector_->assignments.push_back(instance);
+    // Only report if the assignment is not in one of the API functions for
+    // network traffic annotations.
+    if (!IsAPIFunction(instance.function_name))
+      collector_->assignments.push_back(instance);
   }
 
   // Stores an annotation.
@@ -371,6 +414,7 @@ int main(int argc, const char* argv[]) {
   for (const NetworkAnnotationInstance& instance : collector.annotations) {
     llvm::outs() << "==== NEW ANNOTATION ====\n";
     llvm::outs() << instance.location.file_path << "\n";
+    llvm::outs() << instance.location.function_name << "\n";
     llvm::outs() << instance.location.line_number << "\n";
     llvm::outs() << instance.GetTypeName() << "\n";
     llvm::outs() << instance.annotation.unique_id << "\n";
@@ -383,6 +427,7 @@ int main(int argc, const char* argv[]) {
   for (const CallInstance& instance : collector.calls) {
     llvm::outs() << "==== NEW CALL ====\n";
     llvm::outs() << instance.location.file_path << "\n";
+    llvm::outs() << instance.location.function_name << "\n";
     llvm::outs() << instance.location.line_number << "\n";
     llvm::outs() << instance.called_function_name << "\n";
     llvm::outs() << instance.has_annotation << "\n";
@@ -393,6 +438,7 @@ int main(int argc, const char* argv[]) {
   for (const Location& instance : collector.assignments) {
     llvm::outs() << "==== NEW ASSIGNMENT ====\n";
     llvm::outs() << instance.file_path << "\n";
+    llvm::outs() << instance.function_name << "\n";
     llvm::outs() << instance.line_number << "\n";
     llvm::outs() << "==== ASSIGNMENT ENDS ====\n";
   }

tools/traffic_annotation/auditor/BUILD.gn

@@ -95,7 +95,8 @@ test("traffic_annotation_auditor_unittests") {
     "tests/annotations_sample1.xml",
     "tests/annotations_sample2.xml",
     "tests/annotations_sample3.xml",
-    "tests/extractor_outputs/bad_assignment.txt",
+    "tests/extractor_outputs/bad_assignment1.txt",
+    "tests/extractor_outputs/bad_assignment2.txt",
     "tests/extractor_outputs/bad_call.txt",
     "tests/extractor_outputs/bad_syntax_annotation1.txt",
     "tests/extractor_outputs/bad_syntax_annotation2.txt",

tools/traffic_annotation/auditor/instance.cc

@@ -130,13 +130,14 @@ AuditorResult AnnotationInstance::Deserialize(
     const std::vector<std::string>& serialized_lines,
     int start_line,
     int end_line) {
-  if (end_line - start_line < 6) {
+  if (end_line - start_line < 7) {
     return AuditorResult(AuditorResult::Type::ERROR_FATAL,
                          "Not enough lines to deserialize annotation.");
   }
 
   // Extract header lines.
   const std::string& file_path = serialized_lines[start_line++];
+  const std::string& function_context = serialized_lines[start_line++];
   int line_number;
   base::StringToInt(serialized_lines[start_line++], &line_number);
   std::string function_type = serialized_lines[start_line++];
@@ -195,6 +196,7 @@ AuditorResult AnnotationInstance::Deserialize(
   traffic_annotation::NetworkTrafficAnnotation_TrafficSource* src =
       proto.mutable_source();
   src->set_file(file_path);
+  src->set_function(function_context);
   src->set_line(line_number);
   proto.set_unique_id(unique_id);
   second_id_hash_code = TrafficAnnotationAuditor::ComputeHashValue(second_id);
@@ -643,6 +645,7 @@ CallInstance::CallInstance() : line_number(0), is_annotated(false) {}
 CallInstance::CallInstance(const CallInstance& other)
     : file_path(other.file_path),
       line_number(other.line_number),
+      function_context(other.function_context),
       function_name(other.function_name),
       is_annotated(other.is_annotated) {}
 
@@ -650,12 +653,13 @@ AuditorResult CallInstance::Deserialize(
     const std::vector<std::string>& serialized_lines,
     int start_line,
     int end_line) {
-  if (end_line - start_line != 4) {
+  if (end_line - start_line != 5) {
     return AuditorResult(AuditorResult::Type::ERROR_FATAL,
                          "Not enough lines to deserialize call.");
   }
 
   file_path = serialized_lines[start_line++];
+  function_context = serialized_lines[start_line++];
   int line_number_int;
   base::StringToInt(serialized_lines[start_line++], &line_number_int);
   line_number = static_cast<uint32_t>(line_number_int);
@@ -669,17 +673,20 @@ AuditorResult CallInstance::Deserialize(
 AssignmentInstance::AssignmentInstance() : line_number(0) {}
 
 AssignmentInstance::AssignmentInstance(const AssignmentInstance& other)
-    : file_path(other.file_path), line_number(other.line_number) {}
+    : file_path(other.file_path),
+      line_number(other.line_number),
+      function_context(other.function_context) {}
 
 AuditorResult AssignmentInstance::Deserialize(
     const std::vector<std::string>& serialized_lines,
     int start_line,
     int end_line) {
-  if (end_line - start_line != 2) {
+  if (end_line - start_line != 3) {
     return AuditorResult(AuditorResult::Type::ERROR_FATAL,
                          "Not enough lines to deserialize assignment.");
   }
   file_path = serialized_lines[start_line++];
+  function_context = serialized_lines[start_line++];
   int line_number_int;
   base::StringToInt(serialized_lines[start_line++], &line_number_int);
   line_number = static_cast<uint32_t>(line_number_int);

tools/traffic_annotation/auditor/instance.h

@@ -156,6 +156,9 @@ class CallInstance : public InstanceBase {
   std::string file_path;
   uint32_t line_number;
 
+  // Name of the function in which the call happens.
+  std::string function_context;
+
   // Name of the function that may need annotation.
   std::string function_name;
 
@@ -184,6 +187,9 @@ class AssignmentInstance : public InstanceBase {
 
   std::string file_path;
   uint32_t line_number;
+
+  // Name of the function in which assignment happens.
+  std::string function_context;
 };
 
 #endif  // TOOLS_TRAFFIC_ANNOTATION_AUDITOR_INSTANCE_H_

tools/traffic_annotation/auditor/tests/extractor_outputs/bad_assignment.txt

-components/download/internal/proto_conversions.cc
\ No newline at end of file

tools/traffic_annotation/auditor/tests/extractor_outputs/bad_assignment1.txt

+components/download/internal/proto_conversions.cc
+267
\ No newline at end of file

tools/traffic_annotation/auditor/tests/extractor_outputs/bad_assignment2.txt

+components/download/internal/proto_conversions.cc
+download::ProtoConversions::EntryFromProto
\ No newline at end of file

tools/traffic_annotation/auditor/tests/extractor_outputs/bad_call.txt

 headless/public/util/http_url_fetcher.cc
+headless::HttpURLFetcher::Delegate::Delegate
 net::URLRequestContext::CreateRequest
 1
\ No newline at end of file

tools/traffic_annotation/auditor/tests/extractor_outputs/bad_syntax_annotation1.txt

 chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
+OnGetTokenSuccess
 166
 Definition
 supervised_user_refresh_token_fetcher

tools/traffic_annotation/auditor/tests/extractor_outputs/bad_syntax_annotation2.txt

 chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
+OnGetTokenSuccess
 166
 Definition
 supervised_user_refresh_token_fetcher

tools/traffic_annotation/auditor/tests/extractor_outputs/bad_syntax_annotation3.txt

 chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
+OnGetTokenSuccess
 166
 Definition
 supervised_user_refresh_token_fetcher

tools/traffic_annotation/auditor/tests/extractor_outputs/bad_syntax_annotation4.txt

 chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
+OnGetTokenSuccess
 166
 Definition
 supervised_user_refresh_token_fetcher

tools/traffic_annotation/auditor/tests/extractor_outputs/fatal_annotation1.txt

 chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
+OnGetTokenSuccess
 Definition
 supervised_user_refresh_token_fetcher
 

tools/traffic_annotation/auditor/tests/extractor_outputs/fatal_annotation2.txt

 chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
+OnGetTokenSuccess
 122
 definition
 supervised_user_refresh_token_fetcher

tools/traffic_annotation/auditor/tests/extractor_outputs/fatal_annotation3.txt

 chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
+OnGetTokenSuccess
 122
 definition
 supervised_user_refresh_token_fetcher

tools/traffic_annotation/auditor/tests/extractor_outputs/good_assignment.txt

 components/download/internal/background_service/proto_conversions.cc
+download::ProtoConversions::EntryFromProto
 267
\ No newline at end of file

tools/traffic_annotation/auditor/tests/extractor_outputs/good_branched_completing_annotation.txt

 chrome/service/cloud_print/cloud_print_url_fetcher.cc
+cloud_print::CloudPrintURLFetcher::StartRequestHelper
 265
 BranchedCompleting
 cloud_print

tools/traffic_annotation/auditor/tests/extractor_outputs/good_call.txt

 headless/public/util/http_url_fetcher.cc
+headless::HttpURLFetcher::Delegate::Delegate
 100
 net::URLRequestContext::CreateRequest
 1
\ No newline at end of file

tools/traffic_annotation/auditor/tests/extractor_outputs/good_complete_annotation.txt

 chrome/browser/supervised_user/legacy/supervised_user_refresh_token_fetcher.cc
+OnGetTokenSuccess
 166
 Definition
 supervised_user_refresh_token_fetcher

tools/traffic_annotation/auditor/tests/extractor_outputs/good_completing_annotation.txt

 chrome/service/cloud_print/cloud_print_url_fetcher.cc
+cloud_print::CloudPrintURLFetcher::StartRequestHelper
 265
 Completing
 cloud_print

tools/traffic_annotation/auditor/tests/extractor_outputs/good_partial_annotation.txt

 components/browsing_data/core/counters/history_counter.cc
+browsing_data::HistoryCounter::Count
 98
 Partial
 web_history_counter

tools/traffic_annotation/auditor/tests/extractor_outputs/good_test_annotation.txt

 google_apis/drive/request_sender_unittest.cc
+google_apis::(anonymous namespace)::RequestSenderTest::RequestSenderTest
 67
 Definition
 test

tools/traffic_annotation/auditor/tests/extractor_outputs/missing_annotation.txt

 net/url_request/url_request_context.cc
+net::URLRequestContext::CreateRequest
 120
 Definition
 missing

tools/traffic_annotation/auditor/traffic_annotation_auditor_unittest.cc

@@ -334,6 +334,7 @@ TEST_F(TrafficAnnotationAuditorTest, AnnotationDeserialization) {
     EXPECT_EQ(annotation.proto.source().file(),
               "chrome/browser/supervised_user/legacy/"
               "supervised_user_refresh_token_fetcher.cc");
+    EXPECT_EQ(annotation.proto.source().function(), "OnGetTokenSuccess");
     EXPECT_EQ(annotation.proto.source().line(), 166);
     EXPECT_EQ(annotation.proto.semantics().sender(), "Supervised Users");
     EXPECT_EQ(annotation.proto.policy().cookies_allowed(), 1);
@@ -364,6 +365,8 @@ TEST_F(TrafficAnnotationAuditorTest, CallDeserialization) {
 
     EXPECT_EQ(call.file_path, "headless/public/util/http_url_fetcher.cc");
     EXPECT_EQ(call.line_number, 100u);
+    EXPECT_EQ(call.function_context,
+              "headless::HttpURLFetcher::Delegate::Delegate");
     EXPECT_EQ(call.function_name, "net::URLRequestContext::CreateRequest");
     EXPECT_EQ(call.is_annotated, true);
   }
@@ -378,7 +381,8 @@ TEST_F(TrafficAnnotationAuditorTest, AssignmentDeserialization) {
 
   Assignmentample test_cases[] = {
       {"good_assignment.txt", AuditorResult::Type::RESULT_OK},
-      {"bad_assignment.txt", AuditorResult::Type::ERROR_FATAL},
+      {"bad_assignment1.txt", AuditorResult::Type::ERROR_FATAL},
+      {"bad_assignment2.txt", AuditorResult::Type::ERROR_FATAL},
   };
 
   for (const auto& test_case : test_cases) {

tools/traffic_annotation/bin/README.md

@@ -74,5 +74,5 @@ and land the resulting CL.
 The following two lines will be updated by the above script, and the modified
 README should be committed along with the updated .sha1 checksums.
 
-CLANG_REVISION = 'd874c057bc2361da5157553e1e2178f43c3ade1a'
-LASTCHANGE=5a2618740eb68a63b73a40f08a13b1e337cb6399-refs/heads/master@{#674114}
+CLANG_REVISION = '360094'
+LASTCHANGE=ecda211dff8f3722704938f87637e0df657d15f1-refs/heads/master@{#659930}

tools/traffic_annotation/bin/linux64/traffic_annotation_auditor.sha1

-24a6a97630bc133de57c4812af3abca8763b5a7f
\ No newline at end of file
+73004f4964def13577f9d060d77908d51c30d521
\ No newline at end of file

tools/traffic_annotation/bin/linux64/traffic_annotation_extractor.sha1

-8618305abe807cbeefaaa512e47796f89116bf98
\ No newline at end of file
+c4e2a772fbab2f1f7e47cf8f4f23425933f8cd0f
\ No newline at end of file

tools/traffic_annotation/bin/win32/traffic_annotation_auditor.exe.sha1

-fe1e664643c009ca4c16fbe3cabe34a206ca50ee
\ No newline at end of file
+e3c00e2607fbe0d5758e60bc42c6802feacdea08
\ No newline at end of file

tools/traffic_annotation/bin/win32/traffic_annotation_extractor.exe.sha1

-76bd7403ff65eeeb27c7c56dda2688e3d9319053
\ No newline at end of file
+eac43e1fb9f3fb1a300515061ad9daef6d51839d
\ No newline at end of file

tools/traffic_annotation/scripts/extractor.py

@@ -54,6 +54,13 @@ class Annotation:
       file_path: Path to the file that contains this annotation.
     """
     self.file_path = file_path
+    # TODO(crbug/966883): Remove function_name from here and from the clang
+    # tool's output.
+    #
+    # |function_name| is not supported, but keep it in the output for
+    # compatibility with the clang tool. Use an obviously wrong function name
+    # in case this details ends up in auditor output.
+    self.function_name = "XXX_UNIMPLEMENTED_XXX"
     self.line_number = line_number
     self.type_name = type_name
     self.unique_id = unique_id
@@ -81,6 +88,7 @@ class Annotation:
     return "\n".join(map(str, [
         "==== NEW ANNOTATION ====",
         self.file_path,
+        self.function_name,
         self.line_number,
         self.type_name,
         self.unique_id,

tools/traffic_annotation/scripts/test_data/valid_file-stdout.txt

 ==== NEW ANNOTATION ====
 valid_file.cc
+XXX_UNIMPLEMENTED_XXX
 14
 Definition
 id1
@@ -25,6 +26,7 @@ id1
 ==== ANNOTATION ENDS ====
 ==== NEW ANNOTATION ====
 valid_file.cc
+XXX_UNIMPLEMENTED_XXX
 36
 Partial
 id2
@@ -40,6 +42,7 @@ completing_id2
 ==== ANNOTATION ENDS ====
 ==== NEW ANNOTATION ====
 valid_file.cc
+XXX_UNIMPLEMENTED_XXX
 46
 Completing
 id3
@@ -59,6 +62,7 @@ id3
 ==== ANNOTATION ENDS ====
 ==== NEW ANNOTATION ====
 valid_file.cc
+XXX_UNIMPLEMENTED_XXX
 61
 BranchedCompleting
 id4
@@ -73,6 +77,7 @@ branch4
 ==== ANNOTATION ENDS ====
 ==== NEW ANNOTATION ====
 valid_file.cc
+XXX_UNIMPLEMENTED_XXX
 126
 Mutable
 
@@ -81,6 +86,7 @@ Mutable
 ==== ANNOTATION ENDS ====
 ==== NEW ANNOTATION ====
 valid_file.cc
+XXX_UNIMPLEMENTED_XXX
 86
 Definition
 test
@@ -89,6 +95,7 @@ Traffic annotation for unit, browser and other tests
 ==== ANNOTATION ENDS ====
 ==== NEW ANNOTATION ====
 valid_file.cc
+XXX_UNIMPLEMENTED_XXX
 88
 Partial
 test_partial

tools/traffic_annotation/traffic_annotation.proto

@@ -38,6 +38,12 @@ message NetworkTrafficAnnotation {
     // specified.
     string file = 1;
 
+    // Function name where the network request is instantiated.
+    // This is typically filled by the extractor and does not need to be
+    // specified in the source code. For manual whitelisting this needs to be
+    // specified.
+    string function = 2;
+
     // __LINE__ in file, where the AuditPolicy object is instantiated.
     // This is typically filled by the extractor and does not need to be
     // specified in the source code.