[Webkit-unassigned] [Bug 192736] HTTPS Upgrade: Use full sqlite upgrade list

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=192736

--- Comment #6 from Andy Estes <aestes at apple.com> ---
Comment on attachment 357507
  --> https://bugs.webkit.org/attachment.cgi?id=357507
Patch

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

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:28
> +

#if ENABLE(HTTPS_UPGRADE) should go here.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:36
> +#if ENABLE(HTTPS_UPGRADE)

Not here.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:43
> +auto kNetworkHTTPSUpgradeCheckerSQLQuery = "SELECT host FROM hosts WHERE host = ?;"_s;

static constexpr auto httpsUpgradeCheckerSQLQuery = ...

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:45
> +String NetworkHTTPSUpgradeCheckerDatabasePath();

No need to forward declare. You can just move the function definition here.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:59
> +        auto path = NetworkHTTPSUpgradeCheckerDatabasePath();
> +        if (!path) {
> +            RELEASE_LOG_IF_ALLOWED("NetworkHTTPSUpgradeChecker() - Database does not exist, cannot do upgrades.");
> +            return;
> +        }

Under what scenario would the HTTPS_UPGRADE feature be enabled with a missing database? Can this be an ASSERT instead?

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:79
> +    // The database and statement need to be destroyed on the background thread.

Don't think you need this comment.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:134
> +String NetworkHTTPSUpgradeCheckerDatabasePath()

static const String& httpsUpgradeCheckerDatabasePath()

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:141
> +#if PLATFORM(COCOA)
> +    CFBundleRef webKitBundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebKit"));
> +    path = CFURLGetString(adoptCF(CFBundleCopyResourceURL(webKitBundle, CFSTR("HTTPSUpgradeList"), CFSTR("db"), nullptr)).get());
> +#endif // PLATFORM(COCOA)

It's expensive to compute this path. Since it never changes, you can cache it in a static NeverDestroyed<String>.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.h:47
> +    // Returns `true` after internal setup is sucessfully completed. If there is an error with setup, or if setup is in-progress, it will return `false`.
> +    bool isSetupComplete() const { return m_isSetupComplete; };

Not sure you really need this function. I see where you call it below, but it seems unnecessary because query() will already do the right thing when m_isSetupComplete is false. The benefit of removing this function and letting query() check setup completion on the work queue is that you don't need to make m_isSetupComplete an std::atomic.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.h:55
> +    std::atomic<bool> m_isSetupComplete { false };

This doesn't need to be atomic so long as you only access it on the work queue (which you can and should).

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.h:58
> +    std::unique_ptr<WebCore::SQLiteDatabase> m_database;
> +    std::unique_ptr<WebCore::SQLiteStatement> m_statement;

You can use WTF::UniqueRef here.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:216
> +    // Do not wait for httpsUpgradeChecker to complete its setup.
> +    if (!httpsUpgradeChecker.isSetupComplete()) {
> +        handler(WTFMove(request));
>          return;
> +    }

I think you should let NetworkHTTPSUpgradeChecker::query() do this check on the work queue.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:44
> +#include "UserContentControllerIdentifier.h"
>  #include <WebCore/ContentExtensionActions.h>
> +#include <WebCore/ContentSecurityPolicyResponseHeaders.h>
> +#include <WebCore/FetchOptions.h>
>  #include <WebCore/NetworkLoadInformation.h>
>  #include <WebCore/ResourceError.h>
>  #include <wtf/CompletionHandler.h>
>  #include <wtf/Expected.h>
>  #include <wtf/Variant.h>
> +#include <wtf/WeakPtr.h>
>  
>  namespace WebCore {
>  class ContentSecurityPolicy;
>  struct ContentSecurityPolicyClient;
> +class SecurityOrigin;
> +enum class PreflightPolicy : uint8_t;
> +enum class StoredCredentialsPolicy : bool;

Not sure you still need all this.

> Source/WebKit/NetworkProcess/NetworkProcess.h:407
> +#if ENABLE(HTTPS_UPGRADE)
> +    NetworkHTTPSUpgradeChecker m_networkHTTPSUpgradeChecker;
> +#endif // ENABLE(HTTPS_UPGRADE)

Could you make NetworkHTTPSUpgradeChecker itself be a singleton, as opposed to adding a member to an existing singleton?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181218/d2e746ab/attachment.html>