Closed Bug 1403699 Opened 3 years ago Closed 3 years ago

stylo: Use a single lock in ServoBindings.cpp

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: bholley, Assigned: bradwerth)

References

Details

Attachments

(1 file)

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.
Brad, I'd suggest starting with bug 1403690, and tackling this next.
Depends on: 1403690
Priority: -- → P2
See Also: → 1354966
Attachment #8914899 - Flags: review?(bobbyholley)
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.
Attachment #8914899 - Flags: review?(bobbyholley) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e8237735403
Part 1: Unify ServoBindings synchronization primitives into a single RWLock. r=bholley
https://hg.mozilla.org/mozilla-central/rev/7e8237735403
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Bobby, you want this uplifted for 57?
Flags: needinfo?(bobbyholley)
(In reply to Brad Werth [:bradwerth] from comment #7)
> Bobby, you want this uplifted for 57?

Yes.
Flags: needinfo?(bobbyholley)
[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.
Flags: needinfo?(bobbyholley)
Target Milestone: mozilla58 → mozilla57
Flags: needinfo?(bobbyholley)
Target Milestone: mozilla57 → mozilla58
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.
Attachment #8914899 - Flags: approval-mozilla-beta?
Comment on attachment 8914899 [details]
Bug 1403699 Part 1: Unify ServoBindings synchronization primitives into a single RWLock.

Stylo related, Beta57+
Attachment #8914899 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.