1969141 - gtk_check_version logic is backwards for 1958174's fix

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[jayisoverhere]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:139.0) Gecko/20100101 Firefox/139.0

Steps to reproduce:

ran firefox 139 with the patch for working around the gtk issue described in 1958174. built with gtk 3 24 49.

Actual results:

buggy behavior still appeared described in 1958174.

Expected results:

the patch works as expected, but the version check (1959565) causes the conditional to incorrectly fail.
https://docs.gtk.org/gtk3/func.check_version.html#return-value
gtk_check_version(3, 24, 50) return would be 'true' if the version is less then 3 24 50 is the way i read the docs (?)

Comment 1

[jayisoverhere]

gdb for ref (testing against version 3 24 49):

p (gchar*)gtk_check_version(3, 24, 50)
  (gchar *) 0x7ffff5e98e40 "GTK+ version too old (micro mismatch)"
p (gchar*)gtk_check_version(3, 24, 49)
  (gchar *) 0x0p (gchar*)gtk_check_version(3, 24, 49)
  (gchar *) 0x0
p ((gchar(*)())gtk_check_version)(3, 24, 50)
  64 '@'
p ((gchar(*)())gtk_check_version)(3, 24, 49)
  0 '\000'

Comment 2

[jayisoverhere]

also, pinging :emilio since i see activity on the OG report, but i cannot comment there.

Comment 3

[jayisoverhere]

just to add, in nsWindow.cpp there are gtk_check_version calls that compare to nullptr which might be nicer looking since the intent is "more clear"? gtk_check_version(X, X, X) != nullptr kind of is nicer for "doesnt meet version requirements"

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1969141 - Fix version check for bug 1958174's workaround. r=stransky

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to jayisoverhere from comment #3)

just to add, in nsWindow.cpp there are gtk_check_version calls that compare to nullptr which might be nicer looking since the intent is "more clear"? gtk_check_version(X, X, X) != nullptr kind of is nicer for "doesnt meet version requirements"

That is generally discouraged by our coding style. Maybe an exception could be made here but... In any case it seems I got this wrong because there was another wrong check right above :'(

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1969141 - Fix version check for another workaround. r=stransky

This is probably why I got the other one wrong as well :(

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Maybe we should introduce a clearer bool IsGtkVersionAtLeast(...) function or something...

Comment 8

[Emilio Cobos Álvarez (:emilio)]

The other workaround seems it'd be more problematic to remove. I found that in bug 1775017, but we're no longer using it to implement that API. So I'll do some digging before landing to see where is it causing wrong behavior...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/85fba559f3e0
https://hg.mozilla.org/integration/autoland/rev/db31726c9e7c
Fix version check for bug 1958174's workaround. r=stransky

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c11e51613ad4
https://hg.mozilla.org/integration/autoland/rev/f5856b2d895b
Fix version check for another workaround. r=stransky

Comment 11

[jayisoverhere]

The other workaround seems it'd be more problematic to remove. I found that in bug 1775017, but we're no longer using it to implement that API. So I'll do some digging before landing to see where is it causing wrong behavior...

i tried looking into this- but got a bit lost anything i can test as far as regressions go?

thanks again for dealing with this corner case issue.

This is probably why I got the other one wrong as well :(
if the function return is already a bit unintuative to the name.... the macro version i think makes i doubly so.

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/db31726c9e7c
https://hg.mozilla.org/mozilla-central/rev/f5856b2d895b