Bug 136611

Summary: Pass certificate info as part of ResourceResponse
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, berto, cdumez, cgarcia, clopez, commit-queue, danw, gustavo, mitz, mrobinson, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch 2
none
patch 3
none
patch 4
none
patch 5
none
patch 6 darin: review+

Description Antti Koivisto 2014-09-07 11:19:20 PDT
The way we currently pull certificate info out of ResourceResponse for serialization and then put it back is confusing.
Comment 1 Alexey Proskuryakov 2014-09-07 11:24:22 PDT
The reason was that certificate info is relatively huge, and most of the time unneeded.
Comment 2 Antti Koivisto 2014-09-07 11:34:45 PDT
Passing it via a side channel doesn't make it any smaller. 

I'm fixing that too by only initialising it for the main resource.
Comment 3 Alexey Proskuryakov 2014-09-07 11:51:17 PDT
> Passing it via a side channel doesn't make it any smaller. 

Who said that it did? What helped was that we didn't pass it over IPC most of the time we were sending an NSURLResponse.
Comment 4 Antti Koivisto 2014-09-07 12:02:28 PDT
What other high-volume messages with ResourceResponse are there besides DidReceiveResponseWithCertificateInfo?

In any case the certificate is not used for anything else than the main resource so bulk of those are unnecessary.
Comment 5 Antti Koivisto 2014-09-07 12:05:18 PDT
Created attachment 237756 [details]
patch
Comment 6 Antti Koivisto 2014-09-08 01:01:32 PDT
Created attachment 237765 [details]
patch 2
Comment 7 Antti Koivisto 2014-09-08 02:20:49 PDT
Created attachment 237768 [details]
patch 3
Comment 8 Antti Koivisto 2014-09-08 02:32:26 PDT
Created attachment 237770 [details]
patch 4

apparently ews doesn't like file move+modify
Comment 9 Antti Koivisto 2014-09-08 02:39:11 PDT
Created attachment 237771 [details]
patch 5
Comment 10 Antti Koivisto 2014-09-08 02:44:10 PDT
Created attachment 237774 [details]
patch 6
Comment 11 Alexey Proskuryakov 2014-09-08 09:25:09 PDT
> What other high-volume messages with ResourceResponse are there besides DidReceiveResponseWithCertificateInfo?

WillSendRequest is one, WebPageProxy's DecidePolicyForResponseSync is another. The way to find this out is to search for "ResourceResponse" in WebKit2's *.messages.in files.

I suspect that the proposed patch may be a performance regression on SSL pages due to sending certificate data to UI process, which we didn't do before. I don't know how to verify this though.

There may be a lot more in client injected bundles as they send API wrapped response objects, but I couldn't find any. Either I was searching incorrectly, or there aren't any today in Safari.

CC'ing Mitz, the Master of Certificates.
Comment 12 Darin Adler 2014-09-08 09:55:05 PDT
Comment on attachment 237774 [details]
patch 6

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:9202
> +		5F2DBBE8178E336900141486 /* CertificateInfo.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = CertificateInfo.h; path = ../mac/CertificateInfo.h; sourceTree = "<group>"; };

This seems strange. Can we move this in the mac group in the project too, so it doesn’t have a path with ".." in it?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:49
> +    , m_didInitializeCertificateInfo(false)

Shouldn’t this be true? Maybe we should use a dirty bit instead of a “did initialize” bit?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:76
> +    , m_didInitializeCertificateInfo(true)

If false is right for the empty constructor, not sure why true is right here.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:220
> +CertificateInfo ResourceResponseBase::certificateInfo() const
> +{
> +    ASSERT(m_didInitializeCertificateInfo);
> +    return m_certificateInfo;
> +}

Seems like this should be inlined in the header. I wouldn’t put it in the class definition, but I would put it at the bottom of the header. Hate to have function call overhead for a simple getter like this.

> Source/WebCore/platform/network/ResourceResponseBase.h:30
> +#include "CertificateInfo.h"

Windows build error:

     1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\network\ResourceResponseBase.h(30): fatal error C1083: Cannot open include file: 'CertificateInfo.h': No such file or directory (..\html\canvas\WebGLRenderingContext.cpp)

> Source/WebCore/platform/network/ResourceResponseBase.h:96
> +    void initializeCertificateInfo() const;

I’m not entirely sure that initializeCertificateInfo is the right name for this.

This function does something different from “initializing”. It effectively sets a discretionary “should include certificate info” flag that allows someone to get the certificate info from this object; implements a policy about which responses should include certificate info. Very tricky concept, I think, because you could always call initializeCertificateInfo at any time, except if you serialize and deserialize an object, and then it would be too late for that to work. The right name could make this easier to understand.

Maybe addCertificateInfo?

> Source/WebCore/platform/network/ResourceResponseBase.h:146
>      // The ResourceResponse subclass may "shadow" this method to lazily initialize platform specific fields

Should say “these functions” instead of “this method” since there are now three functions below.

> Source/WebCore/platform/network/ResourceResponseBase.h:149
> +    String platformSuggestedFileName() const { return String(); }

I don’t understand exactly why this was added. I don’t see any code that seems to require it.

> Source/WebCore/platform/network/ResourceResponseBase.h:151
>      // The ResourceResponse subclass may "shadow" this method to compare platform specific fields

This separate comment for one function isn’t great when the three functions above share only a single comment. I suggest either a comment for every function, or a single comment for all four functions.

> Source/WebCore/platform/network/ResourceResponseBase.h:162
> +    mutable bool m_didInitializeCertificateInfo;

I think this should just be m_includesCertificateInfo. Or maybe WTF::Optional would be the best way to handle this.

> Source/WebCore/platform/network/mac/CertificateInfo.h:52
> +typedef int32_t CertificateInfo;

What is this about? A workaround to let use use the CertificateInfo type on other platforms? Is the integer actually used?
Comment 13 Antti Koivisto 2014-09-08 14:26:13 PDT
> Seems like this should be inlined in the header. I wouldn’t put it in the class definition, but I would put it at the bottom of the header. Hate to have function call overhead for a simple getter like this.

I think we should only manually inline things that are meaningful in terms of performance (this isn't) or reduce number of code lines (single line accessors). Everything else goes to cpp.

One practical reason is that routine inlining often results in unnecessary includes from the header.

The compiler should really be smart enough to inline what it wants whether it is in cpp or h (==build WebKit with LTO).
Comment 14 Antti Koivisto 2014-09-09 01:20:02 PDT
https://trac.webkit.org/r173423
Comment 15 Csaba Osztrogonác 2014-09-09 01:51:01 PDT
(In reply to comment #14)
> https://trac.webkit.org/r173423

It broke the Apple Windows build:

    1>WebCore.lib(ResourceResponseBase.obj) : error LNK2001: unresolved external symbol "private: class WebCore::CertificateInfo __thiscall WebCore::ResourceResponse::platformCertificateInfo(void)const " (?platformCertificateInfo@ResourceResponse@WebCore@@ABE?AVCertificateInfo@2@XZ)
     1>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\WebKit.dll : fatal error LNK1120: 1 unresolved externals
     1>Done Building Project "C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj" (Build target(s)) -- FAILED.
Comment 16 Antti Koivisto 2014-09-09 02:11:21 PDT
Looking
Comment 17 Carlos Alberto Lopez Perez 2014-09-09 02:18:18 PDT
(In reply to comment #14)
> https://trac.webkit.org/r173423

It has caused a regression on GTK (and probably EFL too since it also uses soup). Reported here: https://bugs.webkit.org/show_bug.cgi?id=136669
Comment 18 Antti Koivisto 2014-09-09 02:37:28 PDT
Windows build fix https://trac.webkit.org/r173425
Comment 19 mitz 2014-09-30 14:12:11 PDT
(In reply to comment #18)
> Windows build fix https://trac.webkit.org/r173425

This caused bug 137262.