Bug 184024 – Avoid constructing SecurityOrigin objects from non-main threads

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Avoid constructing SecurityOrigin objects from non-main threads as SecurityOrigin is not thread safe.

[Ryosuke Niwa]

Can we make add a release assertion for this as well?

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #1)
> Can we make add a release assertion for this as well?

I was planning on a debug assertion but I guess we could try a release one.

[Chris Dumez]

Created attachment 336545 [details]
WIP Patch

[Chris Dumez]

Created attachment 336547 [details]
WIP Patch

[EWS Watchlist]

Comment on attachment 336547 [details]
WIP Patch

Attachment 336547 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7107542

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 336556 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 336557 [details]
Patch

[youenn fablet]

Comment on attachment 336557 [details]
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.

[Chris Dumez]

Comment on attachment 336557 [details]
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?

This restores previous behavior. This is merely reverted https://trac.webkit.org/changeset/228972 which I landed recently.

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

I believe we need it because SecurityOrigin::create() implements caching:
Ref<SecurityOrigin> SecurityOrigin::create(const URL& url)
{
    if (RefPtr<SecurityOrigin> cachedOrigin = getCachedOrigin(url))
        return cachedOrigin.releaseNonNull();
...
}

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

Ok.

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

Ok.

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

Ok.

[Chris Dumez]

Created attachment 336618 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 336618 [details]
Patch

Clearing flags on attachment: 336618

Committed r230009: <https://trac.webkit.org/changeset/230009>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38929412>