1747439 - Fix a bunch of locking/race issues in cache2

Status: RESOLVED FIXED

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

Description

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

Clang static thread-safety analysis found a number of issues in netwerk/cache2. See the patch. I moved mPinned to an Atomic to avoid adding a bunch of additional locking (similar to mDoomed). (I'll note that if mDoomed hadn't already been moved to an Atomic in bug 1692299 a few days ago, I would have had fixes for that too - it had flagged that as not locked properly). Also does some minor related cleanup (I think we were locking around mDoomed in at least one place still).

Comment 1

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

Attachment: Bug 1747439: netwerk/cache2 cleanup r=#necko-reviewers

Comment 2

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

Landed: https://hg.mozilla.org/integration/autoland/rev/7c1a1109d729b5c52f547c2836c34a8a358b9464

Backed out for causing build bustages in CacheEntry.cpp:
https://hg.mozilla.org/integration/autoland/rev/baba2bcc677f19df575c4f50c3541661395ce41f

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=adcafb4a8a20400decd88d0b7501d407afa51aa4&tochange=baba2bcc677f19df575c4f50c3541661395ce41f&selectedTaskRun=EtFAzYXdQcGlK8TV-XZShQ.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=363868651&repo=autoland&lineNumber=21507

/builds/worker/checkouts/gecko/netwerk/cache2/CacheEntry.cpp:1899:20: error: out-of-line definition of 'SizeOfExcludingThis' does not match any declaration in 'mozilla::net::CacheEntry'
/builds/worker/checkouts/gecko/netwerk/cache2/CacheEntry.cpp:1922:20: error: out-of-line definition of 'SizeOfIncludingThis' does not match any declaration in 'mozilla::net::CacheEntry' 

Comment 3

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

Aha. There was a mis-merge from hg histedit. Fixed

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22300863a3f0

Comment 5

[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 6

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

Comment on attachment 9257733 [details]
Bug 1747439: netwerk/cache2 cleanup r=#necko-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Possible data races
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): adds/moves a few locks. Locks verified by static analysis, and baked on Nightly for a few weeks with no reported deadlocks
• String changes made/needed: none

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9257733 [details]
Bug 1747439: netwerk/cache2 cleanup r=#necko-reviewers

Approved for 97.0b9.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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