979252 - Stop using GetVersionEx from SQLite

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

See the added comment in moz.build about the description.

Comment 1

[Marco Bonardo [:mak]]

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?

Comment 2

[Masatoshi Kimura [:emk]]

(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.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Masatoshi Kimura [:emk]]

(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 5

[Marco Bonardo [:mak]]

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"

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Oops, good catch.

Comment 7

[Marco Bonardo [:mak]]

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':

Comment 8

[Masatoshi Kimura [:emk]]

Thanks you. Landed with the fix.
https://hg.mozilla.org/integration/mozilla-inbound/rev/39523a8931d5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39523a8931d5