Bug 185273 – Use SafeBrowsing in WKWebView

• Component: Page Loading
• Status: RESOLVED CONFIGURATION CHANGED

[Ali Juma]

Use the SafeBrowsing SPI added in bug 181804 to check URLs before loading them.

[Ali Juma]

Created attachment 339558 [details]
WIP

[Ali Juma]

(In reply to Ali Juma from comment #1)
> Created attachment 339558 [details]
> WIP

The main things that still need to be done here:

1) Add tests.

2) Make the warning page more detailed (i.e., match Safari's warning page), with different content for different warning types.

3) Figure out what needs to be done to make text on the warning page localizable (that is, where to put the strings -- maybe in LocalizedStringsCocoa.mm?).

4) Decide on a persistence model for user decisions to bypass warning -- should they persist for that URL for the lifetime of the WKWebView, or only for back/forward navigation to the same item, or not persist at all.

[Ali Juma]

Created attachment 339580 [details]
WIP

Rebased

[Ali Juma]

Created attachment 339730 [details]
WIP

Try to fix Sierra build

[Ali Juma]

Created attachment 340500 [details]
WIP

Fixed back/forward navigation to/from a warning page

[Ali Juma]

(In reply to Ali Juma from comment #2)
> (In reply to Ali Juma from comment #1)
> > Created attachment 339558 [details]
> > WIP
> 
> The main things that still need to be done here:
> 
> 1) Add tests.
> 
> 2) Make the warning page more detailed (i.e., match Safari's warning page),
> with different content for different warning types.
> 
> 3) Figure out what needs to be done to make text on the warning page
> localizable (that is, where to put the strings -- maybe in
> LocalizedStringsCocoa.mm?).
> 
> 4) Decide on a persistence model for user decisions to bypass warning --
> should they persist for that URL for the lifetime of the WKWebView, or only
> for back/forward navigation to the same item, or not persist at all.

Another thing that still needs to be done:

5) Figure out how this feature interacts with process-swap-on-navigation. In particular, make sure swapping works as expected when navigating to/from a SafeBrowsing warning, and when bypassing a warning.

[Ali Juma]

Created attachment 340514 [details]
WIP

Rebased

[Ali Juma]

Created attachment 340519 [details]
WIP

Fix Release builds

[Ali Juma]

Created attachment 341674 [details]
WIP

More back/forward list fixes

[EWS Watchlist]

Attachment 341674 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/FrameLoader.cpp:1456:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ali Juma]

Created attachment 341677 [details]
WIP

Fix style

[Ali Juma]

Created attachment 342442 [details]
WIP

Add tests

[Ali Juma]

Created attachment 342444 [details]
WIP

Fix build

[Ali Juma]

Created attachment 342446 [details]
WIP

Fix build

[Ali Juma]

Created attachment 342459 [details]
WIP

More build fixes

[Ali Juma]

Created attachment 342555 [details]
WIP

[Ali Juma]

Created attachment 342591 [details]
WIP

[Ali Juma]

Created attachment 342674 [details]
WIP

[EWS Watchlist]

Comment on attachment 342674 [details]
WIP

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

New failing tests:
js/dom/JSON-stringify.html

[EWS Watchlist]

Created attachment 342682 [details]
Archive of layout-test-results from ews112 for mac-sierra

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

[Chris Dumez]

As explained offline, to facilitate landing the change, we have the following suggestions:
1. Do not include any new API in the first patch (i.e. no client delegate)
2. Do not try and fix history just yet
3. I believe Alex might have some concerns about the global SafeBrowsingContextProvider too? Alex, could you please clarify?

This should keep the patch a lot simpler and facilitate its review / prompt landing. We can build on it in follow-up patches.

[Ali Juma]

Created attachment 342977 [details]
Patch

[Ali Juma]

(In reply to Chris Dumez from comment #21)
> As explained offline, to facilitate landing the change, we have the
> following suggestions:
> 1. Do not include any new API in the first patch (i.e. no client delegate)

Done, removed the client delegate method.

> 2. Do not try and fix history just yet

Removed too.

[Alex Christensen]

(In reply to Chris Dumez from comment #21)
> 3. I believe Alex might have some concerns about the global
> SafeBrowsingContextProvider too? Alex, could you please clarify?
Yes, right now it's being used to hold a global static boolean that is set via an instance method with no corresponding accessor.  We should really avoid global things in the WKWebView API design because they prevent us from having multiple WKWebViews with different settings.

[EWS Watchlist]

Comment on attachment 342977 [details]
Patch

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

New failing tests:
accessibility/mac/selection-notification-focus-change.html

[EWS Watchlist]

Created attachment 343043 [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

[Ali Juma]

Created attachment 343090 [details]
Patch

[Ali Juma]

(In reply to Alex Christensen from comment #24)
> (In reply to Chris Dumez from comment #21)
> > 3. I believe Alex might have some concerns about the global
> > SafeBrowsingContextProvider too? Alex, could you please clarify?
> Yes, right now it's being used to hold a global static boolean that is set
> via an instance method with no corresponding accessor.  We should really
> avoid global things in the WKWebView API design because they prevent us from
> having multiple WKWebViews with different settings.

Good point, moved the boolean to WKWebViewConfiguration.

[Ali Juma]

Created attachment 344296 [details]
patch

Rebased

[Ali Juma]

Created attachment 344297 [details]
Patch

Rebased

[Radar WebKit Bug Importer]

<rdar://problem/58363818>

[Brent Fulgham]

This was shipped in WebKit -- I'm not sure why this bug wasn't updated to reflect that.

[Alex Christensen]

Comment on attachment 344297 [details]
Patch

This was done elsewhere.