Bug 188871 – Implement safe browsing in WebKit

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Implement safe browsing in WebKit

[Alex Christensen]

Created attachment 347880 [details]
Patch

[Alex Christensen]

Created attachment 347888 [details]
Patch

[Alex Christensen]

Created attachment 347904 [details]
Patch

[Sam Weinig]

Comment on attachment 347904 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:4045
> +            loadAlternateHTML({ reinterpret_cast<const uint8_t*>(buffer->data()), buffer->size() }, "UTF-8"_s, navigation->currentRequest().url(), navigation->currentRequest().url(), nullptr, true);

It seems wrong that WebKit would show an error page here rather than leaving that up to the client to do (like we do with all other page load errors).  I would expect a safe browsing failure to just be another type of error returned to the client.

Comment on attachment 347904 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4045
>> +            loadAlternateHTML({ reinterpret_cast<const uint8_t*>(buffer->data()), buffer->size() }, "UTF-8"_s, navigation->currentRequest().url(), navigation->currentRequest().url(), nullptr, true);
> 
> It seems wrong that WebKit would show an error page here rather than leaving that up to the client to do (like we do with all other page load errors).  I would expect a safe browsing failure to just be another type of error returned to the client.

Does this introduce a situation in which, unbeknownst to the client, the WKWebView’s URL property reports a certain URL, but the view is displaying content that wasn’t loaded from that URL?

[Alex Christensen]

Comment on attachment 347904 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:4045
>>> +            loadAlternateHTML({ reinterpret_cast<const uint8_t*>(buffer->data()), buffer->size() }, "UTF-8"_s, navigation->currentRequest().url(), navigation->currentRequest().url(), nullptr, true);
>> 
>> It seems wrong that WebKit would show an error page here rather than leaving that up to the client to do (like we do with all other page load errors).  I would expect a safe browsing failure to just be another type of error returned to the client.
> 
> Does this introduce a situation in which, unbeknownst to the client, the WKWebView’s URL property reports a certain URL, but the view is displaying content that wasn’t loaded from that URL?

It does.  For the record, I have also argued that this ought to be a new error type, so I'm not the person to argue against your views which I see as correct.

Comment on attachment 347904 [details]
Patch

The WebKit project works best when we only make changes that at least one author and one reviewer deem to be correct.

[Alex Christensen]

Comment on attachment 347904 [details]
Patch

I'm going to take a different approach.

[Alex Christensen]

Created attachment 349327 [details]
Patch

[Alex Christensen]

Created attachment 349334 [details]
Patch

[Alex Christensen]

Created attachment 349462 [details]
Patch

[Sam Weinig]

Comment on attachment 349462 [details]
Patch

This new patch doesn't seem to address the concerns raised earlier in this bug.

[Alex Christensen]

Comment on attachment 349462 [details]
Patch

This patch does address the concerns from earlier in this bug.
With the new delegate callback in WKUIDelegate, an app will have the ability to see and control whether a safe browsing warning is being shown.
Because loading an unsafe URL in an iframe is determined to indicate that the whole page should not be shown, we cannot use WKNavigationDelegate's existing error callbacks because they are not called for iframe navigation.
Because we want unmodified clients to get this safety coverage but still have the ability to not be broken, this is the design.  Re-requesting review.

As noted before, the patch introduces new meaning to WKWebView’s URL property for clients that were built with an SDK in which it had a different meaning.

[Sam Weinig]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 349462 [details]
> Patch
> 
> This patch does address the concerns from earlier in this bug.
> With the new delegate callback in WKUIDelegate, an app will have the ability
> to see and control whether a safe browsing warning is being shown.
> Because loading an unsafe URL in an iframe is determined to indicate that
> the whole page should not be shown, we cannot use WKNavigationDelegate's
> existing error callbacks because they are not called for iframe navigation.
> Because we want unmodified clients to get this safety coverage but still
> have the ability to not be broken, this is the design.  Re-requesting review.

I think you may have misunderstood my concern. My specific stated concern was that the design up until this point has been to require clients to implement their own error pages, the idea being that only the client could design something that matches the UI of their application. This was a deliberate choice when designing the API. As such, it seems like a better approach would be to return an error to the client and allow them to construct an error page.

[Geoffrey Garen]

> As such, it seems like a better
> approach would be to return an error to the client and allow them to
> construct an error page.

What default behavior are you proposing for clients that don't design and implement their own error pages?

(In reply to Geoffrey Garen from comment #16)
> What default behavior are you proposing for clients that don't design and
> implement their own error pages?

I’m no Sam Weinig, but I would propose the same behavior that clients that don’t design and implement their own “untrustworthy TLS certificate” error pages have: the navigation fails (if it’s a main-frame navigation, the navigation delegate receives an error).

[Alex Christensen]

I'm all for that, but I ran up against a problem: if it's an iframe navigating to an unsafe URL, there's no API on WKWebView to tell the application about the error.  didFailProvisionalNavigation and didFailNavigation are only called for top level navigations.  Do you think it would be better to just unexpectedly call didStartProvisionalNavigation and didFailProvisionalNavigation even when the WKFrameInfos of the decidePolicyForNavigationAction call said it was not for a main frame navigation?  I could, but that would also be changing the meaning of the API.

(In reply to Alex Christensen from comment #18)
> I'm all for that, but I ran up against a problem: if it's an iframe
> navigating to an unsafe URL, there's no API on WKWebView to tell the
> application about the error.

Right, this mirrors what happens when an iframe navigates to a URL with an untrustworthy certificate.

> Do you think
> it would be better to just unexpectedly call didStartProvisionalNavigation
> and didFailProvisionalNavigation even when the WKFrameInfos of the
> decidePolicyForNavigationAction call said it was not for a main frame
> navigation?

This doesn’t strike me as a good design, and like you say, it’s not a binary-compatible change.

There is private API for notifying the delegate of errors in subframes, which is used by Safari in iOS for the “untrustworthy certificate in subframe navigation” case. Perhaps this private API is a good starting point for public API for communicating subframe navigation errors.

[Geoffrey Garen]

(In reply to mitz from comment #17)
> (In reply to Geoffrey Garen from comment #16)
> > What default behavior are you proposing for clients that don't design and
> > implement their own error pages?
> 
> I’m no Sam Weinig, but I would propose the same behavior that clients that
> don’t design and implement their own “untrustworthy TLS certificate” error
> pages have: the navigation fails (if it’s a main-frame navigation, the
> navigation delegate receives an error).

OK, now that I understand this proposal, I see that it may be aesthetically pleasing and similar to existing behaviors -- maybe that's why Sam called it "better"? -- but it's also a security regression compared to the behavior in this patch (and the behavior Safari currently implements).

The Safe Browsing service can detect that a website is a phishing attempt at any time -- before, during, or after the load. When the Safe Browsing service flags a subframe or subresource, that doesn't mean "this resource is phishing, but everything else is OK". Instead it means "because of this resource, we believe the whole webpage is a phishing attempt". 

So, responding to a flagged subframe or subresource by just neglecting to load it would leave the user exposed to the phishing attack in the main frame.

Can you modify your proposal in some way to resolve this problem?

(In reply to Geoffrey Garen from comment #20)
> (In reply to mitz from comment #17)
> > (In reply to Geoffrey Garen from comment #16)
> > > What default behavior are you proposing for clients that don't design and
> > > implement their own error pages?
> > 
> > I’m no Sam Weinig, but I would propose the same behavior that clients that
> > don’t design and implement their own “untrustworthy TLS certificate” error
> > pages have: the navigation fails (if it’s a main-frame navigation, the
> > navigation delegate receives an error).
> 
> OK, now that I understand this proposal, I see that it may be aesthetically
> pleasing and similar to existing behaviors -- maybe that's why Sam called it
> "better"? -- but it's also a security regression compared to the behavior in
> this patch (and the behavior Safari currently implements).
> 
> The Safe Browsing service can detect that a website is a phishing attempt at
> any time -- before, during, or after the load. When the Safe Browsing
> service flags a subframe or subresource, that doesn't mean "this resource is
> phishing, but everything else is OK". Instead it means "because of this
> resource, we believe the whole webpage is a phishing attempt". 
> 
> So, responding to a flagged subframe or subresource by just neglecting to
> load it would leave the user exposed to the phishing attack in the main
> frame.
> 
> Can you modify your proposal in some way to resolve this problem?

Since, as you explained, being unsafe-to-browse is not a characteristic of a navigation but rather a state that the web view enters, a good binary-compatible way to present this state to clients would be to mimic one of these existing error states: Web Content process hang and Web Content process termination.

[Geoffrey Garen]

> Since, as you explained, being unsafe-to-browse is not a characteristic of a
> navigation but rather a state that the web view enters, a good
> binary-compatible way to present this state to clients would be to mimic one
> of these existing error states: Web Content process hang and Web Content
> process termination.

I think something like the process termination API could fit here. In that case, what user-facing behavior would you propose?

[Alex Christensen]

Created attachment 349793 [details]
Patch

[Alex Christensen]

Created attachment 350060 [details]
Patch

[Alex Christensen]

Created attachment 350071 [details]
Patch

(In reply to Geoffrey Garen from comment #22)
> > Since, as you explained, being unsafe-to-browse is not a characteristic of a
> > navigation but rather a state that the web view enters, a good
> > binary-compatible way to present this state to clients would be to mimic one
> > of these existing error states: Web Content process hang and Web Content
> > process termination.
> 
> I think something like the process termination API could fit here. In that
> case, what user-facing behavior would you propose?

The difference between initiating a main-frame navigation to an unsafe URL and learning that the current page has become unsafe is significant enough, that they should be presented differently to existing clients.

I would propose that a main-frame navigation to an unsafe URL will appear to have failed prior to being committed. The user-facing behavior depends on how the client handles navigation errors (or not) so it can range from nothing happening through the app presenting a modal alert to the app loading an HTML error page.

The current page becoming unsafe, while currently can only be triggered by a main frame navigation, is not really a navigational concept. It’s conceivable that in the future it might be triggered by any sub-resource load or even by analysis of something the page is doing with the DOM or with style. Once this condition is triggered, it’s unsafe for the user to interact with the webpage. The user-facing behavior, again, will depend on whether and how the client handles -webViewWebContentProcessDidTerminate: so it can range from the view just going blank through the app presenting a modal alert, or reloading the view with or without showing banner, or reloading the view repeatedly, or navigating to an HTML error page. Whatever it is, it’s something the app and its users are should be familiar with from experiencing Web Content process crashes.

I would propose that those behaviors for existing clients also apply to clients building against the future SDK, as long as they don’t bother to adopt any new WebKit API related to safe browsing.

The above behaviors also inform what such new API might look like, because it will remain important for clients to distinguish between “a bad navigation could happen, or we could stop it” and “the currently committed webpage is now known to be unsafe”. The former is best presented to clients as a new error from the existing navigation delegate method. The latter is better presented via a new API similar to -webViewWebContentProcessDidTerminate:, which requires them to take specific action prior to returning if they wish to avoid the default behavior of blanking-the-web-view-as-if-the-process-has-crashed.

[Alex Christensen]

Created attachment 350738 [details]
Patch

[Chris Dumez]

Comment on attachment 350738 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:4077
> +                    case UnsafeContentDecision::YepItsUnsafe:

YepItsUnsafe? really?

> Source/WebKit/UIProcess/WebPageProxy.cpp:4085
> +                    case UnsafeContentDecision::IThinkItsActuallySafe:

IThinkItsActuallySafe ?

> Source/WebKit/UIProcess/API/APINavigationClient.h:61
> +enum class UnsafeContentDecision : uint8_t { YepItsUnsafe, IThinkItsActuallySafe };

I do not think this naming style is good or common in the project.

> Source/WebKit/UIProcess/API/Cocoa/WKNavigationDelegatePrivate.h:65
> +    _WKUnsafeContentDecisionYepItsUnsafe,

Ditto.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:477
> +        provisionalLoader = static_cast<WebDocumentLoader*>(m_frame->coreFrame()->loader().policyDocumentLoader());

We would not need this workaround if you moved the DocumentLoader from policy to provisional before calling didStartProvisionalLoad(), like we usually do, e.g.:
setProvisionalDocumentLoader(m_policyDocumentLoader.get());
setState(FrameStateProvisional);
setPolicyDocumentLoader(nullptr);
m_client.dispatchDidStartProvisionalLoad();

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4142
> +        documentLoader = frame->coreFrame()->loader().policyDocumentLoader();

Ditto

[Alex Christensen]

> YepItsUnsafe? really?
No, not really.  This is still in the design exploration phase, but I think we're closing in on what we want, and I'd be up for some name suggestions.

[Alex Christensen]

Created attachment 350808 [details]
Patch

[Alex Christensen]

Created attachment 350813 [details]
Patch

[EWS Watchlist]

Comment on attachment 350813 [details]
Patch

Attachment 350813 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9350684

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html
editing/selection/iframe.html

[EWS Watchlist]

Created attachment 350826 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

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

[Alex Christensen]

Created attachment 350872 [details]
Patch

[Geoffrey Garen]

Here's my understanding of where we stand on this feature, and why.

# Consensus items

1. WebKit should do safe browsing checks by default. There are lots of WebKit clients and it's not practical to expect each one to adopt a safe browsing API separately.

2. Clients should be able to disable WebKit's checks, or override their outcomes. Some clients may consider it more private not to perform checks. Others might want to consult the user on the outcome.

3. loadAlternateHTML is not the best way for WebKit to present the user with an interstitial choice about safe browsing. It doesn't work for POST and other one-time loads because it cancels the load, making it impossible to bypass the warning and continue. It also invites apps that modify the DOM to accidentally modify the presentation of the interstitial choice.

4. Safe browsing is a continuous evaluation. You can decide that a webpage is unsafe at any time -- even after a page finishes loading. A simple page load error would be an incomplete implementation (though it sure would be simpler!).

# Open Questions

1. Do we need to offer a default UI to bypass a safe browsing warning? Unlike a TLS error or a network disconnection, a safe browsing warning is a trusted guesstimate and not a fact. We might want or need to maintain some humility about WebKit's guesstimates.

# Geoff says

1. It's not so great to kill the web process. Might a client display something like a crash banner if we did? It would be surprising and confusing if a user complained about a crash, and the complaint turned out to be a successful but poorly communicated identification of a flagged webpage -- if and only if the webpage were flagged because of a subframe. One alternative is to "undo" the navigation -- by going back or forward or to about:blank, depending on whether the navigation had been a forward or a back or a new WebView. Another alternative is to fail the load in v1, and retain the option to add a default interstitial UI prior to the load failure in v2 if it proves necessary. Another option is to add a default interstitial UI in v1, using a mechanism other than loadAlternateHTML.

[Alex Christensen]

(In reply to Geoffrey Garen from comment #35)

> 3. loadAlternateHTML is not the best way for WebKit to present the user with
> an interstitial choice about safe browsing. It doesn't work for POST and
> other one-time loads because it cancels the load, making it impossible to
> bypass the warning and continue. It also invites apps that modify the DOM to
> accidentally modify the presentation of the interstitial choice.

It's not about losing the POST data, but more about losing the state of the current page.  If we decide to go back to safety when doing a main frame navigation, we want to not have destroyed the state of the previous page.  Even more important, if the user or client sees that a subresource load is marked as unsafe but decides to load it anyway, we want to be able to load that subresource into a page that has not lost state or been reloaded.

[Alex Christensen]

Created attachment 352850 [details]
Patch

[Alex Christensen]

Created attachment 353655 [details]
Patch

[Alex Christensen]

Known issues with this latest patch:
1. no tests yet :(
2. WKSafeBrowsingClickableTextView.intrinsicContentSize isn't quite right, but it's good enough for all sizes
3. Not implemented on iOS yet even though it builds
4. Navigating the WKWebView should clear the interstitial.
5. Minor UI tweaks are probably needed

[Tim Horton]

Comment on attachment 353655 [details]
Patch

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

> Source/WebKit/UIProcess/PageClient.h:120
> +enum class HeedSafeBrowsingWarning : bool { No, Yes };

I don't love "Heed"

> Source/WebKit/UIProcess/WebPageProxy.cpp:4139
> +            if (result->needsInterstitialWarning()) {

Reduce indentation a little with a `if (!..) continue`?

> Source/WebKit/UIProcess/API/C/mac/WKContextPrivateMac.mm:176
> +#if PLATFORM(MAC)

What was this even here for before! Does anyone use it?

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.h:61
> +- (void)frameChangedTo:(CGRect)frame;

Why? Why not just set the frame in the place where this is called?

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:60
> +static ColorType *redColor()

You should stick all of the colors in an asset catalog with specializations for dark mode and use them by name. We have one on iOS, but you should add one that we use on both platforms.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:96
> +    return WebCore::URL({ }, "http://webkit.org/"_s);

What‽ Why webkit.org

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:146
> +    _dark = dark;

Why all this passing dark state around? Just use NSAppearance like it was made for.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:178
> +    [[NSColor windowBackgroundColor] set];

Ditto the question from below; this could just be a layer with background color and corner radius. Also does it want continuous curvature corners? Probably.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:304
> +}
> +- (void)dealloc

Lots of missing newlines

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:325
> +        NSAlert *alert = [[NSAlert alloc] init];

Leak?

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:361
> +- (void)drawRect:(CGRect)rect
> +{
> +    [redColor() set];
> +    [[BezierPathType bezierPathWithRect:rect] fill];
> +}

Why make backing store when it could just be a layer with background color!

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:365
> +- (void)frameChangedTo:(CGRect)frame
> +{
> +    [self setFrame:frame];
> +}

This is wrong. You're setting the frame of this view to the frame of the WKWebView. It's OK in Safari and MiniBrowser because Safari/MiniBrowser put the WKWebView at the origin, but in general you want to make this view (which is installed as a child of the WKWebView, right?) cover the *bounds* of the WKWebView, not its frame. But I also think as stated above that you should just set the frame of this view in the places where currently call this (frameChangedTo:) and get rid of this.

[Alex Christensen]

Comment on attachment 353655 [details]
Patch

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

Thanks for the great review, Tim!

>> Source/WebKit/UIProcess/API/C/mac/WKContextPrivateMac.mm:176
>> +#if PLATFORM(MAC)
> 
> What was this even here for before! Does anyone use it?

This was added and adopted in Safari so that one change in WebKit can enable safe browsing in WebKit and disable safe browsing in Safari at the same time so there aren't broken builds between landing two patches in two repositories.  It will be removed after this project is over.

[Tim Horton]

Smart!

[Alex Christensen]

Comment on attachment 353655 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:178
>> +    [[NSColor windowBackgroundColor] set];
> 
> Ditto the question from below; this could just be a layer with background color and corner radius. Also does it want continuous curvature corners? Probably.

NSView doesn't have backgroundColor, just UIView.

[Tim Horton]

Comment on attachment 353655 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:178
>>> +    [[NSColor windowBackgroundColor] set];
>> 
>> Ditto the question from below; this could just be a layer with background color and corner radius. Also does it want continuous curvature corners? Probably.
> 
> NSView doesn't have backgroundColor, just UIView.

I said layer! Which you can make a view have by setting wantsLayer, and ten grab it via ‘layer’ and set a background color

[Alex Christensen]

Created attachment 353711 [details]
Patch

[Wenson Hsieh]

Comment on attachment 353711 [details]
Patch

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

> Source/WebKit/UIProcess/SafeBrowsingResult.h:56
> +    bool needsSafeBrowsingWarning() { return m_isPhishing || m_isMalware || m_isUnwantedSoftware; }

Nit - needsSafeBrowsingWarning() const?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:3282
> +    _impl->frameOrBoundsChanged();

Can we make this work using existing machinery (e.g. WebViewImpl::setFrameSize) instead?

> Source/WebKit/UIProcess/Cocoa/SafeBrowsingResultCocoa.mm:36
> +// FIXME: These ought to be API calls to the SafariSafeBrowsing framework when such SPI is available.

If there's a radar tracking this, I think it would be nice to reference it here.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:340
> +        NSFontAttributeName:[FontType systemFontOfSize:13],
> +        NSForegroundColorAttributeName : foregroundColor

Nit - spaces around the : seem inconsistent here.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1625
> +    [m_safeBrowsingWarning setFrame:[m_view frame]];
> +    [m_safeBrowsingWarning setBounds:[m_view bounds]];

🤔

[Alex Christensen]

Created attachment 353715 [details]
Patch

[Alex Christensen]

Created attachment 353740 [details]
Patch

[Alex Christensen]

Created attachment 353776 [details]
Patch

[Alex Christensen]

Created attachment 353780 [details]
Patch

[Tim Horton]

Comment on attachment 353780 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/mac/WKContextPrivateMac.mm:177
> +    return true;

Should this follow the preference?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:3282
> +    _impl->frameOrBoundsChanged();

Prettttty likely you can get rid of this and just use WebViewImpl::setFrameSize. But this is also fine I guess? Though the name is a bit of a lie.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:89
> +        NSUnderlineStyleAttributeName: [NSNumber numberWithInteger:1]

@(1)

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:90
> +    } range:NSMakeRange(0, [replaceWith length])];

More dots! 

replaceWith.length

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:102
> +    [colorNamed(@"WKSafeBrowsingWarningTitle") set];

Pretty good chance this function would look way less crazy if you just used CG, but w/e.

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:257
> +    if (_completionHandler)
> +        _completionHandler(WebKit::ContinueUnsafeLoad::Yes);

Really, continue if you're being deallocated?

[Alex Christensen]

Created attachment 353792 [details]
Patch

[Alex Christensen]

Created attachment 353870 [details]
Patch

[Tim Horton]

Comment on attachment 353870 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6657
> +- (NSView *)_safeBrowsingWarningForTesting

This obviously isn't going to fly on iOS

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:200
> ++ (instancetype)viewWithAttributedString:(NSMutableAttributedString *)attributedString linkTarget:(WKSafeBrowsingWarning *)target;

Why mutable! (Ideally there would be a mutableCopy hiding inside here, and the caller's string wouldn't get mutated)

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:295
> +    _completionHandler(makeUnexpected(link));

Alex says "I could use a variant instead of Expected"

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:302
> +    NSStackView *bottom =box.views.lastObject;

Missing a space after the =

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1609
> +            weakThis->m_ignoresNonWheelEvents = oldIgnoresNonWheelEvents;

?? do you want to allow scrolling (but not other events) ??

[Joseph Pecoraro]

Comment on attachment 353870 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WKSafeBrowsingWarning.mm:89
> +        NSUnderlineStyleAttributeName: @(1)

Or just @1

[Alex Christensen]

Created attachment 353915 [details]
Patch

[Alex Christensen]

Created attachment 353918 [details]
Patch

[Alex Christensen]

Created attachment 353928 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r237863

[Radar WebKit Bug Importer]

<rdar://problem/45842451>

[Ryan Haddad]

This change introduced an API test failure on High Sierra and Mojave:

    TestWebKitAPI.ProcessSwap.LoadAfterPolicyDecision
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:485
        Expected equality of these values:
          @"pson://www.webkit.org/main2.html"
            Which is: "pson://www.webkit.org/main2.html"
          [[webView URL] absoluteString]
            Which is: "pson://www.apple.com/main.html"

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/7662

[Alex Christensen]

Tests fixed in http://trac.webkit.org/r237876
Also, this is part of <rdar://problem/26892893>