Summary: | Update checks that determine if WebKit is system WebKit | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Rollin <krollin> | ||||||||
Component: | New Bugs | Assignee: | Keith Rollin <krollin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, ap, benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, jberlin, koivisto, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Keith Rollin
2019-03-14 13:03:54 PDT
Created attachment 364678 [details]
Patch
Attachment 364678 [details] did not pass style-queue:
ERROR: Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm:66: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 364678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364678&action=review > Source/WTF/wtf/Platform.h:1518 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) The braces are not necessary. > Source/WebKit/Configurations/WebKit.xcconfig:146 > +OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS) -isystem $(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders -DPLATFORM_OOB_SYSTEM_CONTENT_DIR="$(PLATFORM_OOB_SYSTEM_CONTENT_DIR)"; Do we have to duplicate content from Base.xcconfig here, or can it be referenced? Also, is there any harm in having PLATFORM_OOB_SYSTEM_CONTENT_DIR defined in Base? > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:727 > +#if HAVE(ALTERNATE_SYSTEM_LAYOUT) > + if ([[webKit2Bundle() bundlePath] hasPrefix:@ STRINGIZE_VALUE_OF(PLATFORM_OOB_SYSTEM_CONTENT_DIR) "/System/"]) > + return YES; > +#endif > return [[webKit2Bundle() bundlePath] hasPrefix:@"/System/"]; It seems like just the new version should work everywhere, does it not? Also, the spaces around STRINGIZE_VALUE_OF() seem unnecessary. > Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm:65 > + return !path.isEmpty() && !path.startsWith("/System/") > +#if HAVE(ALTERNATE_SYSTEM_LAYOUT) > + && !path.startsWith(STRINGIZE_VALUE_OF(PLATFORM_OOB_SYSTEM_CONTENT_DIR) "/System") > +#endif Ditto, not sure why we need to check two variants. (In reply to Alexey Proskuryakov from comment #3) > Comment on attachment 364678 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=364678&action=review > > > Source/WTF/wtf/Platform.h:1518 > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) > > The braces are not necessary. This will be addressed along the lines that we discussed in person. > > Source/WebKit/Configurations/WebKit.xcconfig:146 > > +OTHER_CPLUSPLUSFLAGS = $(ASAN_OTHER_CPLUSPLUSFLAGS) -isystem $(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders -DPLATFORM_OOB_SYSTEM_CONTENT_DIR="$(PLATFORM_OOB_SYSTEM_CONTENT_DIR)"; > > Do we have to duplicate content from Base.xcconfig here, or can it be > referenced? Also, is there any harm in having > PLATFORM_OOB_SYSTEM_CONTENT_DIR defined in Base? I'll put it in Base. > > > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:727 > > +#if HAVE(ALTERNATE_SYSTEM_LAYOUT) > > + if ([[webKit2Bundle() bundlePath] hasPrefix:@ STRINGIZE_VALUE_OF(PLATFORM_OOB_SYSTEM_CONTENT_DIR) "/System/"]) > > + return YES; > > +#endif > > return [[webKit2Bundle() bundlePath] hasPrefix:@"/System/"]; > > It seems like just the new version should work everywhere, does it not? I think you're right. I was going to reply that I thought we needed to be careful of a transition period where not all trains supported ALTERNATE_SYSTEM_LAYOUT, but the change above (and other below that you noted) concern only WebKit, the current state of which we know. > > Also, the spaces around STRINGIZE_VALUE_OF() seem unnecessary. I'll double-check that. I had problems getting the syntax sufficiently correct. > > > Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm:65 > > + return !path.isEmpty() && !path.startsWith("/System/") > > +#if HAVE(ALTERNATE_SYSTEM_LAYOUT) > > + && !path.startsWith(STRINGIZE_VALUE_OF(PLATFORM_OOB_SYSTEM_CONTENT_DIR) "/System") > > +#endif > > Ditto, not sure why we need to check two variants. Ditto. (In reply to Keith Rollin from comment #4) > (In reply to Alexey Proskuryakov from comment #3) > > Comment on attachment 364678 [details] > > > > > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:727 > > > +#if HAVE(ALTERNATE_SYSTEM_LAYOUT) > > > + if ([[webKit2Bundle() bundlePath] hasPrefix:@ STRINGIZE_VALUE_OF(PLATFORM_OOB_SYSTEM_CONTENT_DIR) "/System/"]) > > > + return YES; > > > +#endif > > > return [[webKit2Bundle() bundlePath] hasPrefix:@"/System/"]; > > > > It seems like just the new version should work everywhere, does it not? > > I think you're right. I was going to reply that I thought we needed to be > careful of a transition period where not all trains supported > ALTERNATE_SYSTEM_LAYOUT, but the change above (and other below that you > noted) concern only WebKit, the current state of which we know. Actually, yes. We need to check both locations. Let's talk about this. Comment on attachment 364678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364678&action=review >>> Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm:65 >>> +#endif >> >> Ditto, not sure why we need to check two variants. > > Ditto. Discussed in person, and these cases are not the same. Will start an e-mail thread. Created attachment 365240 [details]
Patch
Changes in this version: * I removed the .xcconfig file changes due to the email comments from Jessie and others. * I left the isSystemWebKit check in shouldAllowNonValidInjectedCode as it was in my first patch. Alexey and I had earlier talked about leaving the isSystemWebKit check unchanged from the original pre-patch code. However, I was uncomfortable with that because that would result in the function behaving differently when WebKit was installed in StagedFrameworks. I looked at the surrounding code to understand how it would react to this change, but didn't really understand it and didn't know how to test it. Comment on attachment 365240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365240&action=review > Source/WebKit/UIProcess/mac/WebProcessProxyMac.mm:49 > bool WebProcessProxy::shouldAllowNonValidInjectedCode() const I'm not quite sure what additional details we got, but it was my understanding that we didn't want to change this code. So I don't know why it's being changed. In other words, changing code thst we don't quite understand and claiming that it's for the best strikes me as the wrong thing to do. The point of the change is to not change the externally-observed behavior of shouldAllowNonValidInjectedCode. The location of StagedFrameworks is changing. As is shouldAllowNonValidInjectedCode will return true when StagedFrameworks is in its old location, but will return false when StagedFrameworks is in its new location. This change causes shouldAllowNonValidInjectedCode to return true with StagedFrameworks in either location, restoring its externally observed behavior and returning true if WebKit is in StagedFrameworks, regardless of where StagedFrameworks actually is. Jessie and I have gone over shouldAllowNonValidInjectedCode, its history, and its purpose, and are in accord with Alexey that we can leave it as-is. New patching coming. Created attachment 365299 [details]
Patch
Comment on attachment 365299 [details] Patch Clearing flags on attachment: 365299 Committed r243230: <https://trac.webkit.org/changeset/243230> All reviewed patches have been landed. Closing bug. Committed r243251: <https://trac.webkit.org/changeset/243251> Committed r243253: <https://trac.webkit.org/changeset/243253> |