[coverage] Correctly report coverage for inline scripts

sigurdschneider · Commit Bot · commit 0d7889d0b149 · 2019-10-16T08:23:55.000Z
This fixes a bug where coverage for the inline script <script>function foo() {}<script> started to get deterministically reported as covered after crrev.com/c/1771776, while before it, we most of the time reported it as uncovered (depending on heap order of SFIs). The correct result is to report `foo` as uncovered as it is never called. The problem arose from the fact that v8:9212 needed to handle extra-wrappers around scripts correctly. Those wrappers have the same source range as the wrapped script and a call count of zero even if the wrapped script is executed. To filter them out, we previously determined nesting for identical source ranges by ascending call count. However, in the script case above, the script has call count one, while `foo` (which has the same source range) has call count zero. In this case, nesting is decreasing order of call counts. This CL is a minimal change that sorts SFIs which are top-level to the front, only then considers call counts in descending order. This preserves the behavior that node's extra wrappers are sorted to the front (and then filtered out by existing logic), but also ensures that for the example above, we report the script's coverage before the coverage for `foo`. Bug: v8:9857, v9:9212 Change-Id: Id224b0d8f12028b1f586ee5039e126bb5b8d8d36 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1863197 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Commit-Queue: Sigurd Schneider <sigurds@chromium.org> Cr-Commit-Position: refs/heads/master@{#64307}
diff --git a/src/debug/debug-coverage.cc b/src/debug/debug-coverage.cc
@@ -577,11 +577,15 @@ struct SharedFunctionInfoAndCount {
   // Sort by:
   // - start, ascending.
   // - end, descending.
-  // - count, ascending.
+  // - info.is_toplevel() first
+  // - count, descending.
   bool operator<(const SharedFunctionInfoAndCount& that) const {
     if (this->start != that.start) return this->start < that.start;
     if (this->end != that.end) return this->end > that.end;
-    return this->count < that.count;
+    if (this->info.is_toplevel() != that.info.is_toplevel()) {
+      return this->info.is_toplevel();
+    }
+    return this->count > that.count;
   }
 
   SharedFunctionInfo info;
@@ -653,12 +657,30 @@ std::unique_ptr<Coverage> Coverage::Collect(
 
       // Find the correct outer function based on start position.
       //
-      // This is not robust when considering two functions with identical source
-      // ranges. In this case, it is unclear which function is the inner / outer
-      // function. Above, we ensure that such functions are sorted in ascending
-      // `count` order, so at least our `parent_is_covered` optimization below
-      // should be fine.
-      // TODO(jgruber): Consider removing the optimization.
+      // This is, in general, not robust when considering two functions with
+      // identical source ranges; then the notion of inner and outer is unclear.
+      // Identical source ranges arise when the source range of top-most entity
+      // (e.g. function) in the script is identical to the whole script, e.g.
+      // <script>function foo() {}<script>. The script has its own shared
+      // function info, which has the same source range as the SFI for `foo`.
+      // Node.js creates an additional wrapper for scripts (again with identical
+      // source range) and those wrappers will have a call count of zero even if
+      // the wrapped script was executed (see v8:9212). We mitigate this issue
+      // by sorting top-level SFIs first among SFIs with the same source range:
+      // This ensures top-level SFIs are processed first. If a top-level SFI has
+      // a non-zero call count, it gets recorded due to `function_is_relevant`
+      // below (e.g. script wrappers), while top-level SFIs with zero call count
+      // do not get reported (this ensures node's extra wrappers do not get
+      // reported). If two SFIs with identical source ranges get reported, we
+      // report them in decreasing order of call count, as in all known cases
+      // this corresponds to the nesting order. In the case of the script tag
+      // example above, we report the zero call count of `foo` last. As it turns
+      // out, embedders started to rely on functions being reported in nesting
+      // order.
+      // TODO(jgruber):  Investigate whether it is possible to remove node's
+      // extra  top-level wrapper script, or change its source range, or ensure
+      // that it follows the invariant that nesting order is descending count
+      // order for SFIs with identical source ranges.
       while (!nesting.empty() && functions->at(nesting.back()).end <= start) {
         nesting.pop_back();
       }
diff --git a/test/mjsunit/code-coverage-block.js b/test/mjsunit/code-coverage-block.js
@@ -1068,4 +1068,19 @@ f(43);                                    // 0450
  {"start":204,"end":226,"count":1}]
 );
 
+TestCoverage(
+"https://crbug.com/v8/9857",
+`function foo() {}`,
+[{"start":0,"end":17,"count":1},
+ {"start":0,"end":17,"count":0}]
+);
+
+TestCoverage(
+"https://crbug.com/v8/9857",
+`function foo() {function bar() {}}; foo()`,
+[{"start":0,"end":41,"count":1},
+ {"start":0,"end":34,"count":1},
+ {"start":16,"end":33,"count":0}]
+);
+
 %DebugToggleBlockCoverage(false);
