Stack trace from https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin7_Tests__dbg__1_%2F55329%2F%2B%2Frecipes%2Fsteps%2Fnet_unittests%2F0%2Flogs%2FDecodeBufferDeathTest.NonNullBufferRequired%2F0:

et\http2\decoder\decode_buffer_test.cc(331): error: Death test: { DecodeBuffer b(nullptr, 3); }
    Result: died but not with expected error.
  Expected: nullptr
Actual msg:
[  DEATH   ] [5552:4336:1205/172411.877:2924004:WARNING:test_suite.cc(236)] Test launcher output path C:\Users\CHROME~2\AppData\Local\Temp72_27554	est_results.xml exists. Not adding test launcher result printer.
[  DEATH   ] Backtrace:
[  DEATH   ] 	std::char_traits<char>::length [0x100AA6B7+7]
[  DEATH   ] 	std::operator<<<std::char_traits<char> > [0x100A8B16+22]
[  DEATH   ] 	logging::MakeCheckOpValueString<char const *> [0x10487152+18]
[  DEATH   ] 	??$MakeCheckOpString@PBD$$T@logging@@YAPAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@ABQBDAB$$TPBD@Z [0x1063E789+73]
[  DEATH   ] 	??$CheckNEImpl@PBD$$T@logging@@YAPAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@ABQBDAB$$TPBD@Z [0x1063E3F6+38]
[  DEATH   ] 	net::DecodeBuffer::DecodeBuffer [0x1063EB0C+92]
[  DEATH   ] 	net::test::DecodeBufferDeathTest_NonNullBufferRequired_Test::TestBody [0x0112AA64+260]
[  DEATH   ] 	testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void> [0x0229A834+52]
[  DEATH   ] 	testing::Test::Run [0x022B5B07+135]
[  DEATH   ] 	testing::TestInfo::Run [0x022B5D8D+173]
[  DEATH   ] 	testing::TestCase::Run [0x022B5C2F+191]
[  DEATH   ] 	testing::internal::UnitTestImpl::RunAllTests [0x022B6215+661]
[  DEATH   ] 	testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> [0x0229A924+52]
[  DEATH   ] 	testing::UnitTest::Run [0x022B5F0F+207]


Minimized test case:

DecodeBuffer::DecodeBuffer(const char* buffer, size_t len) {
  DCHECK_NE(buffer, nullptr);
}

TEST(DecodeBufferDeathTest, NonNullBufferRequired) {
  EXPECT_DEBUG_DEATH({ DecodeBuffer b(nullptr, 3); }, "nullptr");
}

However, DCHECK does not crash normally, printing a message containing nullptr.  Instead, it seems to invoke logging::MakeCheckOpValueString<char const *> with nullptr (instead of logging::MakeCheckOpValueString<nullptr_t>), which of course crashes when trying to dereference the nullptr.  Since the string "nullptr" is not printed, test DecodeBufferDeathTest.NonNullBufferRequired fails.

Assigning to author of https://crrev.com/2515283002, who introduced a number of MakeCheckOpValueString overloads recently.  Thank you.

I don't think this is caused by the overload I introduced, as the previously existing one is being used in the backtrace. The problem is that this operator<< overload is being called:

  template< class Traits >
  basic_ostream<char,Traits>& operator<<( basic_ostream<char,Traits>& os,  
                                          const char* s );

and its behavior is undefined when |s| is null.

thakis/jbroman: I could see if some sort of template black magic only for chars would work here, but something like "if (std::is_pointer<T>::value && !v) (*os) << "null pointer" is a lot easier at the expense of being a runtime check. Do you guys have any preference?

Dunno, a nullptr_t overload is all that's needed maybe?

Why wasn't this needed previously? But crashing on nullptr doesn't seem unreasonable to me either. Just say DCHECK(foo != nullptr)

We already have a nullptr_t overload:

  // We need an explicit overload for std::nullptr_t.
  BASE_EXPORT void MakeCheckOpValueString(std::ostream* os, std::nullptr_t p);

And I think this problem has always been present. The crash only happens with signed/unsigned/nothing character pointers that end up using these overloads:
http://en.cppreference.com/w/cpp/io/basic_ostream/operator_ltlt2

Yeah, the right side should deduce to nullptr_t, but the type of the left side is clearly "const char*", and it's the conversion of that to string that is undefined (as one would expect).

Just "DCHECK(buffer)" will suffice.

OK, so is it fine not to add any other checks or overloads to the logging code?

Yes.

Personally that SGTM, I'm not convinced it's worth adding an overload for char* (with various cv qualifiers) just so that it deals with nullptr.

Don't know how base/ owners (thakis@?) feel.

Alright, reassigning this to bnc@ then.

Thank you y'all very much for looking into this, I appreciate your efforts.

I have already landed the workaround at https://crrev.com/2553223002, the one proposed at comment #4.  Therefore I am closing this as Fixed.