1358288 - js-ctypes call to GetVersionEx is using the wrong ABI

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

The ABI needs to be winapi_abi, not default_abi!

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

(This is in TelemetryEnvironment.jsm)

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1358288: Fix incorrect ABI specification for js-ctypes declaration of GetVersionExW;

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

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8860183 [details]
Bug 1358288: Fix incorrect ABI specification for js-ctypes declaration of GetVersionExW;

https://reviewboard.mozilla.org/r/132212/#review135268

Thanks!

A look through dxr shows that other code does this as well:
https://dxr.mozilla.org/mozilla-central/search?q=kernel32.declare&redirect=false

Do we have bugs on those?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce93446fe101
Fix incorrect ABI specification for js-ctypes declaration of GetVersionExW; r=gfritzsche

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce93446fe101

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> A look through dxr shows that other code does this as well:
> https://dxr.mozilla.org/mozilla-central/search?q=kernel32.
> declare&redirect=false
> 
> Do we have bugs on those?

In progress...

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8860183 [details]
Bug 1358288: Fix incorrect ABI specification for js-ctypes declaration of GetVersionExW;

Approval Request Comment
[Feature/Bug causing the regression]: JS code in Telemetry
[User impact if declined]: Potential for crashes and/or data corruption
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Trivial patch
[String changes made/needed]: None

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8860183 [details]
Bug 1358288: Fix incorrect ABI specification for js-ctypes declaration of GetVersionExW;

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a stability issue
User impact if declined: Potential for crashing / data corruption
Fix Landed on Version: 55, 54 requested
Risk to taking this patch (and alternatives if risky): None. Trivial patch
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8860183 [details]
Bug 1358288: Fix incorrect ABI specification for js-ctypes declaration of GetVersionExW;

Fix a potential crash. Beta54+. Should be in 54 beta 3.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b5526172339c

Comment 11

[Andrei Vaida [:avaida]]

(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now) from comment #7)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Aaron's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8860183 [details]
Bug 1358288: Fix incorrect ABI specification for js-ctypes declaration of GetVersionExW;

Cancelling ESR52 request. I have looked at the libffi code and there is no chance of crashing given the way it is implemented.