Need a way to detect GTK+ version so we don't break users on major update

RESOLVED FIXED in mozilla1.9beta5

Status

()

Toolkit
Application Update
P2
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: reed, Assigned: Michael Ventnor)

Tracking

({verified1.8.1.15})

Trunk
mozilla1.9beta5
x86
Linux
verified1.8.1.15
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.13 -
blocking1.8.1.15 +
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

11 years ago
Firefox 3 is going to require GTK+ 2.10 for running. We need to have a way to detect GTK+ versions using Software Update / AUS so that we don't send those users upgrade snippets that will just break their Firefox. This may require client changes on both branch and trunk.

It may end up that we just don't do a major update for Linux users and force them to download a new build themselves, which would solve this problem, as long as bug 418126 is fixed.
Flags: wanted1.8.1.x?
Flags: blocking-firefox3?
(Assignee)

Comment 1

11 years ago
Created attachment 303994 [details] [diff] [review]
Patch

I'm not sure what procedures of approval a change to AUS needs, but this patch will send the GTK version as part of the %OSVERSION% variable.

Reed, can you do me a really, really big favour; can you see if this can be patched against 1.8 without significant change? I live in a country where internet bandwidth is capped at this time of day (oligopolies :( ) so I can't checkout the 1.8 branch until the next morning.

I also don't know who is qualified to review this...
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
(Reporter)

Comment 2

11 years ago
Created attachment 303997 [details] [diff] [review]
1.8 branch version of patch

I'm crazy enough to have full checkouts of all three major branches (HEAD, MOZILLA_1_8_BRANCH, and MOZILLA_1_8_0_BRANCH), and I have a very nice university backbone supporting my Internet connection. Plus, it's 4 AM. ;)

Only conflicts were in toolkit/mozapps/update/src/nsUpdateService.js.in... The xpcom stuff patched fine. I didn't try to compile this.
(Reporter)

Comment 3

11 years ago
morgamic: AUS is your baby... How do you want us to do this? Is Ventron's idea usable, or should we be taking a different approach here?
(Assignee)

Comment 4

11 years ago
To make things clearer: with the patch %OSVERSION% will put out something like:

2.2.14-generic (GTK 2.12.0)

where the first number is the kernel version, which is what we send by itself without the patch. If you want a different string format please tell me and I will Make It So.
(Assignee)

Updated

11 years ago
Attachment #303994 - Flags: superreview?(benjamin)
Attachment #303994 - Flags: review?(benjamin)
(Reporter)

Comment 5

11 years ago
Comment on attachment 303994 [details] [diff] [review]
Patch

rob_strong would be good to look at this, too.
Attachment #303994 - Flags: review?(robert.bugzilla)
Flags: blocking1.8.1.13?
Flags: blocking-firefox3?
Flags: blocking-firefox3+
The patch would work but we'd need a spinoff for making the OSVERSION check rely on a regex match (much like what we did with version->branch mappings last year.

Updated

11 years ago
Attachment #303994 - Flags: superreview?(benjamin)
Attachment #303994 - Flags: superreview+
Attachment #303994 - Flags: review?(benjamin)
Attachment #303994 - Flags: review+
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.13+
Comment on attachment 303994 [details] [diff] [review]
Patch

>+    char gtkver[15];
>+    sprintf(gtkver, "GTK %d.%d.%d", gtk_major_version, gtk_minor_version, gtk_micro_version);

I'm concerned about the use of sprintf here. Even if provably safe
(is it?) its mere presence will continuously trip "check for buffer overrun"
scanners. Please use the NSPR versions that check length (PR_snprintf, or PR_smprintf).
Attachment #303994 - Flags: superreview+ → superreview-
(Assignee)

Comment 8

11 years ago
Created attachment 305073 [details] [diff] [review]
Patch 2

Use PR_snprintf.
Attachment #303994 - Attachment is obsolete: true
Attachment #303997 - Attachment is obsolete: true
Attachment #305073 - Flags: review?(robert.bugzilla)
Attachment #303994 - Flags: review?(robert.bugzilla)
(Reporter)

Updated

11 years ago
Attachment #305073 - Flags: superreview?(dveditz)
(Reporter)

Comment 9

11 years ago
Setting this to P2, as it blocks final but doesn't block b4. I hope this is the right priority for that categorization...
Priority: -- → P2
Comment on attachment 305073 [details] [diff] [review]
Patch 2

sr=dveditz -- Thanks!

Carrying over r+ from the first patch -- the two patches are identical except for the one minor change I requested.
Attachment #305073 - Flags: superreview?(dveditz)
Attachment #305073 - Flags: superreview+
Attachment #305073 - Flags: review?(robert.bugzilla)
Attachment #305073 - Flags: review+
(Reporter)

Comment 11

11 years ago
I'll land this when the tree reopens after b4.
(Assignee)

Comment 12

10 years ago
Reed, I got an email saying we need a branch version of this patch either now or Real Soon Now as this is a blocker. Can you attach one and request approval on it?
(Reporter)

Comment 13

10 years ago
Created attachment 306437 [details] [diff] [review]
1.8 branch version of patch - v2
Attachment #306437 - Flags: superreview+
Attachment #306437 - Flags: review+
Attachment #306437 - Flags: approval1.8.1.13?
Comment on attachment 306437 [details] [diff] [review]
1.8 branch version of patch - v2

approved for 1.8.1.13, a=dveditz
Attachment #306437 - Flags: approval1.8.1.13? → approval1.8.1.13+
Comment on attachment 306437 [details] [diff] [review]
1.8 branch version of patch - v2

Oh, this is not on trunk yet. removing branch approval, we don't want to be the guinea pigs for this since we don't have many nightly testers.
Attachment #306437 - Flags: approval1.8.1.13+
In fact I'm not sure this really is a blocker for 1.8.1.13 -- we won't be doing any major update until _after_ FF3 ships, we will never have a major update to a beta build.

Please land on trunk as soon as the tree opens after b4--we need the testing--and then we'll land as appropriate on the branch after that.
Flags: blocking1.8.1.13+ → blocking1.8.1.13?
Not blocking '13, hopefully by time we're working on the next release this will have landed on trunk for some testing.
Flags: blocking1.8.1.14?
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.13-
(Reporter)

Updated

10 years ago
Keywords: checkin-needed
(Reporter)

Comment 18

10 years ago
Checking in xpcom/base/Makefile.in;
/cvsroot/mozilla/xpcom/base/Makefile.in,v  <--  Makefile.in
new revision: 1.81; previous revision: 1.80
done
Checking in xpcom/base/nsSystemInfo.cpp;
/cvsroot/mozilla/xpcom/base/nsSystemInfo.cpp,v  <--  nsSystemInfo.cpp
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpdateService.js.in
new revision: 1.147; previous revision: 1.146
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
(Reporter)

Comment 19

10 years ago
Backed out. This fails to compile on debug machines.

../base/libxpcombase_s.a(nsSystemInfo.o): In function `nsSystemInfo::Init()':
/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-53.1.13.el5_Depend/mozilla/xpcom/base/nsSystemInfo.cpp:86: undefined reference to `gtk_micro_version'
/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-53.1.13.el5_Depend/mozilla/xpcom/base/nsSystemInfo.cpp:86: undefined reference to `gtk_minor_version'
/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-53.1.13.el5_Depend/mozilla/xpcom/base/nsSystemInfo.cpp:86: undefined reference to `gtk_major_version'
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

10 years ago
Target Milestone: Firefox 3 → Firefox 3 beta5
(Assignee)

Comment 20

10 years ago
Created attachment 307446 [details] [diff] [review]
Patch 3

Turns out I needed to move the LDOPTS line to a different Makefile. Ah, build system, don't ever change :/
Attachment #305073 - Attachment is obsolete: true
Attachment #306437 - Attachment is obsolete: true
(Assignee)

Comment 21

10 years ago
(In reply to comment #20)
> Created an attachment (id=307446) [details]
> Patch 3
> 
> Turns out I needed to move the LDOPTS line to a different Makefile. Ah, build
> system, don't ever change :/
> 

So I don't wonder why this breaks again, I'll record what happens.

Opt builds get built as libxul builds, debug builds do not. Therefore, in opt builds, xpcom/base is built as a static library, whereas in debug it is built as a shared library. LDOPTS can go in the xpcom/base Makefile when it is a static library, but not a shared library. For that case, the LDOPTS must go in the xpcom/build Makefile.

Lovely.

Comment 22

10 years ago
Comment on attachment 307446 [details] [diff] [review]
Patch 3


>+    char gtkver[15];
>+    PR_snprintf(gtkver, sizeof(gtkver), "GTK %d.%d.%d", gtk_major_version, gtk_minor_version, gtk_micro_version);

gtk_*_version are guint, so in the very pathological case it could be 10 chars wide (G_MAXUINT is 4294967295 on 32bit system). So the buffer may be too short. Otherwise, it should use %u (unsigned).

Comment 23

10 years ago
here's an alternate block for nsSystemInfo::Init after the PR_GetSystemInfo block, the advantage is that if you want your widget to add a thing, you can, and if i want to add a thing, i can. (This code was stolen from the directory service provider.)

#define SYSTEMINFO_CATEGORY "system-info"
nsCOMPtr<nsICategoryManager> catman(do_GetService(NS_CATEGORYMANAGER_CONTRACTID));
if (!catman)
    return NS_OK;
nsCOMPtr<nsISimpleEnumerator> entries;
catman->EnumerateCategory(SYSTEMINFO_CATEGORY,
                          getter_AddRefs(entries));
nsCOMPtr<nsIUTF8StringEnumerator> strings(do_QueryInterface(entries));
if (!strings)
    return NS_OK;
PRBool more;
while (NS_SUCCEEDED(strings->HasMore(&more)) && more) {
    nsCAutoString entry;
    strings->GetNext(entry);
    nsXPIDLCString contractID;
    catman->GetCategoryEntry(SYSTEMINFO_CATEGORY, entry.get(), getter_Copies(contractID));
    if (contractID) {
        nsCOMPtr<nsISupportsCString> value = do_GetService(contractID.get());
        if (value) {
            nsCAutoString data;
            value->GetData(data);
            SetPropertyAsACString(NS_ConvertUTF8toUTF16(entry), data);
        }
    }
}
(Assignee)

Updated

10 years ago
Attachment #307446 - Attachment is obsolete: true
(Assignee)

Comment 24

10 years ago
Created attachment 307609 [details] [diff] [review]
Patch 4

Sorry timeless, your approach won't work since there is no part of widget that loads before xpcom/base. That code is not an observer, everything must be set before it is called.

However, I found a good way of doing this without forcing XPCOM to depend on GTK directly. We load the libraries dynamically (using standard filenames that I know Firefox will use directly later on because of pkg-config) and find the symbol of the version from that.
Attachment #307609 - Flags: superreview?(benjamin)
Attachment #307609 - Flags: review?(benjamin)
(Assignee)

Comment 25

10 years ago
Created attachment 307885 [details] [diff] [review]
Patch 5

Totally get rid of our static dependence on GTK, both at runtime and compile time.
Attachment #307609 - Attachment is obsolete: true
Attachment #307885 - Flags: superreview?(benjamin)
Attachment #307885 - Flags: review?(benjamin)
Attachment #307609 - Flags: superreview?(benjamin)
Attachment #307609 - Flags: review?(benjamin)

Comment 26

10 years ago
Comment on attachment 307885 [details] [diff] [review]
Patch 5

I do not think there is a good reason to avoid the GTK dependency here. MOZ_WIDGET_GTK2 is set; let's just link to GTK directly.
Attachment #307885 - Flags: superreview?(benjamin)
Attachment #307885 - Flags: superreview-
Attachment #307885 - Flags: review?(benjamin)
Attachment #307885 - Flags: review-
(Assignee)

Updated

10 years ago
Attachment #307885 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #307446 - Attachment is obsolete: false
(Assignee)

Comment 27

10 years ago
Patch 3 contains the bustage fix from patch 2 (which is the only change, so I'm going to assume r+).
Keywords: checkin-needed
(Reporter)

Comment 28

10 years ago
I'm going to wait and land this during the daytime since I don't trust 100% that AUS will play well, but if you want to attach a patch that addresses comment #22 for check-in, that would be nice for recording purposes. Otherwise, I'll deal with it on check-in later today.
+    PR_snprintf(gtkver, sizeof(gtkver), "GTK %u.%u.%u", *gtkMajor, *gtkMinor, *gtkMicro);

why not PR_smprintf, then you don't have to worry about buffer sizes.
(Reporter)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Comment 30

10 years ago
Created attachment 309029 [details] [diff] [review]
Patch 6

This is just s/PR_snprintf/PR_smprintf so that length is no longer an issue. Its a trivial change with no change in functionality, so I doubt this needs another review.

Reed, I think AUS ignores the OS version variable anyway right now, at least on Linux. Currently we send the kernel version so do you really think AUS has a partial update for every kernel? Even if I'm wrong (but I'm pretty confident in this case), the trunk was designed to break often so at least we can find out. :)
Attachment #307446 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Reporter)

Comment 31

10 years ago
Checking in xpcom/base/Makefile.in;
/cvsroot/mozilla/xpcom/base/Makefile.in,v  <--  Makefile.in
new revision: 1.89; previous revision: 1.88
done
Checking in xpcom/base/nsSystemInfo.cpp;
/cvsroot/mozilla/xpcom/base/nsSystemInfo.cpp,v  <--  nsSystemInfo.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in xpcom/build/Makefile.in;
/cvsroot/mozilla/xpcom/build/Makefile.in,v  <--  Makefile.in
new revision: 1.104; previous revision: 1.103
done
Checking in toolkit/mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpdateService.js.in
new revision: 1.150; previous revision: 1.149
done
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 32

10 years ago
Reed, can you make a branch version of the patch please?
(Reporter)

Comment 33

10 years ago
Created attachment 309810 [details] [diff] [review]
1.8 branch version of patch - v3

This simply takes attachment 309029 [details] [diff] [review] (Patch 6) and applies it to the branch. There are no differences between trunk and branch except for some fuzz and whitespacing.
Attachment #309810 - Flags: approval1.8.1.14?
Attachment #309810 - Flags: approval1.8.1.13?
Attachment #309810 - Flags: approval1.8.1.13? → approval1.8.1.13-
Flags: blocking1.8.1.14? → blocking1.8.1.14+
Comment on attachment 309810 [details] [diff] [review]
1.8 branch version of patch - v3

this patch has not been reviewed, based on a final trunk patch without review. It's probably OK, but there's at least two steps of change since the last review so we'd like a sanity-check, please, before requesting approval.
Attachment #309810 - Flags: approval1.8.1.14?
(Reporter)

Comment 35

10 years ago
Comment on attachment 309810 [details] [diff] [review]
1.8 branch version of patch - v3

This is really the same as trunk (v6) but just with some fuzz or something fixed so it applies correctly. There were no code changes from the trunk to the branch patch. The trunk patch that landed is just a slightly modified version of one of the older patches with some build stuff fixed. However, as you request, I will get review again just to have it be less confusing for you.
Attachment #309810 - Flags: superreview?(benjamin)
Attachment #309810 - Flags: review?(benjamin)

Updated

10 years ago
Attachment #309810 - Flags: superreview?(benjamin)
Attachment #309810 - Flags: superreview+
Attachment #309810 - Flags: review?(benjamin)
Attachment #309810 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #309810 - Flags: approval1.8.1.14?
(Assignee)

Comment 36

10 years ago
Comment on attachment 309810 [details] [diff] [review]
1.8 branch version of patch - v3

Oh wait, this is blocking, is the branch the same as trunk where blocking patches don't need approval?
Attachment #309810 - Flags: approval1.8.1.14?
(Reporter)

Comment 37

10 years ago
Comment on attachment 309810 [details] [diff] [review]
1.8 branch version of patch - v3

Branch patches need approval.
Attachment #309810 - Flags: approval1.8.1.14?
Comment on attachment 309810 [details] [diff] [review]
1.8 branch version of patch - v3

Dan's going to review this as well before approving.
Attachment #309810 - Flags: review?(dveditz)
Just to double-check, this does not break current AUS functionality right? If
we need an emergency firedrill re-release of 2.0.0.14 the day after this lands
we're not going to get told "oops, haven't updated AUS yet" are we? There was
talk in one of the comments about ignoring this new information until AUS is
upgraded to look at it -- has that been confirmed by testing?
(Reporter)

Comment 40

10 years ago
(In reply to comment #38)
> (From update of attachment 309810 [details] [diff] [review])
> Dan's going to review this as well before approving.

It's been reviewed by bsmedberg, who is the module owner for both XPCOM and Toolkit, which are the only two modules this patch touches. Not sure why another review is needed here.

(In reply to comment #39)
> Just to double-check, this does not break current AUS functionality right?

No, it does not. If it had, all nightly users on Linux would have been screwed when this landed. The AUS field (platformVersion) this patch is (ab)using to include the GTK+ version isn't really used for anything besides unsupported platform checking and bug 360127 issues, so it's not a problem. However, AUS will need to be changed to support the checking of the GTK+ version before the eventual Firefox 3 major update, of course.

Comment 41

10 years ago
I don't mind an extra review, especially for branch-bound patches; certainly nothing to get one's shorts in a knot about.
Comment on attachment 309810 [details] [diff] [review]
1.8 branch version of patch - v3

>-  try {
>-    var sysInfo = 
>-      Components.classes["@mozilla.org/system-info;1"]
>-                .getService(Components.interfaces.nsIPropertyBag2);
>+  var osVersion;
>+  var sysInfo = Components.classes["@mozilla.org/system-info;1"]
>+                          .getService(Components.interfaces.nsIPropertyBag2);

Not sure why you moved the getService out of the try{} block with the var -- it could throw, right? I guess things would have to be pretty screwed up for that, and this file is already pretty mixed on whether it wants to use try {} on getService.

Thanks reed, for the reassurance about AUS.

approved for 1.8.1.14, a=dveditz for release-drivers
Attachment #309810 - Flags: review?(dveditz)
Attachment #309810 - Flags: review+
Attachment #309810 - Flags: approval1.8.1.14?
Attachment #309810 - Flags: approval1.8.1.14+
(Reporter)

Updated

10 years ago
Keywords: checkin-needed
(Reporter)

Comment 43

10 years ago
MOZILLA_1_8_BRANCH:

Checking in xpcom/base/Makefile.in;
/cvsroot/mozilla/xpcom/base/Makefile.in,v  <--  Makefile.in
new revision: 1.67.2.4; previous revision: 1.67.2.3
done
Checking in xpcom/base/nsSystemInfo.cpp;
/cvsroot/mozilla/xpcom/base/nsSystemInfo.cpp,v  <--  nsSystemInfo.cpp
new revision: 1.7.2.1; previous revision: 1.7
done
Checking in xpcom/build/Makefile.in;
/cvsroot/mozilla/xpcom/build/Makefile.in,v  <--  Makefile.in
new revision: 1.90.2.3; previous revision: 1.90.2.2
done
Checking in toolkit/mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpdateService.js.in
new revision: 1.77.2.51; previous revision: 1.77.2.50
done
Keywords: checkin-needed → fixed1.8.1.14
Verified on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.15pre) Gecko/2008061014 BonEcho/2.0.0.15pre

Comparing the requests using Fx20014 and Fx20015pre:

GET /update/2/Firefox/2.0.0.14/2008040414/Linux_x86-gcc3/en-US/release/Linux%202.6.24-16-generic/update.xml?force=1

GET /update/2/Firefox/2.0.0.15pre/2008061014/Linux_x86-gcc3/en-US/nightly/Linux%202.6.24-16-generic%20(GTK%202.12.9)/update.xml?force=1

Keywords: fixed1.8.1.15 → verified1.8.1.15
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.