Bug 184024 - Avoid constructing SecurityOrigin objects from non-main threads
Summary: Avoid constructing SecurityOrigin objects from non-main threads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 183066 184059
  Show dependency treegraph
 
Reported: 2018-03-26 14:21 PDT by Chris Dumez
Modified: 2018-03-27 16:41 PDT (History)
10 users (show)

See Also:


Attachments
WIP Patch (44.66 KB, patch)
2018-03-26 15:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (47.14 KB, patch)
2018-03-26 15:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (1.11 MB, application/zip)
2018-03-26 16:58 PDT, EWS Watchlist
no flags Details
Patch (50.86 KB, patch)
2018-03-26 16:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (50.73 KB, patch)
2018-03-27 14:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-03-26 14:21:33 PDT
Avoid constructing SecurityOrigin objects from non-main threads as SecurityOrigin is not thread safe.
Comment 1 Ryosuke Niwa 2018-03-26 14:36:46 PDT
Can we make add a release assertion for this as well?
Comment 2 Chris Dumez 2018-03-26 14:38:44 PDT
(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.
Comment 3 Chris Dumez 2018-03-26 15:14:14 PDT
Created attachment 336545 [details]
WIP Patch
Comment 4 Chris Dumez 2018-03-26 15:38:18 PDT
Created attachment 336547 [details]
WIP Patch
Comment 5 EWS Watchlist 2018-03-26 16:58:38 PDT
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.
Comment 6 EWS Watchlist 2018-03-26 16:58:39 PDT
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
Comment 7 Chris Dumez 2018-03-26 16:59:52 PDT
Created attachment 336557 [details]
Patch
Comment 8 youenn fablet 2018-03-27 13:56:48 PDT
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.
Comment 9 Chris Dumez 2018-03-27 14:08:29 PDT
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.
Comment 10 Chris Dumez 2018-03-27 14:16:36 PDT
Created attachment 336618 [details]
Patch
Comment 11 WebKit Commit Bot 2018-03-27 15:01:08 PDT
Comment on attachment 336618 [details]
Patch

Clearing flags on attachment: 336618

Committed r230009: <https://trac.webkit.org/changeset/230009>
Comment 12 WebKit Commit Bot 2018-03-27 15:01:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-03-27 15:03:27 PDT
<rdar://problem/38929412>