[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 #18 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 357640
  --> https://bugs.webkit.org/attachment.cgi?id=357640
Patch

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

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:44
> +static constexpr auto httpsUpgradeCheckerSQLQuery = "SELECT host FROM hosts WHERE host = ?;"_s;

Not convinced with need this global, we could just inline it.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:46
> +static const String& NetworkHTTPSUpgradeCheckerDatabasePath()

This should not start with an upper case latter.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:60
> +    , m_database(makeUniqueRef<WebCore::SQLiteDatabase>())

You're likely missing a call to SQLiteDatabase::disableThreadingChecks() since you're using a worker queue and it means the db may be used from different thread (even though sequentially). I expect you'd see assertions getting hit in debug builds.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:87
> +    ASSERT(isMainThread());

We normally use ASSERT(RunLoop::isMain()); in WK2 code.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:89
> +    m_workQueue->dispatch([this, host = WTFMove(host), callback = WTFMove(callback)] () mutable {

host = host.isolatedCopy() for thread safety.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:103
> +        WTF::RunLoop::main().dispatch([foundHost, callback = WTFMove(callback)] () mutable {

You should not need WTF:: prefix.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:111
> +    // FIXME: how should I implement this?

Definitely not OK, pass m_sessionID to the query() method then rely on sessionID.isAlwaysOnLoggingAllowed().

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

We live to put boolean members last.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:218
> +    httpsUpgradeChecker.query(url.host().toString(), [request = WTFMove(request), handler = WTFMove(handler)] (bool foundHost) mutable {

You'll likely need to pass m_sessionID to query() here.

> Source/WebKit/NetworkProcess/NetworkProcess.h:33
> +#if ENABLE(HTTPS_UPGRADE)

Not needed, you can include unconditionally.

> Source/WebKit/NetworkProcess/NetworkProcess.h:408
> +

Unnecessary new line.

> Source/WebKit/NetworkProcess/NetworkProcess.h:409
> +

Unnecessary new line.

-- 
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/20181219/2f4ed382/attachment.html>