Bug 188133 – Check with SafeBrowsing during navigation in WKWebView

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Implement safe browsing in WKWebView

[Alex Christensen]

Created attachment 345975 [details]
Patch

[Alex Christensen]

Created attachment 345976 [details]
Patch

[EWS Watchlist]

Comment on attachment 345976 [details]
Patch

Attachment 345976 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8685614

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 346003 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Alex Christensen]

Created attachment 346297 [details]
Patch

[Alex Christensen]

Created attachment 346299 [details]
Patch

[Alex Christensen]

Created attachment 346301 [details]
Patch

[Alex Christensen]

Created attachment 346305 [details]
Patch

[Alex Christensen]

Created attachment 346309 [details]
Patch

[Alex Christensen]

Created attachment 346388 [details]
Patch

[Chris Dumez]

Comment on attachment 346388 [details]
Patch

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

r=me with suggestion.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4152
> +    listener->didReceiveSafeBrowsingResults({ });

This looks hackish/fragile. Is this temporary? I think I'd much prefer we pass a ShouldExpectSafeBrowsingResult { No, Yes } parameter.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4189
> +    listener->didReceiveSafeBrowsingResults({ });

ditto.

[Alex Christensen]

Created attachment 346398 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r234513

[Radar WebKit Bug Importer]

<rdar://problem/42869909>

[Ryan Haddad]

14 API tests are failing the assert(s) added with this change:

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/4338/steps/run-api-tests/logs/stdio
    TestWebKitAPI.WebKit.DecidePolicyForNavigationActionForPOSTFormSubmissionThatRedirectsToPOST
        ASSERTION FAILED: !m_policyResult
        /Volumes/Data/slave/highsierra-debug/build/Source/WebKit/UIProcess/WebFramePolicyListenerProxy.cpp(77) : void WebKit::WebFramePolicyListenerProxy::ignore()
        1   0x106c661f9 WTFCrash
        2   0x10b74632b WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x10c1cfd47 WebKit::WebFramePolicyListenerProxy::ignore()
        4   0x10c1d13f4 WebKit::WebFrameProxy::webProcessWillShutDown()
        5   0x10c6448fd WebKit::WebProcessProxy::disconnectFramesFromPage(WebKit::WebPageProxy*)
        6   0x10c3b1a30 WebKit::WebPageProxy::close()
        7   0x10c90f0ea -[WKWebView dealloc]
        8   0x7fff6bf6b042 (anonymous namespace)::AutoreleasePoolPage::pop(void*)
        9   0x7fff447a88e6 _CFAutoreleasePoolPop
        10  0x7fff468e68ad -[NSAutoreleasePool drain]
        11  0x1061d9cd7 main
        12  0x7fff6cb8c015 start

[Ryan Haddad]

Reverted r234513 for reason:

14 API tests fail the assertions added in this change.

Committed r234545: <https://trac.webkit.org/changeset/234545>

[Ryan Haddad]

Assertion failures were also seen with the following tests on High Sierra:

fast/events/open-window-from-another-frame.html
fast/events/popup-blocked-from-fake-user-gesture.html
http/tests/xmlhttprequest/access-control-response-with-body-sync.html

This test started failing:
http/tests/navigation/keyboard-events-during-provisional-navigation.html

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r234541%20(4349)/results.html

[Alex Christensen]

Recommitted without those assertions in http://trac.webkit.org/r234552
The assertions are not valid because ignore is called during teardown even if the listener has already been used.  See r234462 for a change motivated by the same reason.

[Ryan Haddad]

(In reply to Alex Christensen from comment #18)
> Recommitted without those assertions in http://trac.webkit.org/r234552

http/tests/navigation/keyboard-events-during-provisional-navigation.html is failing again after the commit. It is not clear to me if the new console output is expected or not.

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/navigation/keyboard-events-during-provisional-navigation-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/navigation/keyboard-events-during-provisional-navigation-actual.txt
@@ -10,6 +10,8 @@
 CONSOLE MESSAGE: line 55: Pasting text "d".
 CONSOLE MESSAGE: line 58: Input element value after text input events: "".
 CONSOLE MESSAGE: line 20: Pressing "z" with access key modifiers should navigate to resources/keyboard-events-after-navigation.html.
+CONSOLE MESSAGE: line 18: keydownevent dispatched (isTrusted: true).
+CONSOLE MESSAGE: line 18: keyupevent dispatched (isTrusted: true).

https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r234559%20(5991)/results.html

[Truitt Savell]

(In reply to Ryan Haddad from comment #19)
> (In reply to Alex Christensen from comment #18)
> > Recommitted without those assertions in http://trac.webkit.org/r234552
> 
> http/tests/navigation/keyboard-events-during-provisional-navigation.html is
> failing again after the commit. It is not clear to me if the new console
> output is expected or not.
> 
> ---
> /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> http/tests/navigation/keyboard-events-during-provisional-navigation-expected.
> txt
> +++
> /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> http/tests/navigation/keyboard-events-during-provisional-navigation-actual.
> txt
> @@ -10,6 +10,8 @@
>  CONSOLE MESSAGE: line 55: Pasting text "d".
>  CONSOLE MESSAGE: line 58: Input element value after text input events: "".
>  CONSOLE MESSAGE: line 20: Pressing "z" with access key modifiers should
> navigate to resources/keyboard-events-after-navigation.html.
> +CONSOLE MESSAGE: line 18: keydownevent dispatched (isTrusted: true).
> +CONSOLE MESSAGE: line 18: keyupevent dispatched (isTrusted: true).
> 
> https://build.webkit.org/results/
> Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r234559%20(5991)/results.html

svg/custom/anchor-on-use.svg Is also failing after this revision with the failure first happening after https://trac.webkit.org/changeset/234513/webkit. 

Image Diff:
https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r234600%20(4376)/svg/custom/anchor-on-use-diffs.html

[Ryan Haddad]

Reverted r234552 for reason:

Introduced 2 layout test failures on High Sierra.

Committed r234615: <https://trac.webkit.org/changeset/234615>

[Alex Christensen]

Created attachment 346669 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 346669 [details]
Patch

Clearing flags on attachment: 346669

Committed r234640: <https://trac.webkit.org/changeset/234640>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

After <https://trac.webkit.org/changeset/234640> 

We have API assertion failures again. 

API failure:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK1%20%28Tests%29/builds/5055/steps/run-api-tests/logs/stdio

Looks like those were not omitted in this patch. additionally after the rebaselining of  http/tests/navigation/keyboard-events-during-provisional-navigation.htmlthe test is failing in Sierra WK2. Looks like it just needs to be baselined on that build as well.

Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fnavigation%2Fkeyboard-events-during-provisional-navigation.html

As well as an internal build is broken.

[Alex Christensen]

http://trac.webkit.org/r234658 should make everything good.