184166 2018-03-29 17:46:06 -0700 Xcode prepends line comments from WTF/Compiler.h to *.sb files 2018-04-03 11:19:03 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=184062 InRadar P2 Normal --- 1 ross.kirsling ross.kirsling ap bfulgham commit-queue webkit-bug-importer oldest_to_newest 1410462 0 ross.kirsling 2018-03-29 17:46:06 -0700 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!). 1410515 1 ap 2018-03-29 20:42:08 -0700 The fix would be two-fold: 1. Add a script step to remove comment lines from processed profiles. 2. Change preprocessor mode to C++. 1410521 2 ross.kirsling 2018-03-29 20:50:26 -0700 (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 1410522 3 ross.kirsling 2018-03-29 20:53:01 -0700 (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 1410524 4 ap 2018-03-29 21:01:16 -0700 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. 1410525 5 ross.kirsling 2018-03-29 21:09:09 -0700 (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. :) 1410669 6 336872 ross.kirsling 2018-03-30 11:01:34 -0700 Created attachment 336872 Patch 1410944 7 ap 2018-03-31 23:03:08 -0700 *** Bug 181309 has been marked as a duplicate of this bug. *** 1411467 8 ross.kirsling 2018-04-03 10:48:31 -0700 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. 1411469 9 336872 bfulgham 2018-04-03 10:50:07 -0700 Comment on attachment 336872 Patch Looks good. r=me. 1411488 10 336872 commit-queue 2018-04-03 11:15:00 -0700 Comment on attachment 336872 Patch Clearing flags on attachment: 336872 Committed r230213: <https://trac.webkit.org/changeset/230213> 1411489 11 commit-queue 2018-04-03 11:15:02 -0700 All reviewed patches have been landed. Closing bug. 1411494 12 webkit-bug-importer 2018-04-03 11:19:03 -0700 <rdar://problem/39147349> 336872 2018-03-30 11:01:34 -0700 2018-04-03 11:15:00 -0700 Patch bug-184166-20180330110134.patch text/plain 2141 ross.kirsling U3VidmVyc2lvbiBSZXZpc2lvbjogMjMwMTA3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDNiODNjMzBkYjYyYTI3OGMw ZjZkZTkyZGMwZWVhNGZiMGJmY2MyMGYuLjNhZTA5MmU2ZDIyNmI1MDE1MmVlNzQ2ZTU5MGZiZDJi MjdlZjMwMWYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTgtMDMtMzAgIFJvc3MgS2ly c2xpbmcgIDxyb3NzLmtpcnNsaW5nQHNvbnkuY29tPgorCisgICAgICAgIFhjb2RlIHByZXBlbmRz IGxpbmUgY29tbWVudHMgZnJvbSBXVEYvQ29tcGlsZXIuaCB0byAqLnNiIGZpbGVzCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODQxNjYKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIERlcml2ZWRTb3VyY2Vz Lm1ha2U6CisgICAgICAgIFN0cmlwIDstY29tbWVudHMgZnJvbSAqLnNiLmluIGZpbGVzIGJlZm9y ZSBwcmVwcm9jZXNzaW5nIHNvIHdlIGNhbiBzdG9wIHRyZWF0aW5nIFBsYXRmb3JtLmgvQ29tcGls ZXIuaCBhcyBDODkuCisKIDIwMTgtMDMtMjkgIEpGIEJhc3RpZW4gIDxqZmJhc3RpZW5AYXBwbGUu Y29tPgogCiAgICAgICAgIFVzZSBGb3J3YXJkLmggaW5zdGVhZCBvZiBmb3J3YXJkLWRlY2xhcmlu ZyBXVEY6OlN0cmluZwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9EZXJpdmVkU291cmNlcy5t YWtlIGIvU291cmNlL1dlYktpdC9EZXJpdmVkU291cmNlcy5tYWtlCmluZGV4IDlkNjZjMGRmNmVj NzY0MmY0ZTAwYmM5MzFmMTJlMTdjNjFlYWJmNGMuLjIyODA0MDAwZjliOTM2MWQ1Njg0ZmJiMDM4 NzY0N2M1MmZlNjQ2YTIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvRGVyaXZlZFNvdXJjZXMu bWFrZQorKysgYi9Tb3VyY2UvV2ViS2l0L0Rlcml2ZWRTb3VyY2VzLm1ha2UKQEAgLTIxNiwxNSAr MjE2LDcgQEAgYWxsIDogXAogCUBlY2hvIEdlbmVyYXRpbmcgbWVzc2FnZSByZWNlaXZlciBmb3Ig JCouLi4KIAlAcHl0aG9uICQoV2ViS2l0MikvU2NyaXB0cy9nZW5lcmF0ZS1tZXNzYWdlcy1oZWFk ZXIucHkgJDwgPiAkQAogCi0KLSMgU29tZSB2ZXJzaW9ucyBvZiBjbGFuZyBpbmNvcnJlY3RseSBz dHJpcCBvdXQgLy8gY29tbWVudHMgaW4gYzg5IGNvZGUuCi0jIFVzZSAtdHJhZGl0aW9uYWwgYXMg YSB3b3JrYXJvdW5kLCBidXQgb25seSB3aGVuIG5lZWRlZCBzaW5jZSB0aGF0IGNhdXNlcwotIyBv dGhlciBwcm9ibGVtcyB3aXRoIGxhdGVyIHZlcnNpb25zIG9mIGNsYW5nLgotaWZlcSAoJChzaGVs bCBlY2hvICcvL3gnIHwgJChDQykgLUUgLVAgLXggYyAtc3RkPWM4OSAtIHwgZ3JlcCB4KSwpCi1U RVhUX1BSRVBST0NFU1NPUl9GTEFHUz0tRSAtUCAteCBjIC10cmFkaXRpb25hbCAtdwotZWxzZQot VEVYVF9QUkVQUk9DRVNTT1JfRkxBR1M9LUUgLVAgLXggYyAtc3RkPWM4OSAtdwotZW5kaWYKK1RF WFRfUFJFUFJPQ0VTU09SX0ZMQUdTPS1FIC1QIC13CiAKIGlmbmVxICgkKFNES1JPT1QpLCkKIAlT REtfRkxBR1M9LWlzeXNyb290ICQoU0RLUk9PVCkKQEAgLTI0NCw3ICsyMzYsNyBAQCBhbGw6ICQo U0FOREJPWF9QUk9GSUxFUykKIAogJS5zYiA6ICUuc2IuaW4KIAlAZWNobyBQcmUtcHJvY2Vzc2lu ZyAkKiBzYW5kYm94IHByb2ZpbGUuLi4KLQkkKENDKSAkKFNES19GTEFHUykgJChUQVJHRVRfVFJJ UExFX0ZMQUdTKSAkKFRFWFRfUFJFUFJPQ0VTU09SX0ZMQUdTKSAkKEZSQU1FV09SS19GTEFHUykg JChIRUFERVJfRkxBR1MpIC1pbmNsdWRlICJ3dGYvUGxhdGZvcm0uaCIgJDwgPiAkQAorCWdyZXAg LW8gJ15bXjtdKicgJDwgfCAkKENDKSAkKFNES19GTEFHUykgJChUQVJHRVRfVFJJUExFX0ZMQUdT KSAkKFRFWFRfUFJFUFJPQ0VTU09SX0ZMQUdTKSAkKEZSQU1FV09SS19GTEFHUykgJChIRUFERVJf RkxBR1MpIC1pbmNsdWRlICJ3dGYvUGxhdGZvcm0uaCIgLSA+ICRACiAKIEFVVE9NQVRJT05fUFJP VE9DT0xfR0VORVJBVE9SX1NDUklQVFMgPSBcCiAJJChKYXZhU2NyaXB0Q29yZV9TQ1JJUFRTX0RJ UikvY3BwX2dlbmVyYXRvcl90ZW1wbGF0ZXMucHkgXAo=