1732885 - Investigate bug 1732249

Status: RESOLVED WORKSFORME

(Core :: Networking: HTTP, defect, P2)

Description

[Dragana Damjanovic [:dragana]]

We will add some diagnostic asserts to figure out why certificates are not already set when response is received.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: Add assertions to make sure certificates are set if a handshake succeeds.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bb05a86bd2e
Add assertions to make sure certificates are set if a handshake succeeds. r=necko-reviewers,valentin

Comment 3

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/5bb05a86bd2e

Comment 4

[Alexandru Michis [:malexandru]]

Backed out changeset 5bb05a86bd2e (Bug 1732885) for causing networking crashes. a=backout
Backout link: https://hg.mozilla.org/mozilla-central/rev/75ff6f392acad0ccffff8a6484dfcb0bfd1bc408
Autoland backout link: https://hg.mozilla.org/mozilla-central/rev/75ff6f392acad0ccffff8a6484dfcb0bfd1bc408

Comment 6

[Jens Stutte [:jstutte]]

I hit this yesterday on my Android nightly, too and thus took a look. At SetServerCert (which seems to be the only place we set mServerCert) there is a possible and probably expected code path that leaves us with a nullptr inside mServerCert if we have an mListener set. That means probably that we cannot expect an existing mSecurityInfo to always carry an initialized certificate?

Comment 7

[Dragana Damjanovic [:dragana]]

(In reply to Jens Stutte [:jstutte] from comment #6)

I hit this yesterday on my Android nightly, too and thus took a look. At SetServerCert (which seems to be the only place we set mServerCert) there is a possible and probably expected code path that leaves us with a nullptr inside mServerCert if we have an mListener set. That means probably that we cannot expect an existing mSecurityInfo to always carry an initialized certificate?

This is a server side socket, httpTransactions never hit that code.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:dragana, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

[Dragana Damjanovic [:dragana]]

The patch was made to investigate an issue, it will not land permanently.

Comment 10

[Dragana Damjanovic [:dragana]]

I will not have time to work on this.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

often-wildptr crashes; ~3-4 per week.

No CheckCert() failures in the last 6 months, so the diagnostic appears to be showing us nothing.

A fair number of the wildptr crashes seem to be at the call to HandleContent()

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:valentin, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

It's stalled.

Comment 14

[Daniel Veditz [:dveditz]]

I don't see any crashes in recent builds. a bunch of 102.5 which was the current ESR until last week, but nothing on main newer that 105

Maybe this is WFM now?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Changelog:
https://hg.mozilla.org/releases/mozilla-esr102/pushloghtml?fromchange=FIREFOX_102_5_0esr_RELEASE&tochange=FIREFOX_102_6_0esr_RELEASE

Comment 16

[Valentin Gosu [:valentin] (he/him)]

If I were to guess I think it was bug 1797370 that fixed this. Thank you for the regression range Ryan!

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Still no more crashes. Closing as WFM.

Comment 18

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.