Tagging as a JavaScript>API issue, since it's not clear what the "correct" way is to get the desired behaviour, without relying on fragile and possibly-deprecated command-line flags.

There doesn't seem to be a return value from the SetFlagsFromString() API, so we can't easily spot when this gets broken, unfortunately.

hpayer: Is there a cleaner way to configure V8 to omit hard CHECKs in generated code?  Leaving DCHECK/validation enabled seems reasonable, although as noted performance may be an issue.

Giving this one to Danno, the master of CSA and Torque :)

Any v8 embedder that uses custom flags that are not "mainstream" (noverify_csa is one of them) needs to maintain their flag usage based on what's available. In this case, noverify_csa is only present when the V8 GN build is configured with "v8_enable_verify_csa = true", which is probably not the case here. So the solution is probably to simply to remove the "--noverify_csa" from render_process_impl.cc in this case. I'm reassigning to the original author of the relevant snippet of render_process_impl.cc (Siggi), but I'd be happy to review and approve the CL for that change from anybody.

wez: I'm not sure what you mean by "configuring V8 to omit CHECKs" in comment 2, since we always want checks. For the most part, disabling DCHECKs in V8 is "at your own risk", since the default DCHECK configuration is designed to actually catch important errors that the V8 team cares about. If they get turned off, even if just for performance reasons, we're not getting the rigor in testing that we'd get in a perfect world. So selective deactivation seems the way to go, but that means the embedder (in this case Chrome) needs to keep an eye on and understand which flags are available and what they do.

Bouncing to Wez, as I'm not involved with Albatross anymore.

Thanks siggi!

Re #3: danno@, re these flags vs what V8 team care about: the Albatross build is pushed to a %age of Canary users, and then DCHECKs are actually made fatal for only a %age of sessions. As per the comment, Siggi disabled those checks partly because of performance impact, and partly because they would be always-fatal for users with the Albatross build, even in sessions where DCHECKs are otherwise non-fatal.

From Albatross PoV we would ideally like a single flag that makes V8 behave identically to the way it would in a non-DCHECK build, that we can set for session with non-fatal DCHECKs.  Is that something that V8 could provide?  Or should we assume that the overhead of these verifications is not significant?

(Assigning back to danno@ for input, but in the meantime I'll remove this flag)

Thanks for removing the flag.

I am still not quite sure what you're trying to do here. If you mean you'd like a flag that makes the debug build code paths as close to release as close to the same as possible (e.g. no fundamentally addition code paths), that might be a possibility, but it hard to measure and probably hard to make sure that it remains correct. However, it's a bit tricky, since it has never been a design goal to keep the overhead of debug builds down, so it's quite likely that even with non of the "extra" paths, performance will still be miserable... fixing that is probably outside the scope of what the V8 team is willing to tackle.

What I am not sure of is what you mean by "non-fatal DCHECKs". From our perspective, pretty much every DCHECK is fatal if it fails, and the only reason that most are not full checks is the performance cost. Perhaps I'm just not clear about what Albatross is trying to accomplish... does it just log these and continue?

Re #6: Albatross is a build in which DCHECK's have a configurable LogSeverity. By default they are LOG_ERROR, but if you have the flag enabled then they become LOG_FATAL. Sessions running Albatross with DCHECK=ERROR therefore get most of the performance down-sides (since the DCHECKs are still actually run) but without actually crashing.

We do that rather than simply enabling DCHECKs all the time in these builds because it means that if the user hits a DCHECK that e.g. is 100% repro given their profile contents, or the site they're using, then when the browser re-starts it _probably_ won't have DCHECKs FATAL any more, so will probably not crash - we minimize the adverse impact to the user so that they don't just give up on Canary entirely. ;)

We already set a V8 DCHECK handler, which implements the LogSeverity configurability. From siggi@'s comments I gather that V8 Debug code generation includes always-fatal checks, which is what we would like to be able to switch off if DCHECK=ERROR - any performance improvement of that would be nice-to-have, but the main thing is to make the checks non-crashy for those "control" users.

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

commit 53a3dde9e101c2b6bcb6003e1f94584afa15eb4a
Author: Wez <wez@chromium.org>
Date: Fri Jul 06 17:52:33 2018

[Albatross] Remove --noverify_csa from debug-disabling flags to V8.

This flag is not supported in the Albatross builds, so passing it causes
the other flags to actually be ignored, plus adds logspam.

TBR: jochen
Bug: 849771
Change-Id: I8a71e219dc7905c1c142a05e00b6b1c66c8cb8cf
Reviewed-on: https://chromium-review.googlesource.com/1127458
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Daniel Clifford <danno@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573010}
[modify] https://crrev.com/53a3dde9e101c2b6bcb6003e1f94584afa15eb4a/content/renderer/render_process_impl.cc

wez: re-spinning this plate. So, just to understand, there are two issues here, and I'd appreciate your help sorting them in my mental model:

1) You would like V8 builds with DCHECKs to be faster (e.g. the configuration with DCHECK=ERROR), anything we can do to improve performance there would be appreciate from your side, because for Albatross we actually run this configuration in the wild. I understand this part.

2) You would like the 'make the checks non-crashy for those "control" users'. I still am not quite sure what this entails. I assume that what you're getting at is that there are currently situations where DCHECKs in V8 that we trigger in the code after which we crash, even if the same "pure" release build would not have crashed at that point (i.e. the underlying V8 behavior is actually different between the builds up-and-beyond just asserting some invariant).

Is this correct?

For 1), we do keep an eye on debug performance, but our range of acceptable is (rough-guess) an order of magnitude slower. There will likely be strong push-back in the V8 to try to pull in performance much closer to release, because it would be a considerable new requirement to track, and we've always favored "more DCHECKs are better" in the spirit of defensive programming.

For 2) I am pretty sure that non-recoverable DCHECKs with side effects are somewhat frequently present in V8. I know that in DEBUG V8 builds it's not uncommon to compute something non-trivial extra to just check it. I think it would be quite tricky to find all of these sites to ensure that the computation is safe regardless of failure (and too expensive to compute in the release path). I don't even know how we'd be able to maintain/test this at scale, i.e. how we'd maintain the invariant that all DCHECK sites recover as well as release builds. Any ideas how to do this?

That being said, if there are specific cases you know of where we crash after a logged DCHECK in V8 where we would just keep running in release, please let me know, and we can try to do a spot-fix.

Thanks Danno!

Re 1: I believe the property we were looking for was that V8 omit some of the more expensive sanity-checks in these builds.  We were passing the nocsa_verify flag to V8 to disable that Debug-mode check, because it was apparently very slow.  However, we've been running with it all enabled, sanity-checks and all, for a year, without issues, AFAIK, so perhaps that isn't such a priority after all. 

Re 2: Requirement is just that _all_ crashes due to failed assertions should call the caller-supplied assertion handler function, which we hook in Chromium to have its behaviour depend on whether DCHECKs are FATAL or not.  I believe some bits of V8 (possibly generated code?) had direct crashes for some assertions.

It's understood that DCHECKs express invariants, and if an invariant doesn't hold then it's quite possible that the subsequent code is horribly broken; what we're really look for is that if DCHECKs are enabled but non-FATAL then the level of brokenness is not noticably worse than in a non-DCHECK build.

So I think there is nothing immediately required here.