Bug 195756 – Update checks that determine if WebKit is system WebKit

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Rollin]

The system WebKit can be installed in additional locations, so check for and allow those, too.

<rdar://problem/47787742>

[Keith Rollin]

Created attachment 364678 [details]
Patch

[EWS Watchlist]

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.

[Alexey Proskuryakov]

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.

[Keith Rollin]

(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.

[Keith Rollin]

(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.

[Alexey Proskuryakov]

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.

[Keith Rollin]

Created attachment 365240 [details]
Patch

[Keith Rollin]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Keith Rollin]

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.

[Keith Rollin]

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.

[Keith Rollin]

Created attachment 365299 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365299 [details]
Patch

Clearing flags on attachment: 365299

Committed r243230: <https://trac.webkit.org/changeset/243230>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Keith Rollin]

Committed r243251: <https://trac.webkit.org/changeset/243251>

[Keith Rollin]

Committed r243253: <https://trac.webkit.org/changeset/243253>