Closed Bug 979252 Opened 6 years ago Closed 6 years ago

Stop using GetVersionEx from SQLite

Categories

(Toolkit :: Storage, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: emk, Assigned: emk)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
See the added comment in moz.build about the description.
Attachment #8385272 - Flags: review?(mak77)
Actually, Sqlite already avoids using GetVersionEx if the windows sdk version is > 8.1 per this check-in http://www.sqlite.org/src/info/0ea9e4722be10221c99cce5bc48d13c7b34e739f
Why should we avoid using it also on previous sdk where it was not deprecated?
(In reply to Marco Bonardo [:mak] from comment #1)
> Actually, Sqlite already avoids using GetVersionEx if the windows sdk
> version is > 8.1 per this check-in
> http://www.sqlite.org/src/info/0ea9e4722be10221c99cce5bc48d13c7b34e739f

We are using the 8.0 SDK (not 8.1).

> Why should we avoid using it also on previous sdk where it was not
> deprecated?

Microsoft deprecated using GetVersionEx. It is not limited to developers are using 8.1 SDK. The 8.1 SDK just makes it explicit for GetVersionEx to be deprecated.
Also, GetVersionEx can actually tell a lie. Just assuming WinNT will be more stable because there is no possibility that we are running on Win9x.
I still don't see why we should fix this also on our side when there's a fix upstream that doesn't seem to cause any trouble. What's our advantage in taking this patch compared to the current situation? Are we getting warnings or such during a build?
Since if there's no benefit I think the upstream fix is more than enough.
(In reply to Marco Bonardo [:mak] from comment #3)
> I still don't see why we should fix this also on our side when there's a fix
> upstream that doesn't seem to cause any trouble.

The upstream fix has no effect as long as we are using 8.0 SDK.

> What's our advantage in
> taking this patch compared to the current situation? Are we getting warnings
> or such during a build?
> Since if there's no benefit I think the upstream fix is more than enough.

I already explained the benefit in comment #2.
Frankly speaking, the upstream fix is bogus. It will change the generated binary depending on that the developer happens to use what version of SDK. Microsoft deprecated using GetVersionEx because it will tell a lie at *runtime*. We should not be lazy just because or current SDK do not warn the deprecated usage yet.
Comment on attachment 8385272 [details] [diff] [review]
patch

Review of attachment 8385272 [details] [diff] [review]:
-----------------------------------------------------------------

while I still think this doesn't really bring tangible benefits, it's so harmless that I'm not going to fight a bikeshed battle. Either way doesn't break us.

Though unfortunately you are setting the define to 1, that means "always use GetVersionEx"
Attachment #8385272 - Flags: review?(mak77) → review-
Attached patch patch v2Splinter Review
Oops, good catch.
Attachment #8385272 - Attachment is obsolete: true
Attachment #8387542 - Flags: review?(mak77)
Comment on attachment 8387542 [details] [diff] [review]
patch v2

Review of attachment 8387542 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following change

::: db/sqlite3/src/moz.build
@@ +43,5 @@
>  
>  DEFINES['SQLITE_DEFAULT_PAGE_SIZE'] = 32768
>  DEFINES['SQLITE_MAX_DEFAULT_PAGE_SIZE'] = 32768
>  DEFINES['SQLITE_MAX_SCHEMA_RETRY'] = 25
> +DEFINES['SQLITE_WIN32_GETVERSIONEX'] = 0

while doesn't hurt, for correctness should be set only
if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows':
Attachment #8387542 - Flags: review?(mak77) → review+
Thanks you. Landed with the fix.
https://hg.mozilla.org/integration/mozilla-inbound/rev/39523a8931d5
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/39523a8931d5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.