Build settings, from https://build.chromium.org/p/chromium.win/builders/Win%20Builder%20%28dbg%29, are:

is_component_build = true
is_debug = true
target_cpu = "x86"
use_goma = true

The test failure is on the browser_tests step, running this command:

.\browser_tests.exe --brave-new-test-launcher --test-launcher-bot-mode --test-launcher-summary-output=e:\b\s\w\ioay1szx\output.json

The bug happens because JSInliningHeuristic::Reduce inserts two nodes into the same ZoneSet (std::set) that have a frequency of NaN. Inserting one is legal because JSInliningHeuristic::CandidateCompare::operator() defines that comparison, but if two nodes with a frequency of NaN get inserted then there is no way to order them.

The VC++ STL detects this and says: _DEBUG_ERROR2("invalid comparator", _File, _Line). I assume that this is a new check in VS 2017 and is therefore a feature, not a bug.

I'll investigate a bit more and then assign this to the v8 team to figure out why the NaN frequencies are being inserted. The fix may be as simple as "don't insert NaN frequencies". Adding a DCHECK (or the v8 equivalent) to the comparator will detect this issue in VS 2015.

bool JSInliningHeuristic::CandidateCompare::operator()(
    const Candidate& left, const Candidate& right) const {
  if (right.frequency.IsUnknown()) {
    // If left and right are both unknown then the ordering is indeterminate,
    // which breaks strict weak ordering requirements.
    DCHECK(left.frequency.IsKnown());
    return true;

In crrev.com/2856103002 a NaN sentinel was introduced. However using NaNs as the key to a set is perilous. JSInliningHeuristic::CandidateCompare::operator() has support for deterministically comparing a NaN to another number, but it does not support comparing two NaNs, which leads to internal check failures in the VC++ implementation of std::set. For some reason these failures only show up with VC++ 2017, but the undefined behavior (inserting two NaNs) occurs with VC++ 2015 as well, and presumably with all compilers.

The fix which seems most consistent with the intent of the code is to use node->id() as a tie breaker. See crrev.com/2894433004.

BTW, the best way to reproduce the problem is to run the test like this:

devenv.exe /debugexe browser_tests.exe --single_process --single-process --gtest_filter=NetInternalsTest.netInternalsEventsViewFilter

Running everything in one process ensures that the invalid comparator check, which is in xutility in _Debug_lt_pred() as:

	{	// test if _Pred(_Left, _Right) and _Pred is strict weak ordering
	return (_Pred(_Left, _Right)
		? (_Pred(_Right, _Left)
			? (_DEBUG_ERROR2("invalid comparator", _File, _Line), true)
			: true)
		: false);
	}

will be caught in the debugger instead of quietly terminating the child process.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/58ba4cefe8aeb05479dfdd0ffb65e2774a7a07b8

commit 58ba4cefe8aeb05479dfdd0ffb65e2774a7a07b8
Author: brucedawson <brucedawson@chromium.org>
Date: Fri May 19 06:10:22 2017

Enforce strict weak ordering on NaN frequencies

In crrev.com/2856103002 sentinel frequency values were introduced, using
NaN as the sentinel. However the comparison function was not *fully*
updated to support these - comparing two NaNs would give ambiguous
results. This caused test failures when building with VS 2017, probably
because of subtle changes in the arrangement of nodes in the tree.

This change uses the the node ID to break ties. An alternative would be
to use a non-NaN sentinel value.

R=bmeurer@chromium.org
BUG= chromium:722480 

Review-Url: https://codereview.chromium.org/2894433004
Cr-Commit-Position: refs/heads/master@{#45415}

[modify] https://crrev.com/58ba4cefe8aeb05479dfdd0ffb65e2774a7a07b8/src/compiler/js-inlining-heuristic.cc