Bug 162962

Summary: [SOUP] Ensure NetworkProcess is ready before whitelisting TLS certificates
Product: WebKit Reporter: Emanuele Aina <emanuele.aina>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, cgarcia, fpizlo, ggaren, mcatanzaro, sam, simon.fraser
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch achristensen: review+, mcatanzaro: commit-queue-

Description Emanuele Aina 2016-10-05 02:09:23 PDT
If the API user tries to whitelist TLS certificates before any web view has been created, the action will be ignored because the NetworkProcess hasn't been fired up yet.

For example, the snippet below using the GTK+ API does not work, unless the whitelisting is moved after the web view creation:
                                                                                                                        
    webkit_web_context_allow_tls_certificate_for_host(webkit_web_context_get_default(), crt, host);
    webView = webkit_web_view_new();
Comment 1 Emanuele Aina 2016-10-05 02:11:28 PDT
Created attachment 290698 [details]
Patch
Comment 2 Alex Christensen 2016-10-05 08:42:51 PDT
I'm not opposed to this change, but long-term we're hoping to get rid of it.  See discussion in https://bugs.webkit.org/show_bug.cgi?id=162910
Is it still wanted?
Comment 3 Emanuele Aina 2016-10-05 15:59:45 PDT
I just stumbled on this unexpected behaviour where the whitelisting silently failed, I'll leave any other consideration to Michael and Carlos. :)
Comment 4 Alex Christensen 2016-10-05 16:01:05 PDT
Comment on attachment 290698 [details]
Patch

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

r=me, but hopefully we'll remove this whole feature soon.  You can decide if you want to land it.

> w/Source/WebKit2/UIProcess/WebProcessPool.cpp:1149
> +    ASSERT(m_networkProcess);

not necessary
Comment 5 Michael Catanzaro 2016-10-05 16:38:39 PDT
We can't remove the feature, it's part of our API and we intend to support it indefinitely. What we can do is reimplement the feature at the GTK API layer using whatever UI process callbacks get added, then it can be removed from the cross-platform WebKit2 code.
Comment 6 Michael Catanzaro 2016-10-05 16:46:36 PDT
Comment on attachment 290698 [details]
Patch

Indeed, the ASSERT is not helpful here.

This looks easy to test, so if you don't want to risk it breaking in the next refactoring, it would be nice if you could try to add a test for it in TestSSL.cpp. The only trick is that you can't use the nice SSLTest class since that doesn't give you the ability to use the WebKitWebContext before the WebKitWebView is created; you'd have to roll your own that inherits from Test rather than LoadTrackingTest/WebViewTest.
Comment 7 Michael Catanzaro 2016-11-19 17:07:15 PST
Emanuele, I think you should commit this (without the ASSERT) even if you don't have time to write a test.
Comment 8 Michael Catanzaro 2016-12-23 09:11:26 PST
Did this get forgotten?
Comment 9 Michael Catanzaro 2016-12-27 09:57:50 PST
Let's land this.
Comment 10 Michael Catanzaro 2016-12-27 09:58:08 PST
Committed r210180: <http://trac.webkit.org/changeset/210180>
Comment 11 Emanuele Aina 2017-02-02 03:30:25 PST
I tried to come up with a test, but then I failed to find the time to finish it. Thank you for taking care of the patch!