1122181 - Stop using ::Gestalt in XPCOM

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

There are just two places in XPCOM that use Gestalt - and one of them isn't necessary as we are only using it to see if we're on 10.5, which we don't support anymore.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Stop using Gestalt for version in XPCOM.

Steven, you were involved in the first crack at the whole Gestalt thing, so I thought I'd run this by you first.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Stop using Gestalt for version in XPCOM.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8549821 [details] [diff] [review]
Stop using Gestalt for version in XPCOM.

This looks fine to me.  Thanks for the patch.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60d61fe2fb21

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Stop using Gestalt for version in XPCOM. Carry r=smichaud

Rebase

Comment 6

[Carsten Book [:Tomcat]]

Hi Milan, the try run shows a bustage could you take a look at this ?

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Carsten Book [:Tomcat] from comment #6)
> Hi Milan, the try run shows a bustage could you take a look at this ?

That's plain strange, I was convinced I looked and saw it green.  Especially as https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a672195c672 (opt, mind you) which also included this change set was.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

My bad; we don't seem to be able to resolve functions from widget/ in xpcom/ at a first glance:

09:55:42     INFO -  Undefined symbols for architecture x86_64:
09:55:42     INFO -    "nsCocoaFeatures::OnLionOrLater()", referenced from:
09:55:42     INFO -        StackWalkInitCriticalAddress() in nsStackWalk.o

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Here's the problem.  (And yes, I also missed it.)

-  MOZ_ASSERT(OnLionOrLater() || gCriticalAddress.mAddr != nullptr);
+  MOZ_ASSERT(nsCocoaFeatures::OnLionOrLater() || gCriticalAddress.mAddr != nullptr);

You need an "#if defined(MOZ_WIDGET_COCOA)" block around this.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #9)

Oops, that's not right.

Instead try getting rid of this:

+#if defined (DEBUG) && defined(MOZ_WIDGET_COCOA)
+#include "nsCocoaFeatures.h"
+#endif

and adding this here:

#include "nsCocoaFeatures.h"

http://hg.mozilla.org/mozilla-central/annotate/c2df1daf74c3/xpcom/base/nsStackWalk.cpp#l53

Do an all platform tryserver build to test it out.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Stop using Gestalt for version in XPCOM. Carry r=smichaud

Fix the includes.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Still a problem (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a6d653155d1).  Regarding comment 10, if it was "just" an include problem, wouldn't we have a compile time, rather than a link time error?  I wonder if xpcom/base doesn't link "to enough", compared to xpcom/io or xpcom/components which use nsCocoaFeatures and don't have this problem?

Comment 13

[Andrew McCreight [:mccr8]]

It looks like the link problem is with libdmd.  Maybe it builds its own copy of nsStackWalk?  Glandium may have some ideas.

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #13)
> It looks like the link problem is with libdmd.  Maybe it builds its own copy
> of nsStackWalk?

It does. See https://dxr.mozilla.org/mozilla-central/source/memory/replace/dmd/moz.build#16

Also note that libdmd is built for debug builds on the test machines but not for opt builds. So that explains why opt builds are succeeding.

Is there some library you have to link with in order to use nsCocoaFeatures::OnLionOrLater()?

Comment 15

[Nicholas Nethercote [inactive]]

To replicate this locally, add |ac_add_options --enable-dmd| to your mozconfig.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Milan, I think you should get rid of the part of your patch that imports nsCocoaFeatures::OnLionOrLater().  Just keep the part that eliminates the unnecessary OS X 10.5 version check, which (of course) is no longer needed.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Oops, sorry.  I didn't really mean for you to keep the call to Gestalt().  Instead copy over from nsCocoaFeatures.mm as much code as you need to do the OnLionOrLater() version check.  Note that you can do this without using any Objective-C syntax (which would be forbidden in a *.c/cpp file).

Comment 18

[Nomis101]

There is no ::Gestalt anymore in XPCOM. The last one was removed in Bug 1059424.