New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 603166 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

DLOG(FATAL) should crash in release-with-asserts builds as well

Project Member Reported by jam@chromium.org, Apr 13 2016

Issue description

See https://codereview.chromium.org/1864583006/ which went through CQ, but then broke the debug bots.

The problem was that DLOG(FATAL) was being hit in one test. It was compiled out in release-with-asserts builds.

However, just like DCHECK is hit in release-with-asserts, so should DLOG(FATAL).
 

Comment 1 by jam@chromium.org, Apr 13 2016

Cc: brettw@chromium.org thakis@chromium.org
Owner: ----
Status: Available (was: Started)
hmm this turns out to be tricky.

The problem is that if I just enable DLOG in release-with-asserts, then there are a bunch of compile errors because variables/functions which are used in DLOGs are behind "ifndef NDEBUG". I could change them to also have || DCHECK_ALWAYS_ON. However some of this code is in outside repos that don't understand DCHECK_ALWAYS_ON, i.e. leveldb, and they're getting the DLOG indirectly because logging.h redefines assert as DLOG_ASSERT

Comment 2 by jam@chromium.org, Apr 13 2016

Owner: jam@chromium.org
Status: Started (was: Available)
ok i decided i'll fix the code so that it compiles.

https://github.com/google/leveldb/pull/367 is for leveldb, and
https://codereview.chromium.org/1885933002 is for the chromium repo which depends on above.

Comment 3 by jam@chromium.org, Apr 13 2016

Cc: cmumford@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b5bcbc691900b666542cce8e0d04f0472bcd981a

commit b5bcbc691900b666542cce8e0d04f0472bcd981a
Author: cmumford <cmumford@chromium.org>
Date: Fri Apr 15 21:11:37 2016

Roll src/third_party/leveldatabase 7306ef85..a7bff697b (4 commits).

* Deleted redundant null ptr check prior to delete.
* Fixes a bug encountered when reading records from leveldb files
  that have been split, as in a [] input task split.
* Change std::uint64_t to uint64_t (#354).
* Fix LevelDB build when asserts are enabled in release builds. (#367).

BUG= 603166 

Review URL: https://codereview.chromium.org/1878943014

Cr-Commit-Position: refs/heads/master@{#387695}

[modify] https://crrev.com/b5bcbc691900b666542cce8e0d04f0472bcd981a/DEPS

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/121900aaaac3ee9407ad40dd05e54b6d50b859c9

commit 121900aaaac3ee9407ad40dd05e54b6d50b859c9
Author: jam <jam@chromium.org>
Date: Tue Apr 19 00:07:34 2016

Ensure that DLOG(FATAL) also asserts in release-with-asserts builds.

Otherwise patches can land in the CQ which trigger DLOG(FATAL) or assert but then break the main waterfall.

BUG= 603166 

Review URL: https://codereview.chromium.org/1885933002

Cr-Commit-Position: refs/heads/master@{#388086}

[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/base/logging.h
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/base/logging_unittest.cc
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/base/message_loop/incoming_task_queue.cc
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/base/metrics/persistent_histogram_allocator.cc
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/base/threading/platform_thread_win.cc
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/chrome/browser/lifetime/keep_alive_registry.cc
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/chrome/browser/lifetime/keep_alive_registry.h
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/chrome/browser/lifetime/keep_alive_types.cc
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/chrome/browser/lifetime/keep_alive_types.h
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/media/base/key_systems_unittest.cc
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/net/spdy/hpack/hpack_huffman_decoder.cc
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/ppapi/utility/completion_callback_factory_thread_traits.h
[modify] https://crrev.com/121900aaaac3ee9407ad40dd05e54b6d50b859c9/third_party/zlib/google/zip_reader.cc

Comment 6 by jam@chromium.org, Apr 19 2016

Status: Fixed (was: Started)

Sign in to add a comment