Add a DevTools warning for a missing subjectAltName

Add a DevTools warning for a missing subjectAltName

BUG=703616
Review-Url: https://codereview.chromium.org/2761333002
Cr-Commit-Position: refs/heads/master@{#458631}
Committed: https://chromium.googlesource.com/chromium/src/+/74a7a294c2802cf5b65bf0c0d4f44ba093aae216

[Ryan Sleevi, 2017-03-21 15:59:41]

rsleevi@chromium.org changed reviewers:
+ elawrence@chromium.org, lgarron@chromium.org

[Ryan Sleevi, 2017-03-21 15:59:42]

Lucas, Eric: Sending this out before the tests to get feedback on this approach
:)

[elawrence, 2017-03-21 16:22:54]

modulo trivial nits, this is exactly what I'd've done and it LGTM.

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
File components/security_state/core/security_state.cc (right):

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.cc:228:
visible_security_state.certificate.GetSubjectAltName(&dns_names, &ip_addrs);
Would it make sense to just have a HasSubjectAltName() property on the
certificate itself?

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
File components/security_state/core/security_state.h (right):

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:147: // server's certificate.
Maybe "True if the server's certificate does not contain a
subjectAlternativeName extension" or "True if the server's certificate lacks a
subjectAlternativeName extension"?

Otherwise, the article "a" and "is missing" are slightly ambiguous.

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:148: bool san_is_missing;
similarly, maybe "cert_lacks_SAN"?

[Ryan Sleevi, 2017-03-21 16:41:41]

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
File components/security_state/core/security_state.cc (right):

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.cc:228:
visible_security_state.certificate.GetSubjectAltName(&dns_names, &ip_addrs);
On 2017/03/21 16:22:54, elawrence wrote:
> Would it make sense to just have a HasSubjectAltName() property on the
> certificate itself?

No. Generally, I explicitly try to avoid adding those APIs, because we saw far
too rampant usage, or too much coercion to X509Certificate. Our goal is to make
X509Certificate as light as possible and basically just be a cache of some
UI-accessed fields, and everything else to be done by consumers parsing/using
accessors.

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
File components/security_state/core/security_state.h (right):

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:148: bool san_is_missing;
On 2017/03/21 16:22:54, elawrence wrote:
> similarly, maybe "cert_lacks_SAN"?

Was trying to match the naming style for sha1_in_chain

I take it the concern is that "chain" links it to cert, but san_is_missing
doesn't clearly indicate a cert-related issue?

cert_missing_subject_alt_name ?

[elawrence, 2017-03-21 16:49:05]

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
File components/security_state/core/security_state.cc (right):

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.cc:225: if
(visible_security_state.certificate) {
Could we change this to:

if (visible_security_state.certificate &&(visible_security_state.cert_status &
net::COMMON_NAME_INVALID)) {

... to improve performance?

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.cc:228:
visible_security_state.certificate.GetSubjectAltName(&dns_names, &ip_addrs);
On 2017/03/21 16:41:41, Ryan Sleevi (conf till 3-24) wrote:
> On 2017/03/21 16:22:54, elawrence wrote:
> > Would it make sense to just have a HasSubjectAltName() property on the
> > certificate itself?
> 
> No. Generally, I explicitly try to avoid adding those APIs, because we saw far
> too rampant usage, or too much coercion to X509Certificate. Our goal is to
make
> X509Certificate as light as possible and basically just be a cache of some
> UI-accessed fields, and everything else to be done by consumers parsing/using
> accessors.

Ok. I was slightly worried about the perf of doing the throwaway work (e.g. with
the Cloudflare certificates that contain ~100 SANs). But if we can limit to the
COMMON_NAME_INVALID case above, I'm less worried.

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
File components/security_state/core/security_state.h (right):

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:148: bool san_is_missing;
> cert_missing_subject_alt_name ?

SGTM

[Ryan Sleevi, 2017-03-21 16:50:40]

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
File components/security_state/core/security_state.cc (right):

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.cc:225: if
(visible_security_state.certificate) {
On 2017/03/21 16:49:04, elawrence wrote:
> Could we change this to:
> 
> if (visible_security_state.certificate &&(visible_security_state.cert_status &
> net::COMMON_NAME_INVALID)) {
> 
> ... to improve performance?

I intentionally omitted this, because like SHA-1, we also have an enterprise
policy that allows this to work, but support will be removed. So I think it's
appropriate to signal regardless of whether it was flagged as an error (no
policy set) or the policy was set and the error was suppressed.

https://codereview.chromium.org/2761333002/diff/1/components/security_state/c...
components/security_state/core/security_state.cc:228:
visible_security_state.certificate.GetSubjectAltName(&dns_names, &ip_addrs);
On 2017/03/21 16:49:04, elawrence wrote:
> On 2017/03/21 16:41:41, Ryan Sleevi (conf till 3-24) wrote:
> > On 2017/03/21 16:22:54, elawrence wrote:
> > > Would it make sense to just have a HasSubjectAltName() property on the
> > > certificate itself?
> > 
> > No. Generally, I explicitly try to avoid adding those APIs, because we saw
far
> > too rampant usage, or too much coercion to X509Certificate. Our goal is to
> make
> > X509Certificate as light as possible and basically just be a cache of some
> > UI-accessed fields, and everything else to be done by consumers
parsing/using
> > accessors.
> 
> Ok. I was slightly worried about the perf of doing the throwaway work (e.g.
with
> the Cloudflare certificates that contain ~100 SANs). But if we can limit to
the
> COMMON_NAME_INVALID case above, I'm less worried.

I don't think we can/should, but I think that makes a more compelling argument
for HasSubjectAltName to avoid the extra copying.

[Ryan Sleevi, 2017-03-21 19:28:05]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[Ryan Sleevi, 2017-03-21 19:28:31]

Eric: What do you think of this optimized approach? :)

[Ryan Sleevi, 2017-03-21 19:28:48]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org

[Ryan Sleevi, 2017-03-21 19:28:48]

Adding eroman as //net reviewer for the //net changes

[commit-bot: I haz the power, 2017-03-21 19:28:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 19:42:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 19:42:13]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Ryan Sleevi, 2017-03-21 19:50:47]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 19:51:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 20:00:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 20:00:26]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[elawrence, 2017-03-21 20:11:33]

On 2017/03/21 19:28:31, Ryan Sleevi (conf till 3-24) wrote:
> Eric: What do you think of this optimized approach? :)

While complicated, it looks happier for performance. Thanks!

[eroman, 2017-03-21 21:09:55]

lgtm after these comments

https://codereview.chromium.org/2761333002/diff/40001/components/security_sta...
File components/security_state_strings.grdp (right):

https://codereview.chromium.org/2761333002/diff/40001/components/security_sta...
components/security_state_strings.grdp:20: The certificate for this site does
not contain a Subject Alternative Name extension.
... containing a domain name or ip ?

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate.h:200: // Unrequired parameters may be passed as
nullptr.
unrequired --> optional

In fact, how about just deleting that line and leaving nullity explanations to
the one below:

Returns true if any dNSName or iPAdress SAN was present. The two parameters are
optional and may be null. If |dns_names| is non-null any found dNSNames will be
appended to it.... etc.

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:127: continue;
Is it expected that this code-path sets |has_san = true|, however does not
correspondingly fill |dns_names| ? This is a deviation in meaning of |has_san|
from patchset 1, which tested non-emptiness of the out-parameters.

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_ios.cc:137: continue;
Same here and other continue.

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_mac.cc (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_mac.cc:223: dns_names->clear();
Note the openssl implementations differ, and do a straight up append rather than
assignment (as the rest seem to do). Please clarify the expectation for
implementations in the common header

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_openssl.cc:117: continue;
Same comments as with the ios one (which this version duplicates)

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:409:
EXPECT_TRUE(san_cert->GetSubjectAltName(nullptr, &ip_addresses));
Add a test to verify the assign vs append behavior is consistent?

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:439:
ASSERT_NE(static_cast<X509Certificate*>(NULL), no_san_cert.get());
nit: ASSERT_TRUE() -- is printing the pointer on failure really useful?

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_win.cc (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_win.cc:172: int num_entries = alt_name->cAltEntry;
Why the implicit signed cast? (I believe cAltEntry is a DWORD).

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_util_nss.h
File net/cert/x509_util_nss.h (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_util_nss....
net/cert/x509_util_nss.h:44: // If |dns_names| is non-NULL, each dNSName will be
stored in |*dns_names|.
(this is the style comment I was suggesting for the common header).

[lgarron, 2017-03-21 21:22:53]

I haven't worked with most of this code, but the string construction LGTM.

https://codereview.chromium.org/2761333002/diff/40001/components/security_sta...
File components/security_state_strings.grdp (right):

https://codereview.chromium.org/2761333002/diff/40001/components/security_sta...
components/security_state_strings.grdp:20: The certificate for this site does
not contain a Subject Alternative Name extension.
On 2017/03/21 at 21:09:54, eroman wrote:
> ... containing a domain name or ip ?

"does not contain a valid Subject Alternative Name extension"?

[Ryan Sleevi, 2017-03-21 21:38:48]

On 2017/03/21 21:22:53, lgarron wrote:
> I haven't worked with most of this code, but the string construction LGTM.
> 
>
https://codereview.chromium.org/2761333002/diff/40001/components/security_sta...
> File components/security_state_strings.grdp (right):
> 
>
https://codereview.chromium.org/2761333002/diff/40001/components/security_sta...
> components/security_state_strings.grdp:20: The certificate for this site does
> not contain a Subject Alternative Name extension.
> On 2017/03/21 at 21:09:54, eroman wrote:
> > ... containing a domain name or ip ?
> 
> "does not contain a valid Subject Alternative Name extension"?

Opted with Eric's wording, since it's valid to have a SAN, but just not a type
we support :)

[Ryan Sleevi, 2017-03-21 21:48:37]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[Ryan Sleevi, 2017-03-21 21:48:44]

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_mac.cc (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_mac.cc:223: dns_names->clear();
On 2017/03/21 21:09:54, eroman wrote:
> Note the openssl implementations differ, and do a straight up append rather
than
> assignment (as the rest seem to do). Please clarify the expectation for
> implementations in the common header

No, they reset in the GetSubjectAltName and then append in the Parse

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:409:
EXPECT_TRUE(san_cert->GetSubjectAltName(nullptr, &ip_addresses));
On 2017/03/21 21:09:54, eroman wrote:
> Add a test to verify the assign vs append behavior is consistent?

That's done in 443

https://codereview.chromium.org/2761333002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:439:
ASSERT_NE(static_cast<X509Certificate*>(NULL), no_san_cert.get());
On 2017/03/21 21:09:54, eroman wrote:
> nit: ASSERT_TRUE() -- is printing the pointer on failure really useful?

I opted not to include this, to keep with local style. I think that's a separate
cleanup.

[commit-bot: I haz the power, 2017-03-21 21:49:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 21:59:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 21:59:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2017-03-21 22:04:00]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 22:04:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 22:21:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:21:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Sleevi, 2017-03-21 22:35:21]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 22:35:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2017-03-21 23:03:42]

rsleevi@chromium.org changed reviewers:
+ estark@chromium.org

[Ryan Sleevi, 2017-03-21 23:03:43]

Ahh, need estark as OWNERS :)

[commit-bot: I haz the power, 2017-03-21 23:19:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 23:19:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2017-03-21 23:33:03]

lgtm with a couple inline comments

https://codereview.chromium.org/2761333002/diff/120001/components/security_st...
File components/security_state/content/content_utils.cc (right):

https://codereview.chromium.org/2761333002/diff/120001/components/security_st...
components/security_state/content/content_utils.cc:230:
security_style_explanations->unauthenticated_explanations.push_back(
I think this probably should be |broken_explanations|, at least in the case
where IsCertStatusError is true.

Apologies that these fields are poorly named/documented, and not particularly
well-differentiated in the UI right now, but |unauthenticated_explanations| is
supposed to be "This is why your page got downgraded to the (i) icon" and
|broken_explanations| is supposed to be "This is why your page has the red
Dangerous triangle."

(filed crbug.com/703882 to clean up the field names/comments)

https://codereview.chromium.org/2761333002/diff/120001/components/security_st...
File components/security_state/content/content_utils_unittest.cc (right):

https://codereview.chromium.org/2761333002/diff/120001/components/security_st...
components/security_state/content/content_utils_unittest.cc:237: // Test that an
explanation is provided if a certificate is missing a
nit: Tests

[Ryan Sleevi, 2017-03-22 00:10:47]

https://codereview.chromium.org/2761333002/diff/120001/components/security_st...
File components/security_state/content/content_utils.cc (right):

https://codereview.chromium.org/2761333002/diff/120001/components/security_st...
components/security_state/content/content_utils.cc:230:
security_style_explanations->unauthenticated_explanations.push_back(
On 2017/03/21 23:33:03, estark wrote:
> I think this probably should be |broken_explanations|, at least in the case
> where IsCertStatusError is true.
> 
> Apologies that these fields are poorly named/documented, and not particularly
> well-differentiated in the UI right now, but |unauthenticated_explanations| is
> supposed to be "This is why your page got downgraded to the (i) icon" and
> |broken_explanations| is supposed to be "This is why your page has the red
> Dangerous triangle."
> 
> (filed crbug.com/703882 to clean up the field names/comments)

I left it as broken for all cases, does that work? If only because it's the
"thing that will break soon" (but we don't presently degrade the lock icon in
the case of enterprise policy)

[estark, 2017-03-22 00:16:21]

https://codereview.chromium.org/2761333002/diff/120001/components/security_st...
File components/security_state/content/content_utils.cc (right):

https://codereview.chromium.org/2761333002/diff/120001/components/security_st...
components/security_state/content/content_utils.cc:230:
security_style_explanations->unauthenticated_explanations.push_back(
On 2017/03/22 00:10:47, Ryan Sleevi (conf till 3-24) wrote:
> On 2017/03/21 23:33:03, estark wrote:
> > I think this probably should be |broken_explanations|, at least in the case
> > where IsCertStatusError is true.
> > 
> > Apologies that these fields are poorly named/documented, and not
particularly
> > well-differentiated in the UI right now, but |unauthenticated_explanations|
is
> > supposed to be "This is why your page got downgraded to the (i) icon" and
> > |broken_explanations| is supposed to be "This is why your page has the red
> > Dangerous triangle."
> > 
> > (filed crbug.com/703882 to clean up the field names/comments)
> 
> I left it as broken for all cases, does that work? If only because it's the
> "thing that will break soon" (but we don't presently degrade the lock icon in
> the case of enterprise policy)

Yeah, that's fine by me, thanks.

[Ryan Sleevi, 2017-03-22 00:21:32]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 00:22:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 02:24:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 02:24:19]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[Ryan Sleevi, 2017-03-22 02:50:02]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2017-03-22 02:50:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
elawrence@chromium.org, lgarron@chromium.org, eroman@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2761333002/#ps140001
(title: "Feedback & fixes")

[commit-bot: I haz the power, 2017-03-22 02:51:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 04:48:03]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490151002282160,
"parent_rev": "4ee95d7a57fbb7f3c17241d28b4b3969127faafe", "commit_rev":
"74a7a294c2802cf5b65bf0c0d4f44ba093aae216"}

[commit-bot: I haz the power, 2017-03-22 04:48:52]

Description was changed from

==========
Add a DevTools warning for a missing subjectAltName

BUG=703616
==========

to

==========
Add a DevTools warning for a missing subjectAltName

BUG=703616

Review-Url: https://codereview.chromium.org/2761333002
Cr-Commit-Position: refs/heads/master@{#458631}
Committed:
https://chromium.googlesource.com/chromium/src/+/74a7a294c2802cf5b65bf0c0d4f4...
==========

[commit-bot: I haz the power, 2017-03-22 04:48:53]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/74a7a294c2802cf5b65bf0c0d4f4...