1749214 - Various locking issues with nsHttpConnectionMgr

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Thread-safety analysis found a bunch of issues with nsHttpConnectionMgr.

Some certainly seem to be real issues; a bunch of others look like the values are only ever accessed off the socket thread when they're modified in OnMsgUpdateParam; if that could be moved to run on SocketThread, there wouldn't be any data races for many of these values, and we could drop a bunch (not all) of the locks I've added.

This patch passes cleanly, but you'll see what I mean. Many (perhaps not all) of the locks that could be dropped are marked with a comment indicating I'd like to remove it.

There are some real issues here too, I believe (and those values updated in OnMsgUpdateParam could cause real data races and undefined behavior as-is).

Comment 1

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

Attachment: Bug 1749214: nsHttpConnectionMgr cleanup r=kershaw

Comment 2

[Kershaw Chang [:kershaw]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #0)

Thread-safety analysis found a bunch of issues with nsHttpConnectionMgr.

Some certainly seem to be real issues; a bunch of others look like the values are only ever accessed off the socket thread when they're modified in OnMsgUpdateParam; if that could be moved to run on SocketThread, there wouldn't be any data races for many of these values, and we could drop a bunch (not all) of the locks I've added.

OnMsgUpdateParam is only used here, so it should be only called on socket thread. I assume most of the locks you've added can be dropped?

Comment 3

[Dragana Damjanovic [:dragana]]

nsHttpConnectionMgr should only be modified on the socket tread. All functions that are called on another thread are posted to the socket thread (the code uses a very old code for posting, this one, some are modernized).
If the above is not true that we are in trouble.

Comment 4

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

Aha, thanks! Since OnMsgUpdateParam is socketthread-only, I can remove most of those locks (and assert it's socket-thread only)

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/fac26885b741d046e5e01cdb4f645afeb860a0da

Backed out for build bustage:
https://hg.mozilla.org/integration/autoland/rev/a1b9ab302b5e305b0c75c37d51fd68fa2a76f444

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=fac26885b741d046e5e01cdb4f645afeb860a0da&selectedTaskRun=XUi1bm4_Qo2A-Gxpd8Vl0g.0
Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=363854688&repo=autoland&lineNumber=9424
Failure message : /builds/worker/checkouts/gecko/netwerk/protocol/http/nsHttpConnectionMgr.h:213:47: error: expected ';' at end of declaration list

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

nsHttpConnectionMgr cleanup r=kershaw,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/4bb5d4538395b34edc7fbb17d87c01f22b8f5712
https://hg.mozilla.org/mozilla-central/rev/4bb5d4538395

Comment 7

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

The patch landed in nightly and beta is affected.
:jesup, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 8

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

I don't see a need to uplift this