Reduce TSan violations in imgRequest on Aurora and Beta

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment)

In bug 1137002, I'm working on eliminating the TSan violations in imgRequest. Unfortunately, the patches there are so large that they can't be uplifted to beta.

It'd be nice to reduce the impact of these TSan violations on beta, too. So in this bug, I'll create an alternate patch that eliminates races involving imgRequest::mImage and imgRequest::mProgressTracker.

For this one, because it's going to be uplifted, the goal is to keep the code as similar as possible to the existing code. That means we're going to be making some performance tradeoffs in this version of the patch.
Here's the patch. It was almost entirely sufficient to make accesses to mImage and mProgressTracker go through accessors that take the lock.

(Of course, there are other races that this patch doesn't even try to address, but these are the most important ones.)

Try job here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4eeca6c7a40
Attachment #8577045 - Flags: review?(tnikkel)
Attachment #8577045 - Flags: review?(tnikkel) → review+
Keywords: leave-open
Thanks for the review!

Before pushing this to beta, I want to land this on central temporarily and make sure there are no serious issues.

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a3f6abf86e8

Ultimately I'm going to back this out of central when the real fix is ready to land.
Comment on attachment 8577045 [details] [diff] [review]
Upliftable fix for imgRequest TSan violations

Approval Request Comment
[Feature/regressing bug #]: 867755
[User impact if declined]: TSan violations could cause significant bugs in theory, although we don't know of any caused by these violations. Lawrence and I have already talked about the tradeoffs.
[Describe test coverage new/current, TreeHerder]: On central.
[Risks and why]: Very low risk. (This patch was specially made to be upliftable.)
[String/UUID change made/needed]: None.
Attachment #8577045 - Flags: approval-mozilla-aurora?
Summary: Reduce TSan violations in imgRequest on Beta → Reduce TSan violations in imgRequest on Aurora and Beta
Comment on attachment 8577045 [details] [diff] [review]
Upliftable fix for imgRequest TSan violations

I have reviewed this change with Seth. We want to take this on m-c and m-a at the same time in order to shake out any issues with this change on the joint audience. Aurora+
Attachment #8577045 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks for the approval! Pushed to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/42d90bceb017
Comment on attachment 8577045 [details] [diff] [review]
Upliftable fix for imgRequest TSan violations

Approval Request Comment
[Feature/regressing bug #]: 867755
[User impact if declined]: TSan violations could cause significant bugs in theory, although we don't know of any caused by these violations. May fix bug 1135272.
[Describe test coverage new/current, TreeHerder]: On central and Aurora.
[Risks and why]: Very low risk. (This patch was specially made to be upliftable.)
[String/UUID change made/needed]: None.
Attachment #8577045 - Flags: approval-mozilla-beta?
Comment on attachment 8577045 [details] [diff] [review]
Upliftable fix for imgRequest TSan violations

We're going to take this fix in Beta 7 as it looks to fix topcrash bug 1135272. Beta+
Attachment #8577045 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
We've accomplished what we wanted to do in this bug.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.