Avoid constructing SecurityOrigin objects from non-main threads as SecurityOrigin is not thread safe.
Can we make add a release assertion for this as well?
(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.
Created attachment 336545 [details] WIP Patch
Created attachment 336547 [details] WIP Patch
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.
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
Created attachment 336557 [details] Patch
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 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.
Created attachment 336618 [details] Patch
Comment on attachment 336618 [details] Patch Clearing flags on attachment: 336618 Committed r230009: <https://trac.webkit.org/changeset/230009>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38929412>