[coverage] Correctly report coverage for inline scripts

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/+/1863197Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64307}

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();
       }

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);