Bug 184166 - Xcode prepends line comments from WTF/Compiler.h to *.sb files
Summary: Xcode prepends line comments from WTF/Compiler.h to *.sb files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
: 181309 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-03-29 17:46 PDT by Ross Kirsling
Modified: 2018-04-03 11:19 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.09 KB, patch)
2018-03-30 11:01 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2018-03-29 17:46:06 PDT
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!).
Comment 1 Alexey Proskuryakov 2018-03-29 20:42:08 PDT
The fix would be two-fold:

1. Add a script step to remove comment lines from processed profiles.
2. Change preprocessor mode to C++.
Comment 2 Ross Kirsling 2018-03-29 20:50:26 PDT
(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
Comment 3 Ross Kirsling 2018-03-29 20:53:01 PDT
(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
Comment 4 Alexey Proskuryakov 2018-03-29 21:01:16 PDT
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.
Comment 5 Ross Kirsling 2018-03-29 21:09:09 PDT
(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. :)
Comment 6 Ross Kirsling 2018-03-30 11:01:34 PDT
Created attachment 336872 [details]
Patch
Comment 7 Alexey Proskuryakov 2018-03-31 23:03:08 PDT
*** Bug 181309 has been marked as a duplicate of this bug. ***
Comment 8 Ross Kirsling 2018-04-03 10:48:31 PDT
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.
Comment 9 Brent Fulgham 2018-04-03 10:50:07 PDT
Comment on attachment 336872 [details]
Patch

Looks good. r=me.
Comment 10 WebKit Commit Bot 2018-04-03 11:15:00 PDT
Comment on attachment 336872 [details]
Patch

Clearing flags on attachment: 336872

Committed r230213: <https://trac.webkit.org/changeset/230213>
Comment 11 WebKit Commit Bot 2018-04-03 11:15:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-04-03 11:19:03 PDT
<rdar://problem/39147349>