Bug 224386 – Consistently include JavaScriptCore's Inspector private interface headers

• Component: New Bugs
• Status: NEW

[Alex Christensen]

Consistently include JavaScriptCore's Inspector private interface headers

[Alex Christensen]

Created attachment 425641 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Alex Christensen]

Created attachment 425647 [details]
Patch

[Blaze Burg]

Comment on attachment 425647 [details]
Patch

Thanks for this cleanup. Alas, your patch needs to include rebaselined code generator results. Try `run-inspector-generator-tests --rebaseline`.

We forgot to update these results last time (with the (__bridge CFStringRef change) so please ensure it's done this time.

[Blaze Burg]

(In reply to BJ Burg from comment #4)
> Comment on attachment 425647 [details]
> Patch
> 
> Thanks for this cleanup. Alas, your patch needs to include rebaselined code
> generator results. Try `run-inspector-generator-tests --rebaseline`.
> 
> We forgot to update these results last time (with the (__bridge CFStringRef
> change) so please ensure it's done this time.

Other than that, I don't see anything worrying. It would be worth having Don take a look at the CMake specific changes.

[Alexey Proskuryakov]

> We forgot to update these results last time (with the (__bridge CFStringRef change) so please ensure it's done this time.

Can/should these run in EWS? I'd appreciate a bug if so.

[Alex Christensen]

Created attachment 425676 [details]
Patch

[Yusuke Suzuki]

Will this failure be solved by this patch?
https://ews-build.webkit.org/#/builders/8/builds/50253

[Don Olmstead]

(In reply to BJ Burg from comment #5)
> (In reply to BJ Burg from comment #4)
> > Comment on attachment 425647 [details]
> > Patch
> > 
> > Thanks for this cleanup. Alas, your patch needs to include rebaselined code
> > generator results. Try `run-inspector-generator-tests --rebaseline`.
> > 
> > We forgot to update these results last time (with the (__bridge CFStringRef
> > change) so please ensure it's done this time.
> 
> Other than that, I don't see anything worrying. It would be worth having Don
> take a look at the CMake specific changes.

Can you explain why the private JavaScriptCore headers are being included?

The reason the public JavaScriptCore headers can be included <JavaScriptCore/*.h> style when building JavaScriptCore is because they use header guards. With everything else using pragma once this could blow up in our face later.

[Darin Adler]

Comment on attachment 425676 [details]
Patch

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

> Source/JavaScriptCore/inspector/scripts/tests/expected/shadowed-optional-type-setters.json-result:1005
> +    Optional<TestProtocolRuntimeKeyPathType> type = fromProtocolString<TestProtocolRuntimeKeyPathType>((__bridge CFStringRef)payload[@"type"]);

What caused this change? Maybe someone forgot to regenerate these expected results and we’re not running this test?

[Darin Adler]

Keep in mind that on Apple platforms the three categories are:

1) Public headers, these are for API and public use.

2) Private headers, these are used for interfaces between frameworks and other system components that are updated in "lock step" with WebKit. On Apple platforms, WTF and JavaScriptCore are in one framework, WebCore in another, WebKit in another, WebKitLegacy in yet another. And Safari uses some of these.

3) Internal headers, used only within a framework.

So when we say "private" headers we are talking about (2), not (3).

The framework-style includes like <JavaScriptCore/*.h> are the correct way to use (1) and (2) when including outside the framework, like in the headers themselves or in the other frameworks. And as you know, we do various tricks to make these framework-style includes work on other platforms where that style doesn’t exist.

Don Olmstead asked "why the private JavaScriptCore headers are being included". The answer would normally be that if they are not intended to be included they should be internal headers.

What complicates things is that within the WebKit project we do have to use category (2) for interfaces between the separate frameworks that make up the project. And maybe some of those are only intended for use in code that is in the WebKit tree. That could create two subcategories:

2a) Private headers that are intended to be exported from the WebKit project for one reason or another. Not the same

2b) Private headers that are actually internal to the WebKit project, but need to be private framework headers so that WebKit's multiple frameworks can compile and link together properly.

Right ow we don’t have a good way to distinguish these (2a) and (2b) categories. Might be nice to do that to help us understand things.

One critical thing about headers in the (1) and (2a) category is that the headers must not use anything from internal headers like the Platform.h family. That’s why we might need to distinguish (2a) from (2b).

[Don Olmstead]

(In reply to Darin Adler from comment #11)
> Keep in mind that on Apple platforms the three categories are:
> 
> 1) Public headers, these are for API and public use.
> 
> 2) Private headers, these are used for interfaces between frameworks and
> other system components that are updated in "lock step" with WebKit. On
> Apple platforms, WTF and JavaScriptCore are in one framework, WebCore in
> another, WebKit in another, WebKitLegacy in yet another. And Safari uses
> some of these.
> 
> 3) Internal headers, used only within a framework.
> 
> So when we say "private" headers we are talking about (2), not (3).
> 
> The framework-style includes like <JavaScriptCore/*.h> are the correct way
> to use (1) and (2) when including outside the framework, like in the headers
> themselves or in the other frameworks. And as you know, we do various tricks
> to make these framework-style includes work on other platforms where that
> style doesn’t exist.
> 
> Don Olmstead asked "why the private JavaScriptCore headers are being
> included". The answer would normally be that if they are not intended to be
> included they should be internal headers.
> 
> What complicates things is that within the WebKit project we do have to use
> category (2) for interfaces between the separate frameworks that make up the
> project. And maybe some of those are only intended for use in code that is
> in the WebKit tree. That could create two subcategories:
> 
> 2a) Private headers that are intended to be exported from the WebKit project
> for one reason or another. Not the same
> 
> 2b) Private headers that are actually internal to the WebKit project, but
> need to be private framework headers so that WebKit's multiple frameworks
> can compile and link together properly.
> 
> Right ow we don’t have a good way to distinguish these (2a) and (2b)
> categories. Might be nice to do that to help us understand things.
> 
> One critical thing about headers in the (1) and (2a) category is that the
> headers must not use anything from internal headers like the Platform.h
> family. That’s why we might need to distinguish (2a) from (2b).

Thanks for the thorough analysis as always Darin!

My concerns are more practical in this matter. When I was going through and cleaning up the CMake build I found a ton of instances where things were getting included wrong but due to how the include directories were specified things worked. As an example that's present right now there's https://trac.webkit.org/browser/trunk/Source/WebKit/UIProcess/API/glib/WebKitInitialize.cpp#L30 which has this include.

#include <WebKit/Shared/WebKit2Initialize.h>

This works because in CMake SOURCE_DIR is an include because of how the forwarding header scripts work. Most likely a code editor decided to insert that rather than #include "WebKit2Initialize.h".

So if an #include <JavaScriptCore/*.h> slips in to JavaScriptCore this could result in problems down the line. In the CMake build everything in the PRIVATE_FRAMEWORK_HEADERS uses #pragma once so with how things are included you could hit an issue where both #include "Foo.h" and #include <JavaScriptCore/Foo.h> are referenced in JavaScriptCore.

I'm fully supportive of the Mac CMake build living again and I just have a very bad feeling about this change so I'd like to know what prompted it to see if there's any other approach that would work before giving the okay.

[Darin Adler]

(In reply to Don Olmstead from comment #12)
> So if an #include <JavaScriptCore/*.h> slips in to JavaScriptCore this could
> result in problems down the line. In the CMake build everything in the
> PRIVATE_FRAMEWORK_HEADERS uses #pragma once so with how things are included
> you could hit an issue where both #include "Foo.h" and #include
> <JavaScriptCore/Foo.h> are referenced in JavaScriptCore.

I don’t understand the recommendation for the best practice on how we can deal with this. In JavaScriptCore, there are headers that must use the <JavaScriptCore/*.h> style to include other headers for the reasons I cited above. And we might include one of those headers with "*.h" style from a JavaScriptCore source file.

One fix would be to *only* allow the <JavaScriptCore/*.h> style for any header where we have to do that even once. We could very easily write a checker in Python that could catch that mistake.

But maybe you know better what the rule should be? I must admit I am a little confused.

[Alex Christensen]

This change is needed because AugmentableInspectorController.h is currently included from JavaScriptCore on macOS, which includes three headers as <JavaScriptCore/*.h> but those three headers are included elsewhere as #include "*.h".  Xcode has some magic that finds the headers consistently, but using command line clang if you build a cpp file in the same directory as one of those three headers, it will search in the cpp file's directory first, then the include directories.  The solution is to consistently include 1) and 2) with <Framework/Header.h> which is what I did here.

[Don Olmstead]

Created attachment 425784 [details]
WIP Patch

[Don Olmstead]

(In reply to Alex Christensen from comment #14)
> This change is needed because AugmentableInspectorController.h is currently
> included from JavaScriptCore on macOS, which includes three headers as
> <JavaScriptCore/*.h> but those three headers are included elsewhere as
> #include "*.h".  Xcode has some magic that finds the headers consistently,
> but using command line clang if you build a cpp file in the same directory
> as one of those three headers, it will search in the cpp file's directory
> first, then the include directories.  The solution is to consistently
> include 1) and 2) with <Framework/Header.h> which is what I did here.

I did a build of WinCairo with ENABLE_INSPECTOR_ALTERNATE_DISPATCHERS turned on and hit the build error you hit. Added a patch that uses "Foo.h" instead to see if the build works on xcode.

[Alex Christensen]

I, too, prefer Don's approach.

[Don Olmstead]

(In reply to Alex Christensen from comment #17)
> I, too, prefer Don's approach.

Does it work for the Mac CMake build?

[Alex Christensen]

Yes.

[Don Olmstead]

(In reply to Alex Christensen from comment #19)
> Yes.

https://bugs.webkit.org/show_bug.cgi?id=224456 for the fix

[Radar WebKit Bug Importer]

<rdar://problem/76774261>