1406793 - IsWindowsVersionOrLater/IsWindowsBuildOrLater/IsWindows10BuildOrLater aren't thread safe

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Jean-Yves Avenard [:jya]]

IsWindowsBuildOrLater and IsWindows10BuildOrLater store previous results (to speed up future access I guess) in static variables.

Both IsWindows10BuildOrLater and IsWindows10BuildOrLater  are called from various threads.

There's no locking mechanism to protect the way those results are stored or access.

Comment 1

[Mike Hommey [:glandium]]

How does it matter? The worst that can happen is that the function code runs twice, gets the same value twice, and stores the same value twice.

Comment 2

[Jean-Yves Avenard [:jya]]

The writes being non atomic, if you have simultaneous write access to the variable, the result is undefined.
As such, the result of the next run is also undefined. 

Of all people, data races should always be a concern.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1406793 - Make IsWindows* methods thread-safe.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Thanks David for fixing the reviewer...

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8916538 [details]
Bug 1406793 - Make IsWindows* methods thread-safe.

https://reviewboard.mozilla.org/r/187674/#review192870

If this was cross-platform code, I think I would care a little more about efficiency, but since this is Windows-only and we'll always be executing on x86, where sequentially consistent atomics are less of a problem (particularly for loads, which is what matters here), I think this is OK.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8916538 [details]
Bug 1406793 - Make IsWindows* methods thread-safe.

https://reviewboard.mozilla.org/r/187674/#review192870

thanks for the review.

im interested to know however, how you could make this more efficient on other platforms.
as it, the code quickly settle in a state wheremthe first two comparisons handle all cases, and the main code never runs

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bf0f40c6755
Make IsWindows* methods thread-safe. r=froydnj

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4bf0f40c6755