Bug 136611 – Pass certificate info as part of ResourceResponse

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

The way we currently pull certificate info out of ResourceResponse for serialization and then put it back is confusing.

[Alexey Proskuryakov]

The reason was that certificate info is relatively huge, and most of the time unneeded.

[Antti Koivisto]

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.

[Alexey Proskuryakov]

> 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.

[Antti Koivisto]

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.

[Antti Koivisto]

Created attachment 237756 [details]
patch

[Antti Koivisto]

Created attachment 237765 [details]
patch 2

[Antti Koivisto]

Created attachment 237768 [details]
patch 3

[Antti Koivisto]

Created attachment 237770 [details]
patch 4

apparently ews doesn't like file move+modify

[Antti Koivisto]

Created attachment 237771 [details]
patch 5

[Antti Koivisto]

Created attachment 237774 [details]
patch 6

[Alexey Proskuryakov]

> 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.

[Darin Adler]

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?

[Antti Koivisto]

> 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).

[Antti Koivisto]

https://trac.webkit.org/r173423

[Csaba Osztrogonác]

(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.

[Antti Koivisto]

Looking

[Carlos Alberto Lopez Perez]

(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

[Antti Koivisto]

Windows build fix https://trac.webkit.org/r173425

(In reply to comment #18)
> Windows build fix https://trac.webkit.org/r173425

This caused bug 137262.