1403699 - stylo: Use a single lock in ServoBindings.cpp

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Bobby Holley (:bholley)]

Right now we have 3 locks:

> static Mutex* sServoFontMetricsLock = nullptr;
> static Mutex* sServoWidgetLock = nullptr;
> static RWLock* sServoLangFontPrefsLock = nullptr;

sServoWidget lock should hopefully go away in bug 1403690, but we'll still have two. This weakens the static analysis, because we have to whitelist everything in the call graph when we hold the lock.

So I think we should have a single RwLock called sServoFFILock, and use it everywhere.

Using a single lock still isn't perfect, since we can still read-write between a thread that holds the lock and mutates and another thread that reads the same memory. But it should prevent write-write races, and generally reduce risk. So I think the safety improvement is worth the additional contention.

Comment 1

[Bobby Holley (:bholley)]

Brad, I'd suggest starting with bug 1403690, and tackling this next.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1403699 Part 1: Unify ServoBindings synchronization primitives into a single RWLock.

Review commit: https://reviewboard.mozilla.org/r/186154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186154/

Comment 3

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd7ae8b418415cfa41e4d965141c286ec5abddb7

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8914899 [details]
Bug 1403699 Part 1: Unify ServoBindings synchronization primitives into a single RWLock.

https://reviewboard.mozilla.org/r/186154/#review191264

Looks great, thanks.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e8237735403
Part 1: Unify ServoBindings synchronization primitives into a single RWLock. r=bholley

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e8237735403

Comment 7

[Brad Werth [:bradwerth]]

Bobby, you want this uplifted for 57?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Brad Werth [:bradwerth] from comment #7)
> Bobby, you want this uplifted for 57?

Yes.

Comment 9

[Brad Werth [:bradwerth]]

[Feature/regressing bug #]: Not related to a specific bug, but part of the effort to strengthen static analysis deployed in Bug 1294915.
[User impact if declined]: Static analysis currently unable to detect contention between multiple sync primitives; could allow through dangerous code.
[Describe test coverage new/current, TBPL]: No new tests. Static analysis already set to absorb this change.
[Risks and why]: Medium. Could introduce deadlock in re-entrant "Gecko_" calls in Stylo builds (none of these patterns have been found and any that do exist should be considered bugs). These would manifest as application hangs.
[String/UUID change made/needed]: none

needinfo on Bobby to set appropriate flags.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8914899 [details]
Bug 1403699 Part 1: Unify ServoBindings synchronization primitives into a single RWLock.

I think we should take this in b7, to give it more bake time on nightly.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914899 [details]
Bug 1403699 Part 1: Unify ServoBindings synchronization primitives into a single RWLock.

Stylo related, Beta57+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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