Bug 136669 - REGRESSION(r173423): CertificateInfo is never sent to the UI process when using shared secondary process model
Summary: REGRESSION(r173423): CertificateInfo is never sent to the UI process when usi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-09 02:16 PDT by Carlos Alberto Lopez Perez
Modified: 2014-09-12 08:42 PDT (History)
9 users (show)

See Also:


Attachments
Patch (18.47 KB, patch)
2014-09-12 05:27 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased to apply on current git master (18.45 KB, patch)
2014-09-12 05:34 PDT, Carlos Garcia Campos
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2014-09-09 02:16:49 PDT
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
Comment 1 Antti Koivisto 2014-09-09 02:36:40 PDT
We don't include certificate info to subresources anymore. Maybe just the assert needs fixing?
Comment 2 Carlos Alberto Lopez Perez 2014-09-09 12:27:22 PDT
(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
Comment 3 Carlos Garcia Campos 2014-09-11 01:54:00 PDT
(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.
Comment 4 Carlos Garcia Campos 2014-09-11 02:52:58 PDT
The problem is that only the network process calls ResourceResponse::includeCertificateInfo(), and our test is using the single shared secondary process model.
Comment 5 Antti Koivisto 2014-09-11 05:08:13 PDT
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.
Comment 6 Carlos Garcia Campos 2014-09-11 05:14:43 PDT
(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.
Comment 7 Antti Koivisto 2014-09-11 05:20:18 PDT
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.
Comment 8 Carlos Garcia Campos 2014-09-11 05:24:55 PDT
(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
Comment 9 Carlos Garcia Campos 2014-09-12 05:27:35 PDT
Created attachment 238023 [details]
Patch
Comment 10 Carlos Garcia Campos 2014-09-12 05:34:47 PDT
Created attachment 238024 [details]
Rebased to apply on current git master
Comment 11 Antti Koivisto 2014-09-12 06:46:17 PDT
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.
Comment 12 Carlos Garcia Campos 2014-09-12 08:39:54 PDT
(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
Comment 13 Carlos Garcia Campos 2014-09-12 08:42:28 PDT
Committed r173558: <http://trac.webkit.org/changeset/173558>