1142849 - Reduce TSan violations in imgRequest on Aurora and Beta

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Upliftable fix for imgRequest TSan violations

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

Comment 2

[Seth Fowler [:seth] [:s2h]]

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 3

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1a3f6abf86e8

Comment 4

[Seth Fowler [:seth] [:s2h]]

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.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 6

[Seth Fowler [:seth] [:s2h]]

Thanks for the approval! Pushed to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/42d90bceb017

Comment 7

[Seth Fowler [:seth] [:s2h]]

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.

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b7aa96d0e11

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9b7aa96d0e11

Comment 11

[Seth Fowler [:seth] [:s2h]]

We've accomplished what we wanted to do in this bug.