Closed Bug 1059424 Opened 10 years ago Closed 7 years ago

[10.10] WARNING: The Gestalt selector gestaltSystemVersion is returning 10.9.0 instead of 10.10.0. Use NSProcessInfo's operatingSystemVersion property to get correct system version number.

Categories

(Core :: Widget: Cocoa, defect, P3)

Product:
Core
Component:
Widget: Cocoa
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: Gijs, Assigned: bradwerth)

References

Details

(Whiteboard: tpi:+)

Attachments

(3 files)

Bug 1059424 Part 1: Simplify cairo quartz surface generation now that we are minimum macOS 10.9.
59 bytes, text/x-review-board-request
mstange
: review+
jrmuizel
: review+
Details
Bug 1059424 Part 2: Simplify nsAppFileLocationProvider Java plugin detection now that we are minimum macOS 10.9.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1059424 Part 3: Simplify DynamicImages::GetDyldAllImageInfosPointer now that we are minimum macOS 10.9.
59 bytes, text/x-review-board-request
mstange
: review+
Details 
See also: bug 772179 comment 32.

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsNativeAppSupportCocoa.mm#79

still uses gestalt (and runs on startup, leading to warnings in the console on OS X 10.10), as do a bundle of other ones:

http://mxr.mozilla.org/mozilla-central/search?string=gestalt

I expect our own consumers can probably be updated to use nsCocoaFeatures::OSXVersion() (and potentially that can be updated to use NSProcessInfo's operatingSystemVersion property ?), while updating ipc, webrtc, breakpad and cairo might be more work.
The operatingSystemVersion property was introduced in Yosemite :-(

I also understand (from bug 1021309 comment #6) that Apple now recommends using
- (BOOL)isOperatingSystemAtLeastVersion:(NSOperatingSystemVersion)version

But that, too, was introduced in Yosemite.

So Apple now (finally) does have an official replacement for Gestalt, but it'll be years before we can rely on it.

In the meantime the code currently in nsCocoaFeatures.mm's GetSystemVersion() should work just fine.
(In reply to :Gijs Kruitbosch from comment #0)
> See also: bug 772179 comment 32.
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/
> nsNativeAppSupportCocoa.mm#79
> 
> still uses gestalt (and runs on startup, leading to warnings in the console
> on OS X 10.10), as do a bundle of other ones:

In another bug it was mentioned, that this kind of version checks is probably redundant and can be removed:
https://bugzilla.mozilla.org/show_bug.cgi?id=782157#c3
Now, that this seems to make some problems on 10.10, can this one only be removed?
> Here are the remaining uses of Gestalt() in the tree.

I should have said:

Here are the remaining uses of Gestalt() in the tree to check the OS X version.
16.03.15 00:02:49,031 firefox[8866]: WARNING: The Gestalt selector gestaltSystemVersion is returning 10.9.2 instead of 10.10.2. Use NSProcessInfo's operatingSystemVersion property to get correct system version number.
Call location:
16.03.15 00:02:49,032 firefox[8866]: 0   CarbonCore                          0x00007fff96409d9b ___Gestalt_SystemVersion_block_invoke + 113
16.03.15 00:02:49,032 firefox[8866]: 1   libdispatch.dylib                   0x00007fff95c44c13 _dispatch_client_callout + 8
16.03.15 00:02:49,032 firefox[8866]: 2   libdispatch.dylib                   0x00007fff95c44b26 dispatch_once_f + 117
16.03.15 00:02:49,032 firefox[8866]: 3   CarbonCore                          0x00007fff963b23ca _Gestalt_SystemVersion + 987
16.03.15 00:02:49,033 firefox[8866]: 4   CarbonCore                          0x00007fff963b1fb7 Gestalt + 144
16.03.15 00:02:49,033 firefox[8866]: 5   XUL                                 0x000000010359ed71 XRE_FreeAppData + 34641

Adding another data point and giving the info this persists on OSX 10.10.2 with Firefox 36.0.1.
Getting the same:

WARNING: The Gestalt selector gestaltSystemVersion is returning 10.9.3 instead of 10.10.3. Use NSProcessInfo's operatingSystemVersion property to get correct system version number.
We know about this.  No need for more "me too" comments.  See comment #3.
Priority: -- → P3
Whiteboard: tpi"+
Whiteboard: tpi"+ → tpi:+
What's the status of this? A quick check on macOS 10.12 no longer shows this message in the console log. Has code been removed or is macOS ignoring this by now?
Assignee: nobody → bwerth
Attachment #8885028 - Flags: review?(jmuizelaar)
Attachment #8885029 - Flags: review?(jmuizelaar)
Attachment #8885030 - Flags: review?(jmuizelaar)
We actually only support 10.9 and higher since Firefox 49.
(In reply to Stefan [:stefanh] from comment #15)
> We actually only support 10.9 and higher since Firefox 49.

Well, the proposed patches just route things through the existing nsCocoaFeatures::GetSystemVersion() method. That method could be rewritten to use [NSProcessInfo operatingSystemVersion] and perhaps should be rewritten to do this. Should I add that to the patch?
Flags: needinfo?(stefanh)
(In reply to Brad Werth [:bradwerth] from comment #16)
> Well, the proposed patches just route things through the existing
> nsCocoaFeatures::GetSystemVersion() method. That method could be rewritten
> to use [NSProcessInfo operatingSystemVersion] and perhaps should be
> rewritten to do this. Should I add that to the patch?

Markus would know more, but it looks like [NSProcessInfo operatingSystemVersion] is Yosemite-only (comment #1). I was just thinking that Gecko_OnSnowLeopardOrLater() always will be true since we're always on 10.9 or higher.
Flags: needinfo?(stefanh)
(In reply to Stefan [:stefanh] from comment #17)
> I was just thinking
> that Gecko_OnSnowLeopardOrLater() always will be true since we're always on
> 10.9 or higher.

I concur.
Comment on attachment 8885028 [details]
Bug 1059424 Part 1: Simplify cairo quartz surface generation now that we are minimum macOS 10.9.

https://reviewboard.mozilla.org/r/155872/#review161406

I'd rather not introduce a cairo dependency on Gecko here. In fact even though Gestalt only returns 10.9, that should be sufficient for cairo's needs. I think it's unlikely that it will be removed from the OS.
Attachment #8885028 - Flags: review?(jmuizelaar) → review-
I agree on all points, but I would like to suppress the warning, and one of the easiest ways to do that is to stop calling Gestalt for gestaltSystemVersion. What's the preferred way to resolve this warning?
Flags: needinfo?(mstange)
(In reply to Brad Werth [:bradwerth] from comment #20)
> I agree on all points, but I would like to suppress the warning, and one of
> the easiest ways to do that is to stop calling Gestalt for
> gestaltSystemVersion. What's the preferred way to resolve this warning?

All checks that handle pre-10.9 scenarios, both in Gecko and in cairo, can be removed.
In part 1: _cairo_quartz_osx_at_least_10_6 can be set to true unconditionally.
In part 2: nsAppFileLocationProvider::IsOSXLeopard() can return false unconditionally.
In part 3: HasTaskDyldInfo() can return true unconditionally.

This will of course enable further simplifications in the callers. It's up to you how much you want to make those simplification.
Flags: needinfo?(mstange)
Attachment #8885028 - Flags: review?(jmuizelaar) → review?(mstange)
Attachment #8885029 - Flags: review?(jmuizelaar) → review?(mstange)
Attachment #8885030 - Flags: review?(jmuizelaar) → review?(mstange)
Comment on attachment 8885028 [details]
Bug 1059424 Part 1: Simplify cairo quartz surface generation now that we are minimum macOS 10.9.

https://reviewboard.mozilla.org/r/155872/#review161426
Attachment #8885028 - Flags: review+
Comment on attachment 8885028 [details]
Bug 1059424 Part 1: Simplify cairo quartz surface generation now that we are minimum macOS 10.9.

https://reviewboard.mozilla.org/r/155872/#review161450
Attachment #8885028 - Flags: review?(mstange) → review+
Comment on attachment 8885029 [details]
Bug 1059424 Part 2: Simplify nsAppFileLocationProvider Java plugin detection now that we are minimum macOS 10.9.

https://reviewboard.mozilla.org/r/155874/#review161452

::: xpcom/io/nsAppFileLocationProvider.cpp:550
(Diff revision 3)
>      // As of Java for Mac OS X 10.5 Update 10, Apple has (in effect) deprecated Java Plugin2 on
>      // on OS X 10.5, and removed the soft link to it from /Library/Internet Plug-Ins/.  Java
>      // Plugin2 is still present and usable, but there are no longer any links to it in the
>      // "normal" locations.  So we won't be able to find it unless we look in the "non-normal"
>      // location where it actually is.  Safari can use the WebKit-specific JavaPluginCocoa.bundle,
>      // which (of course) is still fully supported on OS X 10.5.  But we have no alternative to

Please file a bug on removing the rest of NS_MACOSX_JAVA2_PLUGIN_DIR and on adjusting this comment.

::: xpcom/io/nsAppFileLocationProvider.cpp:556
(Diff revision 3)
>      // using Java Plugin2.  For more information see bug 668639.
>      static const char* keys[] = {
>        NS_APP_PLUGINS_DIR,
>        NS_MACOSX_USER_PLUGIN_DIR,
>        NS_MACOSX_LOCAL_PLUGIN_DIR,
> -      IsOSXLeopard() ? NS_MACOSX_JAVA2_PLUGIN_DIR : nullptr,
> +      nullptr,

This code is using nullptr as the end-of-array marker, so you only need one nullptr here, not two.
Attachment #8885029 - Flags: review?(mstange) → review+
Comment on attachment 8885030 [details]
Bug 1059424 Part 3: Simplify DynamicImages::GetDyldAllImageInfosPointer now that we are minimum macOS 10.9.

https://reviewboard.mozilla.org/r/155876/#review161454

Oh, hmm, this is breakpad code. I'm not sure what our rules are about upstreaming changes / diverging from upstream.
The patch itself looks good to me, but I'll leave the question to Ted.
Attachment #8885030 - Flags: review?(mstange) → review+
Oh, but Ted is on vacation.
The answer is here: https://groups.google.com/d/msg/mozilla.dev.platform/WmjC4U-iMPI/CP22Jcw2BQAJ
We've forked, and we can make any changes we want. So that's good news :)
Blocks: 1380165
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98c2ac392305
Part 1: Simplify cairo quartz surface generation now that we are minimum macOS 10.9. r=jrmuizel,mstange
https://hg.mozilla.org/integration/autoland/rev/088181b07296
Part 2: Simplify nsAppFileLocationProvider Java plugin detection now that we are minimum macOS 10.9. r=mstange
https://hg.mozilla.org/integration/autoland/rev/f152ee69120c
Part 3: Simplify DynamicImages::GetDyldAllImageInfosPointer now that we are minimum macOS 10.9. r=mstange
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: