Allow assertions to be enabled in Blink Release builds.

Allow assertions to be enabled in Blink Release builds.

The new GYP/GN variable blink_asserts_always_on (mirroring Chromium's
dcheck_always_on) can be set to 1 to enable assertions in Release
builds.

Tests of NDEBUG related to assertions have been changed to test
ENABLE(ASSERT). Printing/debugging-only code is still guarded under
NDEBUG. Debug-only fields and counters remain debug-only.

Once this CL lands, Blink's assertions will be enabled on the GPU
bots, which only test Release builds (with dcheck_always_on).

Tested:
 - WebGL conformance tests, Release build, asserts on
 - Release build, asserts on, inserted ASSERT(FALSE) guarded under
   ENABLE(ASSERT), verified it was triggered
 - Release build, asserts on, inserted ASSERT(FALSE) guarded under
   ENABLE(SECURITY_ASSERT), verified it was triggered
 - WebGL conformance/context tests, Release build, asserts on, with
   https://codereview.chromium.org/251373005, verified assertion
   failure was triggered
 - WebGL conformance/context tests, Debug build, default assertion
   behavior, with https://codereview.chromium.org/251373005, verified
   assertion failure was triggered
 - wtf_unittests, Release build, asserts on

BUG=393838
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178404

[Ken Russell (switch to Gerrit), 2014-07-15 23:25:35]

eseidel, abarth, or jochen: please review everything.
ager or haraken: please review Source/platform/heap.
pfeldman: please review Source/core/inspector.
jsbell: please review IndexedDB and Database changes.
eae or leviw: please review LayoutUnit.h.
cevans: please review PartitionAlloc and test changes.

Please suggest other needed reviewers. Thanks.

https://codereview.chromium.org/397733004/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (left):

https://codereview.chromium.org/397733004/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:281: bool didDispatchViewportPropertiesChanged()
const { return m_didDispatchViewportPropertiesChanged; }
This method was unreferenced.

https://codereview.chromium.org/397733004/diff/20001/Source/core/editing/Edit...
File Source/core/editing/EditCommand.h (left):

https://codereview.chromium.org/397733004/diff/20001/Source/core/editing/Edit...
Source/core/editing/EditCommand.h:34: #include "wtf/HashSet.h"
HashSet was unused in this header.

https://codereview.chromium.org/397733004/diff/20001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseSync.h (left):

https://codereview.chromium.org/397733004/diff/20001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseSync.h:43: #include
"platform/weborigin/SecurityOrigin.h"
SecurityOrigin was unreferenced in this header.

https://codereview.chromium.org/397733004/diff/20001/Source/platform/LayoutUn...
File Source/platform/LayoutUnit.h (right):

https://codereview.chromium.org/397733004/diff/20001/Source/platform/LayoutUn...
Source/platform/LayoutUnit.h:43: #if !ERROR_DISABLED
This seemed the closest #if guard to that covering WTFReportError calls in
Assertions.h.

https://codereview.chromium.org/397733004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/397733004/diff/20001/Source/platform/graphics...
Source/platform/graphics/Image.h:37: #include "wtf/Assertions.h"
I'm pretty sure that based on compile failures seen during development of this
patch, this header was an example of one not properly including Assertions.h,
and previously silently failing.

https://codereview.chromium.org/397733004/diff/20001/Source/wtf/PartitionAllo...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/397733004/diff/20001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAllocTest.cpp:567: // byte pattern.
Updated this comment. Ran wtf_unittests in Release mode on Linux with assertions
enabled. Test passes.

https://codereview.chromium.org/397733004/diff/20001/Source/wtf/ThreadRestric...
File Source/wtf/ThreadRestrictionVerifier.h (left):

https://codereview.chromium.org/397733004/diff/20001/Source/wtf/ThreadRestric...
Source/wtf/ThreadRestrictionVerifier.h:56: #if ASSERT_ENABLED
This entire file was essentially guarded under ASSERT_ENABLED so this #if was
redundant.

https://codereview.chromium.org/397733004/diff/20001/Source/wtf/WTF.cpp
File Source/wtf/WTF.cpp (left):

https://codereview.chromium.org/397733004/diff/20001/Source/wtf/WTF.cpp#oldco...
Source/wtf/WTF.cpp:38: #include "wtf/MainThread.h"
This #include was unnecessary.

[eseidel, 2014-07-15 23:27:50]

Is there any way we can do this in smaller pieces? :(

[Ken Russell (switch to Gerrit), 2014-07-15 23:38:22]

On 2014/07/15 23:27:50, eseidel wrote:
> Is there any way we can do this in smaller pieces? :(

I don't think it's a good idea to do so. The possibility of leaving different
pieces of code in inconsistent states is high.

The changes are mostly mechanical. I just re-reviewed it and it took about an
hour to go through everything.

[jsbell, 2014-07-16 00:03:16]

modules/ lgtm

https://codereview.chromium.org/397733004/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/397733004/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:325: #if ENABLE(ASSERT)
Looks like we can remove this #if guard now?

[Ken Russell (switch to Gerrit), 2014-07-16 00:05:46]

https://codereview.chromium.org/397733004/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/397733004/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:325: #if ENABLE(ASSERT)
On 2014/07/16 00:03:16, jsbell wrote:
> Looks like we can remove this #if guard now?

Sure. Will remove in a subsequent version of this CL.

[Mads Ager (chromium), 2014-07-16 05:28:09]

platform/heap LGTM

[dshwang, 2014-07-16 12:31:20]

That's cool. non-owner lgtm
I've been looking forward of it as I submitted similar CL 7 months ago;
https://codereview.chromium.org/102123002/

[eae, 2014-07-16 12:48:38]

LGTM for LayoutUnit.h

[Ken Russell (switch to Gerrit), 2014-07-16 21:19:07]

After discussion with abarth and eseidel I'm going to put up a separate CL
mechanically replacing SECURITY_ASSERT_ENABLED with ENABLE(SECURITY_ASSERT) and
ASSERT_ENABLED with ENABLE(ASSERT). That should make this one significantly
smaller. Please hold off on further reviews for the moment. Thanks everybody.

[Ken Russell (switch to Gerrit), 2014-07-16 22:06:03]

FYI, the mechanical search-and-replace is being reviewed under
https://codereview.chromium.org/394353002 .

[Ken Russell (switch to Gerrit), 2014-07-17 20:52:43]

This CL has been rebased on top of the mechanical renaming of ASSERT_ENABLED to
ENABLE(ASSERT). Now only 148 files are touched, compared to 229 in the previous
patch set.

Note that we could consider always specifying ENABLE_ASSERT in features.gypi and
config.gni, to avoid bugs where wtf/Assertions.h might accidentally not be
included. I elected not to make this change to minimize the impact on the
default configuration.

Please review. Thanks.

[abarth-chromium, 2014-07-17 21:02:41]

LGTM

https://codereview.chromium.org/397733004/diff/40001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.h (right):

https://codereview.chromium.org/397733004/diff/40001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.h:74: #endif
This change looks incorrect.  We should be able to build debug without asserts.

[Ken Russell (switch to Gerrit), 2014-07-17 21:25:24]

At this point I'm confident in the correctness of these changes and they're
strongly desired before further changes occur in Blink's graphics stack, so am
going to CQ this now.

https://codereview.chromium.org/397733004/diff/40001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.h (right):

https://codereview.chromium.org/397733004/diff/40001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.h:74: #endif
On 2014/07/17 21:02:41, abarth wrote:
> This change looks incorrect.  We should be able to build debug without
asserts.

It's correct. The base class defines the virtual destructor. This one only
overrides it in order to do assertions against data members conditionally
included with ENABLE(ASSERT). I verified with a local debug build forcing
ENABLE_ASSERT=0. There's one other build problem with that configuration and in
general I realized we can't set ENABLE_ASSERT from GYP because GYP files can't
test the flavor being built (i.e., Debug vs. Release).

[Ken Russell (switch to Gerrit), 2014-07-17 21:27:14]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2014-07-17 21:27:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kbr@chromium.org/397733004/60001

[commit-bot: I haz the power, 2014-07-17 23:17:31]

Change committed as 178404