Bug 181804 – Expose Safe Browsing SPI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Zach Li]

Expose Safe Browsing SPI

[Zach Li]

Created attachment 331644 [details]
Patch

[Alex Christensen]

Comment on attachment 331644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331644&action=review

> Source/WebKit/Configurations/WebKit.xcconfig:94
> +WK_SAFE_BROWSING_LDFLAGS[sdk=iphone*] = -framework SafariSafeBrowsing;
> +WK_SAFE_BROWSING_LDFLAGS[sdk=iphone*10.*] = ;
> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx*] = -weak_framework SafariSafeBrowsing;
> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.11*] = ;
> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ;

If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above.  I'm not sure if this works.  Also, why don't we just link on all operating systems?

[Zach Li]

Comment on attachment 331644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331644&action=review

>> Source/WebKit/Configurations/WebKit.xcconfig:94
>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ;
> 
> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above.  I'm not sure if this works.  Also, why don't we just link on all operating systems?

We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal?

[Alex Christensen]

Comment on attachment 331644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331644&action=review

>>> Source/WebKit/Configurations/WebKit.xcconfig:94
>>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ;
>> 
>> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above.  I'm not sure if this works.  Also, why don't we just link on all operating systems?
> 
> We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal?

I'm not sure.  I'm interested in what Dan thinks about this.  I don't know this syntax or best practices well.

> Source/WebKit/Platform/spi/Cocoa/SafeBrowsingSPI.h:28
> +#pragma once

I think pragma once needs to be first.

> Source/WebKit/Platform/spi/Cocoa/SafeBrowsingSPI.h:73
> +#endif // not USE(APPLE_INTERNAL_SDK)

not should be removed

Comment on attachment 331644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331644&action=review

>>>> Source/WebKit/Configurations/WebKit.xcconfig:94
>>>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ;
>>> 
>>> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above.  I'm not sure if this works.  Also, why don't we just link on all operating systems?
>> 
>> We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal?
> 
> I'm not sure.  I'm interested in what Dan thinks about this.  I don't know this syntax or best practices well.

The target macOS version is the right thing to use for linking (as well as for any code that uses the feature). As soon as the patch from bug 181803 lands, you should be able to write
WK_SAFE_BROWSING_LDFLAGS[sdk=macosx*] = $(WK_SAFE_BROWSING_LDFLAGS$(WK_MACOS_1013));
WK_SAFE_BROWSING_LDFLAGS_MACOS_SINCE_1013 = -weak_framework SafariSafeBrowsing;

[Alex Christensen]

Comment on attachment 331644 [details]
Patch

Great!  Please adopt Dan's new future-proof way of doing this.

[Zach Li]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 331644 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331644&action=review
> 
> >>> Source/WebKit/Configurations/WebKit.xcconfig:94
> >>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ;
> >> 
> >> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above.  I'm not sure if this works.  Also, why don't we just link on all operating systems?
> > 
> > We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal?
> 
> I'm not sure.  I'm interested in what Dan thinks about this.  I don't know
> this syntax or best practices well.
> 
> > Source/WebKit/Platform/spi/Cocoa/SafeBrowsingSPI.h:28
> > +#pragma once
> 
> I think pragma once needs to be first.

Sure.

> 
> > Source/WebKit/Platform/spi/Cocoa/SafeBrowsingSPI.h:73
> > +#endif // not USE(APPLE_INTERNAL_SDK)
> 
> not should be removed

Should it? Because this #endif is closer to the else statement which is when we are not using Apple internal SDK. But if this is the style WebKit uses, I will follow.

[Zach Li]

(In reply to mitz from comment #5)
> Comment on attachment 331644 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331644&action=review
> 
> >>>> Source/WebKit/Configurations/WebKit.xcconfig:94
> >>>> +WK_SAFE_BROWSING_LDFLAGS[sdk=macosx10.12*] = ;
> >>> 
> >>> If we do want to do this differently for different operating system versions, I think it will need to use $(TARGET_MAC_OS_X_VERSION_MAJOR) like we do above.  I'm not sure if this works.  Also, why don't we just link on all operating systems?
> >> 
> >> We could link against SafariSafeBrowsing framework on all operating systems we support, but we will only use Safe Browsing pieces on High Sierra and iOS 11 and above because the symbols were introduced then. This will use the SDK version that comes with Xcode, so it will behave differently based on the SDK version. Is that not ideal?
> > 
> > I'm not sure.  I'm interested in what Dan thinks about this.  I don't know this syntax or best practices well.
> 
> The target macOS version is the right thing to use for linking (as well as
> for any code that uses the feature). As soon as the patch from bug 181803
> lands, you should be able to write
> WK_SAFE_BROWSING_LDFLAGS[sdk=macosx*] =
> $(WK_SAFE_BROWSING_LDFLAGS$(WK_MACOS_1013));
> WK_SAFE_BROWSING_LDFLAGS_MACOS_SINCE_1013 = -weak_framework
> SafariSafeBrowsing;

Yay, thanks Dan for making it easier to use build settings.

[Zach Li]

Created attachment 331687 [details]
Patch v2

[WebKit Commit Bot]

Comment on attachment 331687 [details]
Patch v2

Clearing flags on attachment: 331687

Committed r227211: <https://trac.webkit.org/changeset/227211>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

<rdar://problem/36626946>

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #10)
> Comment on attachment 331687 [details]
> Patch v2
> 
> Clearing flags on attachment: 331687
> 
> Committed r227211: <https://trac.webkit.org/changeset/227211>
This change breaks testing on iOS Simulator. WebKitTestRunnerApp crashes on launch, as seen here on EWS:
https://webkit-queues.webkit.org/results/6136155

[Ryan Haddad]

(In reply to Ryan Haddad from comment #13)
> (In reply to WebKit Commit Bot from comment #10)
> > Comment on attachment 331687 [details]
> > Patch v2
> > 
> > Clearing flags on attachment: 331687
> > 
> > Committed r227211: <https://trac.webkit.org/changeset/227211>
> This change breaks testing on iOS Simulator. WebKitTestRunnerApp crashes on
> launch, as seen here on EWS:
> https://webkit-queues.webkit.org/results/6136155

Crashed Thread:        0

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Termination Reason:    DYLD, [0x3] Wrong version

Application Specific Information:
dyld: launch, loading dependent libraries
DYLD_FRAMEWORK_PATH=/Volumes/Data/slave/ios-simulator-11-release-tests-wk2/build/WebKitBuild/Release-iphonesimulator
DYLD_FALLBACK_LIBRARY_PATH=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/usr/lib
DYLD_ROOT_PATH=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot
DYLD_FALLBACK_FRAMEWORK_PATH=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/Frameworks
DYLD_LIBRARY_PATH=/Volumes/Data/slave/ios-simulator-11-release-tests-wk2/build/WebKitBuild/Release-iphonesimulator

Dyld Error Message:
  Library not loaded: /System/Library/PrivateFrameworks/SafariSafeBrowsing.framework/SafariSafeBrowsing
  Referenced from: /Volumes/VOLUME/*/WebKit.framework/WebKit
  Reason: Incompatible library version: WebKit requires version 1.0.0 or later, but SafariSafeBrowsing provides version 0.0.0

[Ryan Haddad]

Reverted r227211 for reason:

Breaks iOS Simulator tests.

Committed r227224: <https://trac.webkit.org/changeset/227224>

[Zach Li]

Dan helped me figure out I should include "current-version: 0.0.0" in SafariSafeBrowsing.tbd file.

[Zach Li]

Created attachment 331784 [details]
Patch v3

[EWS Watchlist]

Comment on attachment 331784 [details]
Patch v3

Attachment 331784 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6142313

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framerate.html

[EWS Watchlist]

Created attachment 331811 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Zach Li]

Created attachment 331813 [details]
Patch v3

[Zach Li]

Created attachment 331827 [details]
Patch v3

[WebKit Commit Bot]

Comment on attachment 331827 [details]
Patch v3

Clearing flags on attachment: 331827

Committed r227265: <https://trac.webkit.org/changeset/227265>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 331827 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=331827&action=review

> Source/WebKit/ChangeLog:15
> +        and iOS 11 and above. Weak link against SafariSafeBrowsing framework
> +        because it is not present on the Base system.

Maybe it should be present on the base system. Scammers target people looking for technical support.

[Alexey Proskuryakov]

Comment on attachment 331827 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=331827&action=review

> Source/WebKit/Configurations/WebKit.xcconfig:93
> +WK_SAFE_BROWSING_LDFLAGS_MACOS_SINCE_1013 = -weak_framework SafariSafeBrowsing;

This should also be conditional on 64-bit, see rdar://problem/36964995.

> WebKitLibraries/WebKitPrivateFrameworkStubs/iOS/11/SafariSafeBrowsing.framework/SafariSafeBrowsing.tbd:7
> +  - armv7
> +  - armv7s
> +  - arm64
> +  - i386
> +  - x86_64

This doesn't seem accurate.