1339010 - Assertion failure: isValid (Accumulation using invalid id), at TelemetryHistogram.cpp:1334

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

Bughunter run into this while loading https://9anime.to/watch/eldlive-dub.p1vq/pjjyvq

Talked to dexter and seems this might be interesting for you guys. From the logs it seems that just visiting this site caused the assertion but still looking into it.

Also it might be related to bug 1338814 somehow

Comment 1

[Georg Fritzsche [:gfritzsche]]

I can't immediately trigger this on Nightly.

Is this with e10s on or off?
Which Firefox version and platform?
For STR, is allowing Flash required?

Quickly checking over SSLServerCertVerification.cpp, there are quite a few histogram accumulations, most of them directly using Telemetry enum ids.
It would be good to at least track it to the specific line.

Comment 2

[Carsten Book [:Tomcat]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> I can't immediately trigger this on Nightly.
> 
> Is this with e10s on or off?
e10s is on


> Which Firefox version and platform?

aurora debug:   Windows NT 10.0 x86 32/32   Windows NT 6.1 x86 32/32 3   
beta debug:   Windows NT 6.1 x86 32/32    Windows NT 10.0 x86 32/32 3   
nightly debug:   Windows NT 10.0 x86 32/32   

> For STR, is allowing Flash required?

i don't think we have flash installed on the test workers anymore (bc do you know?)

> 
> Quickly checking over SSLServerCertVerification.cpp, there are quite a few
> histogram accumulations, most of them directly using Telemetry enum ids.
> It would be good to at least track it to the specific line.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Which channel and build is the stack here from?

Comment 4

[Carsten Book [:Tomcat]]

Attachment: windows 10 trunk stack

this is from a windows 10 trunk build from mozilla-central

Comment 5

[Carsten Book [:Tomcat]]

channel is i guess nightly then (since this is also the default). We take the builds from m-c (like latest m-c build via taskcluster)

Comment 6

[Georg Fritzsche [:gfritzsche]]

This is more helpful, thanks.
This points us specifically here:
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/security/manager/ssl/SSLServerCertVerification.cpp#1369

... which is one the places that uses a variable for the histogram id.

Cykesiopka, i see you recently touched these files, do you have an idea on why `pinningTelemetryInfo.certPinningResultHistogram` could contain invalid values?

mgoodwin also might have context as i see he reworked the key pinning telemetry at some point.

Comment 7

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Well, it looks like some of that code might be using uninitialized values.

Comment 8

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: bug 1339010 - ensure pinning and CT telemetry info has been initialized

Review commit: https://reviewboard.mozilla.org/r/112254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112254/

Comment 9

[Georg Fritzsche [:gfritzsche]]

Bug 1339365 shows that 51+ is affected.
If this histogram is important for monitoring, you should consider uplifting this patch to 52+.

Comment 10

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8836949 [details]
bug 1339010 - ensure pinning and CT telemetry info has been initialized

https://reviewboard.mozilla.org/r/112254/#review113804

This looks like it should resolve the uninitialized value errors. LGTM.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb7b874ca753
ensure pinning and CT telemetry info has been initialized r=jcj

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb7b874ca753

Comment 13

[Carsten Book [:Tomcat]]

David : per comment #9 could you request uplifting ? Thanks!

Comment 14

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: patch for aurora/beta

Approval Request Comment
[Feature/Bug causing the regression]: long-standing issue
[User impact if declined]: potential assertion failures, bogus telemetry data for us
[Is this code covered by automated tests?]: this area of code, yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no (I'm writing a test that will cover exactly this case, but it first requires fixing bug 1339923)
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: initializing memory is almost always more correct than not
[String changes made/needed]: none

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8838242 [details] [diff] [review]
patch for aurora/beta

Fix an assertion failure. Aurora53+.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8838242 [details] [diff] [review]
patch for aurora/beta

tls telemetry fix, beta52+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4c83176013d3

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bb25cae5b8f7

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/bb25cae5b8f7

Comment 20

[babubill]

iam unable to load content completely for this site https://govtjobsbabu.com/ loading partially without css and javascript in mozilla firefox but working fine on chrome 

can anyone tell me the problem

Comment 21

[Alessio Placitelli [:Dexter]]

(In reply to babubill from comment #20)
> iam unable to load content completely for this site
> https://govtjobsbabu.com/ loading partially without css and javascript in
> mozilla firefox but working fine on chrome 
> 
> can anyone tell me the problem

I'm not sure that's related to this bug. Please seek for help at https://support.mozilla.org/ or in the #firefox IRC channel on irc.mozilla.org.