772179 - 'Gestalt' is deprecated in Mac OS X 10.8

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Nomis101]

Gestalt is deprecated since Mac OS X 10.8. If I build on 10.8 with the 10.8 SDK I get over 50 warnings in the kind of:

mozilla/xpcom/base/nsStackWalk.cpp:111:19: warning: 
      'Gestalt' is deprecated: first deprecated in Mac OS X 10.8
      [-Wdeprecated-declarations]
    OSErr err = ::Gestalt(gestaltSystemVersion, (SInt32*)&gOSXVersion);
                  ^
mozilla/xpcom/base/nsStackWalk.cpp:111:11: warning: 
      unused variable 'err' [-Wunused-variable]
    OSErr err = ::Gestalt(gestaltSystemVersion, (SInt32*)&gOSXVersion);
          ^
mozilla/xpcom/base/nsStackWalk.cpp:117:13: warning: 
      unused function 'OnLionOrLater' [-Wunused-function]
static bool OnLionOrLater()
            ^



mozilla/netwerk/protocol/http/nsHttpHandler.cpp:778:12: warning: 
      'Gestalt' is deprecated: first deprecated in Mac OS X 10.8
      [-Wdeprecated-declarations]
    if ((::Gestalt(gestaltSystemVersionMajor, &majorVersion) == noErr) &&
           ^
/Developer/SDKs/MacOSX10.8.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/Gestalt.h:123:1: note: 
      'Gestalt' declared here
Gestalt(
^
mozilla/netwerk/protocol/http/nsHttpHandler.cpp:779:12: warning: 
      'Gestalt' is deprecated: first deprecated in Mac OS X 10.8
      [-Wdeprecated-declarations]
        (::Gestalt(gestaltSystemVersionMinor, &minorVersion) == noErr)) {
           ^
/Developer/SDKs/MacOSX10.8.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/Gestalt.h:123:1: note: 
      'Gestalt' declared here
Gestalt(
^



mozilla/widget/cocoa/nsCocoaFeatures.mm:60:23: warning: 
      'Gestalt' is deprecated: first deprecated in Mac OS X 10.8
      [-Wdeprecated-declarations]
        OSErr err = ::Gestalt(gestaltSystemVersion, ...
                      ^
1 warning generated.



mozilla/toolkit/xre/nsNativeAppSupportCocoa.mm:117:17: warning: 
      'Gestalt' is deprecated: first deprecated in Mac OS X 10.8
      [-Wdeprecated-declarations]
  OSErr err = ::Gestalt (gestaltSystemVersion, &response);
                ^
3 warnings generated.

Comment 1

[André Reinald]

Attachment: Changed method of discovering mac os version

Comment 2

[André Reinald]

Comment on attachment 722210 [details] [diff] [review]
Changed method of discovering mac os version

Steven, the author of the bit of code I changed is not a bugzilla user, so I turned to you.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 722210 [details] [diff] [review]
Changed method of discovering mac os version

I think the basic approach is fine, but we shouldn't assume major versions greater than 8 are still 8.  In other words, I think you should drop the following from your patch:

+    else if (minor > 8) {
+      NS_ERROR("OS X version too new, assuming 10.8");
+      mOSXVersion = MAC_OS_X_VERSION_10_8_HEX;
+    }

Dealing with this bug is complicated by the fact that, as best I can tell, Apple doesn't have any official replacement for Gestalt!  That is a good reason to simply postpone finding any replacement for Gestalt ourselves.

But /System/Library/CoreServices/SystemVersion.plist has been around for a long time, and parts of Apple's UI have (unofficially at least) for a long time used it to find out what OS version is running.  (On OS X Server there's also a /System/Library/CoreServices/ServerVersion.plist, which uses the same syntax, and whose presence tells the same parts of Apple's UI that "server" is running instead of "client".)  So though your patch (apparently) relies on undocumented information, I think it's probably quite safe.

When I looked at this bug a few months ago, I was able to find an Apple doc that said we should use sysctl() as a replacement for Gestalt.  But I can no longer find it, and in any case there are problems using sysctl():

This isn't because of any problems with sysctl() itself.  Instead the version information read by sysctl() (and put in place by Apple) doesn't always match the "official" version number.  For example if you use "uname -a" on OS X 10.7.5 (which reads the same information read by systcl()), you get version 11.4.2, not 11.5.[n] as you should.

By the way, how did you decide to use /System/Library/CoreServices/SystemVersion.plist?  And why didn't you tell us? :-)

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 722210 [details] [diff] [review]
Changed method of discovering mac os version

Another thing:

+  if ( versions.count >= 1 ) {
+    major = [versions[0] integerValue];
+    if ( versions.count >= 2 ) {
+      minor = [versions[1] integerValue];
+      if ( versions.count >= 3 ) {
+        bugfix = [versions[2] integerValue];
+      }
+    }
+  }

I'm not sure the syntax you use here (versions.count and versions[n]) is supported on older versions of OS X.  You should do a tryserver build to check.

Or now that I think of it, last I knew you still didn't have permission to use the tryservers, so I'll do a build myself.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 722210 [details] [diff] [review]
Changed method of discovering mac os version

And yet another thing:

+    else if (minor < 5) {
+      NS_ERROR("OS X version too old, assuming 10.5");

The oldest version of OS X we currently support is 10.6.  So if in doubt you should assume 10.6, not 10.5.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 722210 [details] [diff] [review]
Changed method of discovering mac os version

Note that if/when Apple does deign to document an official replacement for Gestalt (and if it's not what what this patch does), we'll have to revisit this bug, and possibly change what we've done here.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 722210 [details] [diff] [review]
Changed method of discovering mac os version

Also please make the styling (e.g. the size of the indents) match what's already in this file.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #4)

This syntax doesn't work even on OS X 10.8.2 with a recent version of XCode -- it failed when I tried to build locally.  You'll need to change it.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #8)

I find the following syntax works for me:

    NSUInteger count = [versions count];
    if (count >= 1) {
        major = [(NSString *)[versions objectAtIndex:0] integerValue];
        if (count >= 2) {
            minor = [(NSString *)[versions objectAtIndex:1] integerValue];
            if (count >= 3) {
                bugfix = [(NSString *)[versions objectAtIndex:2] integerValue];
            }
        }
    }

Comment 10

[André Reinald]

Attachment: Use a system plist instead of deprecated Gestalt (since 10.8)

Changed the patch to take into account Steven's comments. Actually I reformatted the whole source file (which is very short) to match our current "coding style" (no tabs, 2 space indent).

Comment 11

[Stephen A Pohl [:spohl]]

(In reply to André Reinald from comment #10)
> Changed the patch to take into account Steven's comments. Actually I
> reformatted the whole source file (which is very short) to match our current
> "coding style" (no tabs, 2 space indent).

Ah, I'd like to know more about this. Is this encouraged, or should we avoid doing this because it messes up hg blame?

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> I think the basic approach is fine, but we shouldn't assume major
> versions greater than 8 are still 8.  In other words, I think you
> should drop the following from your patch:
>
> +    else if (minor > 8) {
> +      NS_ERROR("OS X version too new, assuming 10.8");
> +      mOSXVersion = MAC_OS_X_VERSION_10_8_HEX;
> +    }

On reflection you should also change this

+    else {
+      int translated[] = {
+        MAC_OS_X_VERSION_10_5_HEX,
+        MAC_OS_X_VERSION_10_6_HEX,
+        MAC_OS_X_VERSION_10_7_HEX,
+        MAC_OS_X_VERSION_10_8_HEX
+      };
+      mOSXVersion = translated[minor - 5];
+    }

to this

    else {
            mOSXVersion = 0x1000 + (minor << 4);
    }

This may all need to change when OS X hits version 10.  But knowing
Apple, we'll just have to wait to see what they do when the time
comes.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

>> Changed the patch to take into account Steven's comments. Actually
>> I reformatted the whole source file (which is very short) to match
>> our current "coding style" (no tabs, 2 space indent).
>
> Ah, I'd like to know more about this. Is this encouraged, or should
> we avoid doing this because it messes up hg blame?

I *really* *don't* like it :-)

For the reason you gave -- that it messes up hg blame.

Sometimes a file has inconsistent style.  In those cases I may change
the style of part of it -- but only those parts my patch would have
touched anyway.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

André, you need to change you patch's syntax to match comment #9.  Also please make the change I recommend in comment #12.

As for reformatting the whole file, I could go either way -- the file's very short.

But in general I really hate style-only changes in a substantive patch.

Comment 15

[André Reinald]

Attachment: Changed method of discovering mac os version

Previous patch was already avoiding dotted property syntax. Wasn't it ? I changed to int to NSUInteger also. And adjusted formatting to the rest of the source file.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

> versions[0]

This kind of syntax is also new, and I'm not sure it'd work building on (say) OS X 10.6, with older versions of XCode.

Comment 17

[André Reinald]

Ok, let me change that... then I have to leave, it's 9 pm here :-)

Comment 18

[André Reinald]

Attachment: Changed method of discovering mac os version

Replaced versions[index] with [versions objectAtIndex:index]
Sorry I didn't notice that difference in your snippet.

Comment 19

[Jorge Luis Mendez [:jlmendezbonini]]

(In reply to Steven Michaud from comment #16)
> > versions[0]
> 
> This kind of syntax is also new, and I'm not sure it'd work building on
> (say) OS X 10.6, with older versions of XCode.

http://developer.apple.com/library/ios/#releasenotes/ObjectiveC/ObjCAvailabilityIndex/index.html

Looks like it requires Xcode4.4 and deploys back to 10.6

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> Looks like it requires Xcode4.4 and deploys back to 10.6

XCode 4.4 is still very recent.

Perhaps I'm being too conservative.  But our lives are complicated enough already without increasing the complication for no compelling reason.

Yes, I suppose the dot-property and subscript notations are simpler and friendlier.  But they're a drop in the ocean, and the older syntax they replace is only very moderately less friendly.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 722416 [details] [diff] [review]
Changed method of discovering mac os version

Just one last, little change is needed.  And there's no problem letting it wait for tomorrow.

>+        major = [[versions objectAtIndex:0] integerValue];

This should really be

+        major = [(NSString*)[versions objectAtIndex:0] integerValue];

An NSArray holds NSObject objects, but integerValue is an NSString method.

It's possible your compiler doesn't complain about this.  But it really *should* :-)

Comment 22

[André Reinald]

Attachment: Changed method of discovering mac os version

The compiler may emit a warning because we call a method on an untyped ObjC object, I didn't check compiler output for warnings, as there are already so many!

I corrected with a twist: as the expression became quite long and was repeated 3 times, I factorized into a single function. So if you want other changes I'll only have to change once :-)

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 722709 [details] [diff] [review]
Changed method of discovering mac os version

Looks fine to me.

Thanks for your patch.  Thanks particularly for the idea of using SystemVersion.plist.  You still haven't told us how you decided to do this :-)

Comment 24

[Steven Michaud [:smichaud] (Retired)]

By the way, I don't know if you yet have permissions to land patches on the tree.  If so I'll just land this myself (on mozilla-inbound).

Comment 25

[André Reinald]

I thought I wrote where I found the trick, though they disagree with it:
http://cocoadev.com/wiki/DeterminingOSVersion

And I don't have permission to land patches in mozilla-inbound, which is fine to me, so I have less responsibility :-)

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 722709 [details] [diff] [review]
Changed method of discovering mac os version

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/495fb4bf05f8

> http://cocoadev.com/wiki/DeterminingOSVersion

Thanks for the info.

Interesting that this doc doesn't mention sysctl at all.  Also interesting that "Apple recommended at the 2012 WWDC" to use SystemVersion.plist.  So that may actually become the official replacement for Gestalt.

But I suspect there will be a few more twists and turns before Apple finally gets its act together on this issue :-(

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/495fb4bf05f8

Comment 28

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #3)

> When I looked at this bug a few months ago, I was able to find an
> Apple doc that said we should use sysctl() as a replacement for
> Gestalt.  But I can no longer find it ...

I started to wonder if Apple might have suppressed this doc.  But no,
I've now looked more carefully and found it again:

http://developer.apple.com/library/mac/#releasenotes/General/CarbonCoreDeprecations/index.html#//apple_ref/doc/uid/TP40012224-CH1-SW16

Comment 29

[André Reinald]

(In reply to Steven Michaud from comment #28)
> (Following up comment #3)
> I started to wonder if Apple might have suppressed this doc.  But no,
> I've now looked more carefully and found it again:
> 
> http://developer.apple.com/library/mac/#releasenotes/General/
> CarbonCoreDeprecations/index.html#//apple_ref/doc/uid/TP40012224-CH1-SW16

And I guess you'd rather call sysctl than read the info from the plist :-)
I can submit another patch, that won't be long to write.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

No, Andre, you don't understand why I commented.

I strongly prefer your patch to using sysctl, for the reasons I gave in comment #3.

I just wanted to show a complete history to-the-present of Apple's bumbling on this issue.

Comment 31

[Nomis101]

Do we need this kind of fix for all Components in which we are using 'Gestalt' (xpcom, netwerk, ipc, media, gfx, dom, toolkit)?

Comment 32

[Steven Michaud [:smichaud] (Retired)]

What we really need is for someone make all our OS X version queries use nsCocoaFeatures::OSXVersion().

This isn't urgent.  But then neither is it urgent to stop using Gestalt.

Apple has botched the transition away from Gestalt very badly.  They can't possibly get rid of it until well after they have an official replacement -- which they still don't have.  (By an official replacement I mean one that's documented in some kind of footnote to the original Gestalt documentation.)

Nonetheless, it'd still be nice to see someone clean up our OS X version checking code.  Interested? :-)

Comment 33

[Jean-Yves Avenard [:jya]]

Looking into how to retrieve the version number, I fell on this bug... Accessing /System/Library/CoreServices/SystemVersion.plist isn't possible if the application is sandboxed.. And sooner or later, we'll have to be

Comment 34

[Josiah Bruner [:jsbruner]]

Unfortunately my Mac Developer Program expired, so I can't link to the Apple docs, but [[NSProcessInfo processInfo] operatingSystemVersion] should return a NSOperatingSystemVersion struct which contains three NSIntegers. majorVersion, minorVersion, and patchVersion.

This is supposedly the "official" replacement we've been waiting for, but it's only available on 10.10.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

> Accessing /System/Library/CoreServices/SystemVersion.plist isn't possible if the application is sandboxed.

Until all supported versions of OS X support Apple's official method to get the OS version, we'll have to add a rule to the sandbox ruleset to allow this access.  And if we sandbox NPAPI plugins, we'll probably also need to add exceptions to allow all kinds of other, unofficial ways to determine the OS version.