Bug 136669 – REGRESSION(r173423): CertificateInfo is never sent to the UI process when using shared secondary process model

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

The GTK API test WebKit2Gtk/TestSSL is failing since r173423 <http://trac.webkit.org/r173423>

$ Tools/Scripts/run-gtk-tests --verbose --release WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestSSL 
TEST: WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestSSL... (pid=32700)
  /webkit2/WebKitWebView/ssl:                                          **
ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestSSL.cpp:72:void testSSL(SSLTest*, gconstpointer): assertion failed: (test->m_certificate)
FAIL
GTester: last random seed: R02Sc9ca19c165138113981a6793268440f9
(pid=312)
  /webkit2/WebKitWebView/insecure-content:                             OK
  /webkit2/WebKitWebView/tls-errors-policy:                            OK
  /webkit2/WebKitWebView/tls-errors-redirect-to-http:                  OK
  /webkit2/WebKitWebView/load-failed-with-tls-errors:                  OK
FAIL: WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestSSL
Tests failed (1): WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestSSL

[Antti Koivisto]

We don't include certificate info to subresources anymore. Maybe just the assert needs fixing?

[Carlos Alberto Lopez Perez]

(In reply to comment #1)
> We don't include certificate info to subresources anymore. Maybe just the assert needs fixing?

Maybe.

I guess that cgarcia can check better than me if this is the case.

In the meanwhile. I skipped the test on http://trac.webkit.org/changeset/173435

[Carlos Garcia Campos]

(In reply to comment #1)
> We don't include certificate info to subresources anymore. Maybe just the assert needs fixing?

No, we are not testing the certificate of a subresource, there aren't even subresources in the html of the test. What we are testing here is that the view has a certifciate once the load has been committed, and we use mainFrame->certificateInfo()->certificateInfo() for that.

[Carlos Garcia Campos]

The problem is that only the network process calls ResourceResponse::includeCertificateInfo(), and our test is using the single shared secondary process model.

[Antti Koivisto]

Ah ok. Would it be difficult for you to move to using network process everywhere? Eliminating WK2-without-network-process mode would simplify the code and avoid this sort of issues.

[Carlos Garcia Campos]

(In reply to comment #5)
> Ah ok. Would it be difficult for you to move to using network process everywhere? Eliminating WK2-without-network-process mode would simplify the code and avoid this sort of issues.

Yes, I'm afraid so, we have the single shared secondary process model exposed in our API, and used by applications. I think it's the best model for most applications, except web browsers. Are there any plans to remove that model? 
Would it be possible to implement this in WebCore by adding a ResourceLoaderOption? a CertificateInfoPolicy that will be only set as IncludeCertificateInfo for the main resource loader.

[Antti Koivisto]

I don't think there are any plans to remove it. Having less configurations to support would be nice though.

Yeah, solution along those lines makes sense.

[Carlos Garcia Campos]

(In reply to comment #7)
> I don't think there are any plans to remove it. Having less configurations to support would be nice though.

phew, good to know, thanks :-)

> Yeah, solution along those lines makes sense.

Ok, I'll try that approach then

[Carlos Garcia Campos]

Created attachment 238023 [details]
Patch

[Carlos Garcia Campos]

Created attachment 238024 [details]
Rebased to apply on current git master

[Antti Koivisto]

Comment on attachment 238024 [details]
Rebased to apply on current git master

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

> Source/WebCore/loader/DocumentLoader.cpp:1435
> +    static NeverDestroyed<ResourceLoaderOptions> mainResourceLoadOptions(SendCallbacks, SniffContent, BufferData, AllowStoredCredentials, AskClientForAllCredentials, SkipSecurityCheck, UseDefaultOriginRestrictionsForType, IncludeCertificateInfo);

It would be nice to refactor ResourceLoaderOptions to be in the style of ViewState::Flags and LayerFlushThrottleState::Flags. The current struct is very expansion unfriendly.

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 238024 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238024&action=review

Thanks for the review.

> > Source/WebCore/loader/DocumentLoader.cpp:1435
> > +    static NeverDestroyed<ResourceLoaderOptions> mainResourceLoadOptions(SendCallbacks, SniffContent, BufferData, AllowStoredCredentials, AskClientForAllCredentials, SkipSecurityCheck, UseDefaultOriginRestrictionsForType, IncludeCertificateInfo);
> 
> It would be nice to refactor ResourceLoaderOptions to be in the style of ViewState::Flags and LayerFlushThrottleState::Flags. The current struct is very expansion unfriendly.

Definitely

[Carlos Garcia Campos]

Committed r173558: <http://trac.webkit.org/changeset/173558>