Bug 181804 - Expose Safe Browsing SPI
Summary: Expose Safe Browsing SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-18 10:54 PST by Zach Li
Modified: 2018-01-28 11:33 PST (History)
11 users (show)

See Also:


Attachments
Patch (10.80 KB, patch)
2018-01-18 11:25 PST, Zach Li
achristensen: review-
Details | Formatted Diff | Diff
Patch v2 (12.00 KB, patch)
2018-01-18 18:19 PST, Zach Li
no flags Details | Formatted Diff | Diff
Patch v3 (12.03 KB, patch)
2018-01-19 14:12 PST, Zach Li
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.37 MB, application/zip)
2018-01-19 16:23 PST, EWS Watchlist
no flags Details
Patch v3 (12.07 KB, patch)
2018-01-19 16:31 PST, Zach Li
no flags Details | Formatted Diff | Diff
Patch v3 (12.10 KB, patch)
2018-01-19 17:55 PST, Zach Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zach Li 2018-01-18 10:54:34 PST
Expose Safe Browsing SPI
Comment 1 Zach Li 2018-01-18 11:25:30 PST
Created attachment 331644 [details]
Patch
Comment 2 Alex Christensen 2018-01-18 11:29:49 PST
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?
Comment 3 Zach Li 2018-01-18 11:37:13 PST
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?
Comment 4 Alex Christensen 2018-01-18 11:41:17 PST
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 5 mitz 2018-01-18 11:47:56 PST
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;
Comment 6 Alex Christensen 2018-01-18 11:53:30 PST
Comment on attachment 331644 [details]
Patch

Great!  Please adopt Dan's new future-proof way of doing this.
Comment 7 Zach Li 2018-01-18 12:05:48 PST
(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.
Comment 8 Zach Li 2018-01-18 12:07:14 PST
(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.
Comment 9 Zach Li 2018-01-18 18:19:54 PST
Created attachment 331687 [details]
Patch v2
Comment 10 WebKit Commit Bot 2018-01-19 10:05:45 PST
Comment on attachment 331687 [details]
Patch v2

Clearing flags on attachment: 331687

Committed r227211: <https://trac.webkit.org/changeset/227211>
Comment 11 WebKit Commit Bot 2018-01-19 10:05:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Lucas Forschler 2018-01-19 10:23:02 PST
<rdar://problem/36626946>
Comment 13 Ryan Haddad 2018-01-19 11:39:29 PST
(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
Comment 14 Ryan Haddad 2018-01-19 11:40:17 PST
(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
Comment 15 Ryan Haddad 2018-01-19 11:47:08 PST
Reverted r227211 for reason:

Breaks iOS Simulator tests.

Committed r227224: <https://trac.webkit.org/changeset/227224>
Comment 16 Zach Li 2018-01-19 14:00:50 PST
Dan helped me figure out I should include "current-version: 0.0.0" in SafariSafeBrowsing.tbd file.
Comment 17 Zach Li 2018-01-19 14:12:54 PST
Created attachment 331784 [details]
Patch v3
Comment 18 EWS Watchlist 2018-01-19 16:23:55 PST
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
Comment 19 EWS Watchlist 2018-01-19 16:23:56 PST
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
Comment 20 Zach Li 2018-01-19 16:31:13 PST
Created attachment 331813 [details]
Patch v3
Comment 21 Zach Li 2018-01-19 17:55:45 PST
Created attachment 331827 [details]
Patch v3
Comment 22 WebKit Commit Bot 2018-01-19 22:40:37 PST
Comment on attachment 331827 [details]
Patch v3

Clearing flags on attachment: 331827

Committed r227265: <https://trac.webkit.org/changeset/227265>
Comment 23 WebKit Commit Bot 2018-01-19 22:40:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Geoffrey Garen 2018-01-22 09:59:55 PST
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.
Comment 25 Alexey Proskuryakov 2018-01-28 11:33:14 PST
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.