Bug 181309 – Sandbox preprocessing should handle C++ comments

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit2

[Brent Fulgham]

The text preprocessing rules in WebKit's DerivedSources.make is now only used for processing our sandbox profiles. Sometimes the headers and availability macros included on macOS and iOS SDK's have spurious C++-style comments in them, which are not stripped out during preprocessing since we were telling clang to use C89 rules for preprocessing.

Instead, we should switch to C99 so that C++-style comments will get properly ignored, and we won't confuse the Sandbox library when it tries the read the finished file.

[Radar WebKit Bug Importer]

<rdar://problem/36306874>

[Brent Fulgham]

Created attachment 330490 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 330490 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Switch to C99 rules for preprocessing our sandbox profiles. No other code uses the TEXT_PREPROCESSOR_FLAGS

Can you diff processed profiles? This seems likely to break all comments with "rdar://" in them. Theoretically, it can break actual rules if they have double slashes, but we don't have any like that now.

[Brent Fulgham]

Comment on attachment 330490 [details]
Patch

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

>> Source/WebKit/ChangeLog:9
>> +        Switch to C99 rules for preprocessing our sandbox profiles. No other code uses the TEXT_PREPROCESSOR_FLAGS
> 
> Can you diff processed profiles? This seems likely to break all comments with "rdar://" in them. Theoretically, it can break actual rules if they have double slashes, but we don't have any like that now.

Yes -- it definitely cuts off everything after the "//" in the comment, so maybe we don't want to do that.

We should probably create a WebKit-specific filter program that does exactly what we want.

[Darin Adler]

Comment on attachment 330490 [details]
Patch

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

>>> Source/WebKit/ChangeLog:9
>>> +        Switch to C99 rules for preprocessing our sandbox profiles. No other code uses the TEXT_PREPROCESSOR_FLAGS
>> 
>> Can you diff processed profiles? This seems likely to break all comments with "rdar://" in them. Theoretically, it can break actual rules if they have double slashes, but we don't have any like that now.
> 
> Yes -- it definitely cuts off everything after the "//" in the comment, so maybe we don't want to do that.
> 
> We should probably create a WebKit-specific filter program that does exactly what we want.

No, I don’t think this will break /* */ comments with // inside them. Those work fine in C++; why wouldn’t clang handle that properly?

Actual rules with double slashes is something different, though. Maybe that’s important?

[Alexey Proskuryakov]

The comments in .sb.in files are of this form:

    ; "com.apple.coremedia.endpointpicker.xpc" can be removed when <rdar://problem/30081582> is resolved.

And with this patch, the built .sb file has lines like this:

    ; "com.apple.coremedia.endpointpicker.xpc" can be removed when <rdar:

I don't think that it's particularly important to preserve comments in sandbox profiles, so maybe the solution is to switch from ";" to "//" for comments. The only downside will be that comments will be inconsistent between processed and non-processed sandbox profile sources (e.g. Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in vs. Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb).

And yes, rules with double slashes are probably not important. If one is ever introduced, this will almost certainly break profile compilation, so bots will quickly tell us about the problem.

[Alexey Proskuryakov]

*** This bug has been marked as a duplicate of bug 184166 ***