1400965 - stylo: Gecko_GetFontMetrics can run concurrently with nsLookAndFeel::NativeGetColor, which also calls into gtk

Status: RESOLVED INVALID

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

Description

[Bobby Holley (:bholley)]

See bug 1354966 comment 20.

Comment 1

[Brad Werth [:bradwerth]]

I'd like to solve the whole problem here, if possible. I don't know how to determine if there are other Gecko_ calls in ServoBindings.cpp that could possibly overlap. When the static analysis is available, we might see more pairings. At the moment there two mutexes to protect different classes of calls. This bug notes a problematic third entry point that overlaps with one of the first two. I imagine we might find more cases like this. Presuming that, I'm inclined to replace the narrow mutexes with a common sServoGTKLock to be used in all the cases, and to be exposed publically (so something like nsLookAndFeel could guard on it).

I'll pursue that strategy for this bug in the interest of future-proofing.

Comment 2

[Bobby Holley (:bholley)]

That seems sensible, though we should probably call it sServoPlatformAPILock instead. Manish, was there any particular reason we used multiple locks?

Comment 3

[Manish Goregaokar [:manishearth]]

> Manish, was there any particular reason we used multiple locks?


No, aside from potential perf/contention concerns (which we can measure)

Comment 4

[Brad Werth [:bradwerth]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> That seems sensible, though we should probably call it sServoPlatformAPILock instead.

Well, maybe. It seems to me that our concern is not keeping Servo threads from calling ANY library concurrently; we only care about them calling the same library concurrently. So today we have GTK, but if we later added, I dunno fmod or something, we wouldn't need to keep GTK and fmod calls from being concurrent. So I think one lock per non-thread-safe library is appropriate. It will also make it clear when code needs to request a guard -- does the code call a GTK function? Then request the GTK guard.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Brad Werth [:bradwerth] from comment #4)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> > That seems sensible, though we should probably call it sServoPlatformAPILock instead.
> 
> Well, maybe. It seems to me that our concern is not keeping Servo threads
> from calling ANY library concurrently; we only care about them calling the
> same library concurrently. So today we have GTK, but if we later added, I
> dunno fmod or something, we wouldn't need to keep GTK and fmod calls from
> being concurrent. So I think one lock per non-thread-safe library is
> appropriate. It will also make it clear when code needs to request a guard
> -- does the code call a GTK function? Then request the GTK guard.

The issue isn't about GTK vs fmod. It's about GTK vs win32 vs cocoa. I care much less about allowing one style thread to access Gtk while another accesses fmod than I do about preventing races in the very small set of platform APIs that the style threads can reach. That is why we want coarser-grained locks, and the finer-grained locks you're proposing would take us in the opposite direction.

Comment 6

[Brad Werth [:bradwerth]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> The issue isn't about GTK vs fmod. It's about GTK vs win32 vs cocoa. I care
> much less about allowing one style thread to access Gtk while another
> accesses fmod than I do about preventing races in the very small set of
> platform APIs that the style threads can reach. That is why we want
> coarser-grained locks, and the finer-grained locks you're proposing would
> take us in the opposite direction.

Thanks for explaining it. I'll do as you suggest in comment 2.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1400965 Part 1: Consolidate existing ServoBindings mutexes into one platform API mutex.

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

Comment 8

[Brad Werth [:bradwerth]]

Attachment: Bug 1400965 Part 2: Protect gtk calls with lock on Servo platform API mutex.

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

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8909527 [details]
Bug 1400965 Part 2: Protect gtk calls with lock on Servo platform API mutex.

https://reviewboard.mozilla.org/r/180994/#review186232

This doesn't help at all on non-Linux platforms, right?

How about doing the thing karlt proposed and precaching the various values across the platforms, so that NativeGetColor never needs to do a platform API call. There are only a couple of them, I think?

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8909526 [details]
Bug 1400965 Part 1: Consolidate existing ServoBindings mutexes into one platform API mutex.

https://reviewboard.mozilla.org/r/180992/#review186236

So, per discussion on IRC, I don't think we have any indication that we're actually calling into the NativeGetColor machinery from Gecko_GetFontMetrics. So I don't think we actually need to do anything here.