IsWindowsVersionOrLater/IsWindowsBuildOrLater/IsWindows10BuildOrLater aren't thread safe

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

unspecified
mozilla58
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

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.
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.
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.
Attachment #8916538 - Flags: review?(nfroyd)
Assignee: nobody → jyavenard
Thanks David for fixing the reviewer...
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.
Attachment #8916538 - Flags: review?(nfroyd) → review+
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
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4bf0f40c6755
Make IsWindows* methods thread-safe. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/4bf0f40c6755
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.