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

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cbook, Assigned: keeler)

Tracking

(Blocks 1 bug, {assertion})

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [measurement:client:tracking][psm-assigned], )

Attachments

(4 attachments)

Posted file 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
Priority: -- → P2
Whiteboard: [measurement:client]
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.
(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.
Which channel and build is the stack here from?
Flags: needinfo?(cbook)
this is from a windows 10 trunk build from mozilla-central
Flags: needinfo?(cbook)
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)
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.
Flags: needinfo?(cykesiopka.bmo)
Component: Telemetry → Security: PSM
Priority: P2 → --
Product: Toolkit → Core
Whiteboard: [measurement:client] → [measurement:client:tracking]
Well, it looks like some of that code might be using uninitialized values.
Assignee: nobody → dkeeler
Flags: needinfo?(cykesiopka.bmo)
Priority: -- → P1
Whiteboard: [measurement:client:tracking] → [measurement:client:tracking][psm-assigned]
Bug 1339365 shows that 51+ is affected.
If this histogram is important for monitoring, you should consider uplifting this patch to 52+.
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.
Attachment #8836949 - Flags: review?(jjones) → review+
See Also: → 1339365
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb7b874ca753
ensure pinning and CT telemetry info has been initialized r=jcj
https://hg.mozilla.org/mozilla-central/rev/fb7b874ca753
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
David : per comment #9 could you request uplifting ? Thanks!
Flags: needinfo?(dkeeler)
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
Flags: needinfo?(dkeeler)
Attachment #8838242 - Flags: review+
Attachment #8838242 - Flags: approval-mozilla-beta?
Attachment #8838242 - Flags: approval-mozilla-aurora?
Comment on attachment 8838242 [details] [diff] [review]
patch for aurora/beta

Fix an assertion failure. Aurora53+.
Attachment #8838242 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8838242 [details] [diff] [review]
patch for aurora/beta

tls telemetry fix, beta52+
Attachment #8838242 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
(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.
You need to log in before you can comment on or make changes to this bug.