[webkit-reviews] review granted: [Bug 184024] Avoid constructing SecurityOrigin objects from non-main threads : [Attachment 336557] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 184024: Avoid constructing SecurityOrigin objects from non-main threads
https://bugs.webkit.org/show_bug.cgi?id=184024

Attachment 336557: Patch

https://bugs.webkit.org/attachment.cgi?id=336557&action=review




--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 336557
  --> https://bugs.webkit.org/attachment.cgi?id=336557
Patch

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

> Source/WebCore/page/SecurityOrigin.cpp:162
> +    m_isPotentiallyTrustworthy = shouldTreatAsPotentiallyTrustworthy(url);

Can it make a difference of behavior in some edge cases where we have empty
strings in scheme registers for instance?

> Source/WebCore/workers/WorkerThread.cpp:93
> +    , m_origin(SecurityOrigin::create(m_scriptURL)->isolatedCopy())

I am not sure we actually need this isolated copy here.
I guess this is for extra safety if at some point, URL has some String host
member that would be directly copied in SecurityOrigin?
Even in that case m_scriptURL is already isolated copy.

> Source/WebCore/workers/service/ServiceWorkerProvider.cpp:48
> +bool
ServiceWorkerProvider::mayHaveServiceWorkerRegisteredForOrigin(PAL::SessionID
sessionID, const WebCore::SecurityOriginData& origin)

No need for WebCore::

> Source/WebCore/workers/service/ServiceWorkerProvider.h:47
> +    bool mayHaveServiceWorkerRegisteredForOrigin(PAL::SessionID, const
WebCore::SecurityOriginData&);

Ditto.

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:105
> +static void fireMessageEvent(ServiceWorkerGlobalScope& scope,
MessageWithMessagePorts&& message, ExtendableMessageEventSource&& source,
SecurityOriginData&& sourceOrigin)

Could be changed to a String&& or a const URL& to simplify
ServiceWorkerThread::postMessageToServiceWorker.

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:148
> +void WebSWClientConnection::matchRegistration(SecurityOriginData&&
topOrigin, const URL& clientURL, RegistrationCallback&& callback)

We could also have clientURL be a URL&&, it will allow moving it in one call
site.

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:186
> +void WebSWClientConnection::getRegistrations(SecurityOriginData&& topOrigin,
const URL& clientURL, GetRegistrationsCallback&& callback)

Ditto probably here.