[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 #8 from Vivek Seth <v_seth 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:59
>> +        }
> 
> Under what scenario would the HTTPS_UPGRADE feature be enabled with a missing database? Can this be an ASSERT instead?

Can't think of one, I will make this an assert

>> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.h:47
>> +    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.

Removing this and leaving query() as-is, would result in a slightly different behavior than I was going for. Since running "setup" can take a few milliseconds on some devices, we don't want to block network requests on that. We decided that if we make a network request before setup is complete, we will ignore upgrading those requests. That's why I think I still need property or something like it.

In the past I considered, having "NetworkHTTPSUpgradeChecker" own this logic. If its internal setup is not yet complete, it bails out early without checking the database. I feel like this approach creates a weird API: only return correct answers after internal setup is complete, but clients have no way of knowing when that happens. It also makes this class hard to test for correctness. 

Another idea would be to give query() another param `isBlocking` to determine whether or not it should wait for setup, but I feel like this is weird API too.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:216
>> +    }
> 
> I think you should let NetworkHTTPSUpgradeChecker::query() do this check on the work queue.

If I do this check on the work queue, it means we have to wait for setup is complete. The point of having this check is to avoid waiting for setup to complete.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:44
>> +enum class StoredCredentialsPolicy : bool;
> 
> Not sure you still need all this.

I think I had to add these as a side-effect of unified builds. After I added NetworkHTTPSUpgradeChecker.cpp to Sources.txt, I got a bunch of errors in this file. This is what I came up with after fixing all the errors.

>> Source/WebKit/NetworkProcess/NetworkProcess.h:407
>> +#endif // ENABLE(HTTPS_UPGRADE)
> 
> Could you make NetworkHTTPSUpgradeChecker itself be a singleton, as opposed to adding a member to an existing singleton?

I can. This approach is what Chris suggested, but I don't feel strongly one way or another.

-- 
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/3e47fdcf/attachment.html>