Bug 184166 – Xcode prepends line comments from WTF/Compiler.h to *.sb files

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Ross Kirsling]

Repro:

1. Add a line comment (// not /* */) to the start of a line in this file: https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/Compiler.h
2. Open Xcode and hit the Play button
=> WebContent process crashes immediately

Cause:

The comment gets prepended to this file, in which C-style comments are invalid:
/Users/.../Library/Developer/Xcode/DerivedData/WebKit-.../Build/Products/Debug/WebKit.framework/Resources/com.apple.WebProcess.sb
(It will also be prepended to com.apple.WebKit.NetworkProcess.sb and com.apple.WebKit.Storage.sb in the same directory.)

This file originates here:
https://github.com/WebKit/webkit/blob/master/Source/WebKit/WebProcess/com.apple.WebProcess.sb.in
It appears to be processed by this script:
https://github.com/WebKit/webkit/blob/master/Source/WebKit/DerivedSources.make


Reason for concern:

This issue is very time-consuming to debug for the following reasons.
- It is completely unexpected, as C, C++, and ObjC all support line comments.
- The issue only occurs from the Xcode UI and not from the `build-webkit` CLI.
- Selecting Product > Clean in Xcode is not a fix -- one must actually delete the DerivedData subdirectory (the existence of which is hardly common knowledge!).

[Alexey Proskuryakov]

The fix would be two-fold:

1. Add a script step to remove comment lines from processed profiles.
2. Change preprocessor mode to C++.

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #1)
> The fix would be two-fold:
> 
> 1. Add a script step to remove comment lines from processed profiles.
> 2. Change preprocessor mode to C++.

Looks like both parts might be doable all in one step here:
https://github.com/WebKit/webkit/blob/master/Source/WebKit/DerivedSources.make#L220-L227

I wonder if it's safe to replace all of the above with the following?
> TEXT_PREPROCESSOR_FLAGS=-E -P -x -w

[Ross Kirsling]

(In reply to Ross Kirsling from comment #2)
> I wonder if it's safe to replace all of the above with the following?
> > TEXT_PREPROCESSOR_FLAGS=-E -P -x -w

Sorry, that should have been:
> TEXT_PREPROCESSOR_FLAGS=-E -P -w

[Alexey Proskuryakov]

The C++ preprocessor would result in breaking comments that contain URLs, so a step to remove semicolon based comments is necessary. 

This is all of course based on an assumption that we will never have two slashes in an actual sandbox rule. I think that it’s an ok assumption to make.

[Ross Kirsling]

(In reply to Alexey Proskuryakov from comment #4)
> The C++ preprocessor would result in breaking comments that contain URLs, so
> a step to remove semicolon based comments is necessary.
>
> This is all of course based on an assumption that we will never have two
> slashes in an actual sandbox rule. I think that it’s an ok assumption to
> make.

That makes sense. I just discovered your original motivation in https://bugs.webkit.org/show_bug.cgi?id=83827. :)

[Ross Kirsling]

Created attachment 336872 [details]
Patch

[Alexey Proskuryakov]

*** Bug 181309 has been marked as a duplicate of this bug. ***

[Ross Kirsling]

I believe this patch satisfies the desired criteria. It is naive in indiscriminately treating semicolons as comment-openers in *.sb.in files, but it's a simple fix that's compatible with the way those files are currently used.

[Brent Fulgham]

Comment on attachment 336872 [details]
Patch

Looks good. r=me.

[WebKit Commit Bot]

Comment on attachment 336872 [details]
Patch

Clearing flags on attachment: 336872

Committed r230213: <https://trac.webkit.org/changeset/230213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39147349>