Closed
Bug 1403699
Opened 5 years ago
Closed 5 years ago
stylo: Use a single lock in ServoBindings.cpp
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: bholley, Assigned: bradwerth)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
bholley
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
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.
Reporter | ||
Comment 1•5 years ago
|
||
Brad, I'd suggest starting with bug 1403690, and tackling this next.
Depends on: 1403690
Reporter | ||
Updated•5 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd7ae8b418415cfa41e4d965141c286ec5abddb7
Assignee | ||
Updated•5 years ago
|
Attachment #8914899 -
Flags: review?(bobbyholley)
Reporter | ||
Comment 4•5 years ago
|
||
mozreview-review |
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
Comment 6•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7e8237735403
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Reporter | ||
Comment 8•5 years ago
|
||
(In reply to Brad Werth [:bradwerth] from comment #7) > Bobby, you want this uplifted for 57? Yes.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 9•5 years ago
|
||
[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
Reporter | ||
Updated•5 years ago
|
Flags: needinfo?(bobbyholley)
Target Milestone: mozilla57 → mozilla58
Reporter | ||
Comment 10•5 years ago
|
||
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?
status-firefox57:
--- → affected
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+
Comment 12•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b2fab82b2d77
You need to log in
before you can comment on or make changes to this bug.
Description
•