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

Issue 761212 link

Starred by 1 user
Status: Untriaged
Owner: ----
Cc:
dbeam@chromium.org
dpa...@chromium.org
█ dschuyler@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 3
Type: Bug



Sign in to add a comment

[MD Settings] Chrome shouldn't CHECK fail when incorrect number of arguments are passed.

Project Member Reported by calamity@chromium.org, Sep 1 2017 
1. Go to chrome://settings
2. Open console
3. Type chrome.send('getCaCertificateTrust')

Chrome CHECK fails and dies.

This is too heavy a hammer for protecting against incorrect arguments. It exposes the possibility of future bugs, that would be normally be limited to a broken UI control, turning into CHECK fail crashers.

We should change these to fail gracefully rather than let them take down the whole system. I mean, I did this for fun on my stable Chromebook. Why is an action that the user is completely able to perform resulting in the biggest banhammer in Chrome?

Comment 1 by dschuyler@chromium.org, Sep 1 2017

Cc: dbeam@chromium.org
I felt that way too at one point. dbeam@ convinced me otherwise :)

I'm up for discussing it, but it's intentional (WAI). Changing it would be policy/paradigm change.

What are the failure rates (are they non-zero) for common users?
We also have chrome://crash/, which is arguably easier for users to get to. Is this somehow worse than chrome://crash/?

Comment 2 by calamity@chromium.org, Sep 4 2017 

I guess for me, the crash urls are intentional developer debugging crash points (also, there's a separate browser crash url, chrome://crash crashes a renderer). What's the intention with putting a checkfail for a message handler? I can only think that it would help us catch incorrect WebUI message calls more quickly?

If we decide that the risk of pushing a hidden Chrome crasher widely is worth the benefit of being able to catch broken WebUIs through that mechanism, then so it is. But surely this is still a suboptimal way to do that. A separate reporting mechanism via UMA could be used?

Basically, I'm worried about increasing the surface for 'needless' browser crashes to happen. Also, why CHECK and not at least DCHECK?

Comment 3 by dbeam@chromium.org, Sep 15 2017 

calamity@: never type about:inducebrowsercrashforrealz then, either ;)

Chrome has this thing about being willing to crash to detect issues earlier.  It's just a choice.

Some objective things:

1) Few webui devs used debug builds in my experience (because they're just horribly horribly slow and webui often only involves html/css/js changes).

2) Removing all those CHECK()s might reduce the enduser binary size (if there's a BUNCH of them).

My opinion:

I don't really feel strongly about whether CHECK() (vs DCHECK) should be used for param issues.  This might:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#check_dcheck_and-notreached
►

Sign in to add a comment