Build failure in WebHTMLView.mm with the public SDK (Xcode 11 and Mojave)

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Follow-up of https://bugs.webkit.org/show_bug.cgi?id=199279#c4

Using XCode 11.0 beta 3 and the public SDK, I get errors in the part of WebKitLegacy/mac/WebView/WebHTMLView.mm protected by SUBVIEWS_IVAR_SPI:

use of undeclared identifier '_subviews'

Not sure what is the proper API to use and why SUBVIEWS_IVAR_SPI is actually not enabled. (Removing any reference to _subviews make the build succeed, so that's the only remaining failure)

[Jonathan Bedard]

This is a bit surprising since I got a version building with the public SDK.

Once I finish verifying getting the iOS 13 build working, I'll update my seed build and try and fix this.

[Alexey Proskuryakov]

Fred clarifies in the other bug that this is about Xcode 11.0 beta 3 on Mojave, not on Catalina.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #2)
> Fred clarifies in the other bug that this is about Xcode 11.0 beta 3 on
> Mojave, not on Catalina.

Ok, that makes more sense. HAVE_SUBVIEWS_IVAR_SPI only gets defined on Catalina. Should is be defined unconditionally on public SDKs?

[Timothy Hatcher]

(In reply to Jonathan Bedard from comment #3)
> (In reply to Alexey Proskuryakov from comment #2)
> > Fred clarifies in the other bug that this is about Xcode 11.0 beta 3 on
> > Mojave, not on Catalina.
> 
> Ok, that makes more sense. HAVE_SUBVIEWS_IVAR_SPI only gets defined on
> Catalina. Should is be defined unconditionally on public SDKs?

The SPI only exists on Catalina. It would build but crash with a missing selector otherwise.

I'll need to think of a fix that will work with the older systems and newer SDK.

[Jonathan Bedard]

Talked with Alexey a bit about this last week.

None of our automation even builds with an old OS install and a new SDK, we're always either a new OS with a new SDK or an old OS with and old SDK. Because of this, I don't know that we have a compelling reason to fix this problem.

[Alexey Proskuryakov]

I think that this can't be just ignored, as contributors will certainly be installing a newer Xcode.

The challenge is that even with this build fix, there could be other things that go wrong in less obvious ways.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #6)
> I think that this can't be just ignored, as contributors will certainly be
> installing a newer Xcode.
> 
> The challenge is that even with this build fix, there could be other things
> that go wrong in less obvious ways.

Without builder and testers, I don't know that we can practically expect to keep breakage like this out of the build. If we don't have any supporting automation, I don't know that we can claim this is a configuration we intend to support.

[Frédéric Wang (:fredw)]

Hi, I think it makes sense not to build/test/support a beta version of XCode together with a release operating system, if that's adding burden.

From the user's point of view, it's a shame because it's harder to switch between building with two OS versions than between building with two XCode versions.

In any case, if that configuration is really not supported, I think the build script should explicitly fail during configuration step with a clear error message.

Incidentally, you mention that your automation builds a new OS with a new SDK but I can't find any public buildbot for Catalina/iOS13 on https://build.webkit.org/console ? Without that, how can external contributors find issues like bug 200000 comment 32 ? And to debug them, I guess they have to switch to newer Catalina / XCode ?

[Frédéric Wang (:fredw)]

This failure now happens with the Xcode 11 release on Mojave.

[Jonathan Bedard]

How does your setup compare with the iOS 13 bots?

https://build.webkit.org/buildslaves/bot651

[Frédéric Wang (:fredw)]

(In reply to Jonathan Bedard from comment #10)
> How does your setup compare with the iOS 13 bots?
> 
> https://build.webkit.org/buildslaves/bot651

I still have the same build error after a clean rebuild. I have macOS 10.14.6 but only Xcode 11.0 11A420a (while buildbot is Xcode 11.1 Build version 11A1027). I noticed a new system update today, not sure that would actually help here but I'm downloading it now.

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #11)
> I still have the same build error after a clean rebuild. I have macOS
> 10.14.6 but only Xcode 11.0 11A420a (while buildbot is Xcode 11.1 Build
> version 11A1027). I noticed a new system update today, not sure that would
> actually help here but I'm downloading it now.

System upgrade didn't help. I didn't get Xcode 11.1 from the standard software update so I guess I'll just install it from developer.apple.com

[Yury Semikhatsky]

(In reply to Frédéric Wang (:fredw) from comment #8)
> In any case, if that configuration is really not supported, I think the
> build script should explicitly fail during configuration step with a clear
> error message.
> 
+1 for this. XCode 11 is what is suggested by default for download from Apple's site at the moment and it's so frustrating that a clean build will fail on Mac OS X 10.14.

I assume the workaround for those who want to develop on OS X 10.14 is to downgrade to XCode 10.

[cathiechen]

I tried to clean build with Xcode 11.1 on Mac 10.14.6, it also has the same error.

[Yury Semikhatsky]

(In reply to cathiechen from comment #14)
> I tried to clean build with Xcode 11.1 on Mac 10.14.6, it also has the same
> error.

If you just need to build WebKit on Mac OS X 10.14.6 you can download XCode 10.3 XCode page on developer.apple.com doesn't seem to have other versions of XCode except 11.1. This direct link still works though: https://developer.apple.com/services-account/download?path=/Developer_Tools/Xcode_10.3/Xcode_10.3.xip

(In reply to Timothy Hatcher from comment #4)
> (In reply to Jonathan Bedard from comment #3)
> > (In reply to Alexey Proskuryakov from comment #2)
> > > Fred clarifies in the other bug that this is about Xcode 11.0 beta 3 on
> > > Mojave, not on Catalina.
> > 
> > Ok, that makes more sense. HAVE_SUBVIEWS_IVAR_SPI only gets defined on
> > Catalina. Should is be defined unconditionally on public SDKs?
> 
> The SPI only exists on Catalina. It would build but crash with a missing
> selector otherwise.
> 
> I'll need to think of a fix that will work with the older systems and newer
> SDK.

You should be able to declare the ivar in a class extension in that case.

[Kenneth Russell]

Ran into this problem too after updating to Xcode 11 on 10.4.6 in order to pick up the iOS 13 SDK, which seemed required to build for the iOS Simulator. This presents a chicken-and-egg problem since it's not possible to build for the iOS Simulator without Xcode 11, but not possible to build for macOS with it.

I was able to work around this by adding the following hack to WebHTMLView.mm:

diff --git a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
index 347826af20a..448d4c714d9 100644
--- a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
+++ b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
@@ -217,6 +217,8 @@
 #if !HAVE(SUBVIEWS_IVAR_SPI)
 @implementation NSView (SubviewsIvar)
 
+NSMutableArray<__kindof NSView *> *_subviews;
+
 - (void)_setSubviewsIvar:(NSMutableArray<__kindof NSView *> *)subviews {
     ALLOW_DEPRECATED_DECLARATIONS_BEGIN
     _subviews = subviews;



but am sure this isn't correct. I don't know enough Objective-C to postulate a correct fix.

Any help from folks at Apple?

May I please upgrade this to P1, Major issue? It is a serious problem for all contributors outside Apple.

(In reply to Kenneth Russell from comment #17)
> Ran into this problem too after updating to Xcode 11 on 10.4.6 in order to
> pick up the iOS 13 SDK, which seemed required to build for the iOS
> Simulator. This presents a chicken-and-egg problem since it's not possible
> to build for the iOS Simulator without Xcode 11, but not possible to build
> for macOS with it.
> 
> I was able to work around this by adding the following hack to
> WebHTMLView.mm:
> 
> diff --git a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> index 347826af20a..448d4c714d9 100644
> --- a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> +++ b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> @@ -217,6 +217,8 @@
>  #if !HAVE(SUBVIEWS_IVAR_SPI)
>  @implementation NSView (SubviewsIvar)
>  
> +NSMutableArray<__kindof NSView *> *_subviews;
> +

That’s not correct. It defines a global variable unrelated to the class. Declaring the ivar in a class extension, on the other hand, should work.

[Kenneth Russell]

(In reply to mitz from comment #18)
> (In reply to Kenneth Russell from comment #17)
> > Ran into this problem too after updating to Xcode 11 on 10.4.6 in order to
> > pick up the iOS 13 SDK, which seemed required to build for the iOS
> > Simulator. This presents a chicken-and-egg problem since it's not possible
> > to build for the iOS Simulator without Xcode 11, but not possible to build
> > for macOS with it.
> > 
> > I was able to work around this by adding the following hack to
> > WebHTMLView.mm:
> > 
> > diff --git a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> > b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> > index 347826af20a..448d4c714d9 100644
> > --- a/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> > +++ b/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm
> > @@ -217,6 +217,8 @@
> >  #if !HAVE(SUBVIEWS_IVAR_SPI)
> >  @implementation NSView (SubviewsIvar)
> >  
> > +NSMutableArray<__kindof NSView *> *_subviews;
> > +
> 
> That’s not correct. It defines a global variable unrelated to the class.
> Declaring the ivar in a class extension, on the other hand, should work.

I know it's not correct. As I mentioned I don't know enough Objective-C to write a correct fix - any chance you could put up a patch implementing that? I'll be happy to help test it. Thanks.

Created attachment 380291 [details]
Untested patch

> I know it's not correct. As I mentioned I don't know enough Objective-C to write a correct
> fix - any chance you could put up a patch implementing that? I'll be happy to help test it. Thanks.

I haven’t been able to test this patch, but this is what I had in mind.

[Maciej Stachowiak]

The wincairo and wk1 failures appear unrelated. 

The style checker issue is about having an OS version check here, correctly suggesting it should go in a named macro. I'd do this myself, but I'm not sure what the purpose of the `__MAC_OS_X_VERSION_MAX_ALLOWED >= 101500` clause is.

For folks in this configuration who are having trouble building, could you please report whether the patch solves your problem?

(In reply to Maciej Stachowiak from comment #21)
> The wincairo and wk1 failures appear unrelated. 
> 
> The style checker issue is about having an OS version check here, correctly
> suggesting it should go in a named macro. I'd do this myself, but I'm not
> sure what the purpose of the `__MAC_OS_X_VERSION_MAX_ALLOWED >= 101500`
> clause is.

This makes the ivar declaration only when using the macOS Catalina or later SDK, because earlier SDKs include the declaration, and redeclaring an ivar is an error. I am not aware of WTF macros that express this (since conditionals based on SDK version are so uncommon).

> For folks in this configuration who are having trouble building, could you
> please report whether the patch solves your problem?

[Alexey Proskuryakov]

What style checker is saying is that we should add a HAVE macro for these SDKs in Platform.h. But looking at the patch, there is already HAVE(SUBVIEWS_IVAR_SPI). Isn’t it just defined improperly?

(In reply to Alexey Proskuryakov from comment #23)
> What style checker is saying is that we should add a HAVE macro for these
> SDKs in Platform.h. But looking at the patch, there is already
> HAVE(SUBVIEWS_IVAR_SPI). Isn’t it just defined improperly?

HAVE_SUBVIEWS_IVAR_SPI is defined properly. The SPI is available when deploying to macOS 10.15 or later. When deploying to earlier macOS, the SPI is not available and there is code in WebHTMLView.mm implementing an alternative, but that implementation depends on an ivar declaration. The ivar declaration is not present in the macOS 10.15 and later SDKs. Thus the condition for declaring the ivar in the SPI header is the one stated in the patch.

[Alexey Proskuryakov]

I see, the SPI name confused me into thinking that this macro is about an ivar, which it is not. 

In this case, what you want is HAVE(SUBVIEWS_IVAR_DECLARED_BY_SDK) for the second part of the condition.

[Kenneth Russell]

mitz@ thank you for the patch (which builds fine locally) and ap@ thank you for suggesting how to get it to pass the style checker.

Locally, this patch builds successfully; ap@, not sure whether it's what you had in mind. Without the change to Platform.h, HAVE(SUBVIEWS_IVAR_DECLARED_BY_SDK) isn't defined.

If this looks OK then mitz@ would you like to officially post it?

Thanks again for your help!



diff --git a/Source/WTF/wtf/Platform.h b/Source/WTF/wtf/Platform.h
index 230bfa69843..65706f2ee91 100644
--- a/Source/WTF/wtf/Platform.h
+++ b/Source/WTF/wtf/Platform.h
@@ -1623,6 +1623,10 @@
 #define HAVE_SUBVIEWS_IVAR_SPI 1
 #endif
 
+#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MAX_ALLOWED >= 101500
+#define HAVE_SUBVIEWS_IVAR_DECLARED_BY_SDK 1
+#endif
+
 #if PLATFORM(MAC) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
 #define USE_PLATFORM_SYSTEM_FALLBACK_LIST 1
 #endif
diff --git a/Source/WebCore/PAL/pal/spi/mac/NSViewSPI.h b/Source/WebCore/PAL/pal/spi/mac/NSViewSPI.h
index e0d5b4e7ca1..1731664e4b8 100644
--- a/Source/WebCore/PAL/pal/spi/mac/NSViewSPI.h
+++ b/Source/WebCore/PAL/pal/spi/mac/NSViewSPI.h
@@ -29,7 +29,12 @@
 
 #import <pal/spi/cocoa/QuartzCoreSPI.h>
 
-@interface NSView () <CALayerDelegate>
+@interface NSView () <CALayerDelegate> {
+#if !HAVE(SUBVIEWS_IVAR_SPI) && HAVE(SUBVIEWS_IVAR_DECLARED_BY_SDK)
+@package
+    NSMutableArray<__kindof NSView *> *_subviews;
+#endif
+}
 
 - (NSView *)_findLastViewInKeyViewLoop;

[Maciej Stachowiak]

That change looks good, could you post it for review?

[Alexey Proskuryakov]

> ap@, not sure whether it's what you had in mind.

Yes, looks great. I'll just go ahead and land it, to cut down on red tape.

[Alexey Proskuryakov]

Created attachment 380352 [details]
patch for landing

Comment on attachment 380352 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=380352&action=review

> Source/WTF/wtf/Platform.h:1627
> +#define HAVE_SUBVIEWS_IVAR_DECLARED_BY_SDK 1

This is confusing. The 10.15 SDK does *not* have a declaration of the subviews ivar. It’s also strange to define this here, where out of context it’s not clear which class it’s about. Consolidating all the logic in NSViewSPI.h would make things more clear. The rule that the style checker is trying to enforce makes little sense here.

[Alexey Proskuryakov]

Created attachment 380357 [details]
patch for landing

> This is confusing. The 10.15 SDK does *not* have a declaration of the
> subviews ivar. 

Indeed, thank you for catching this.

> It’s also strange to define this here, where out of context
> it’s not clear which class it’s about. Consolidating all the logic in
> NSViewSPI.h would make things more clear. The rule that the style checker is
> trying to enforce makes little sense here.

The rule is that all versions checks are in Platform.h and similar central files. There were many instances of code where engineers thought that checking versions close to use was better, but moving a subset to Platform.h proved that people were often incorrect about how well they can splatter version checks throughout the code. So now we have this rigid rule. It also helps Apple engineers think twice before doing certain weird things.

[Kenneth Russell]

https://bugs.webkit.org/attachment.cgi?id=380357 fixes the build problem as well. Thanks again!

[WebKit Commit Bot]

Comment on attachment 380357 [details]
patch for landing

Clearing flags on attachment: 380357

Committed r250809: <https://trac.webkit.org/changeset/250809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56058393>