ContentSecurityPolicy::upgradeInsecureRequestIfNeeded() should be called from the main thread. It constructs a SecurityOrigin object internally, which is only safe on the main thread.
Thread 25 Crashed:: WebCore: Worker 0 com.apple.JavaScriptCore 0x00000003c7b24604 WTFCrash + 36 (Assertions.cpp:271) 1 com.apple.WebCore 0x00000003ba855674 WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&) + 180 (SecurityOrigin.cpp:150) 2 com.apple.WebCore 0x00000003ba85582d WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&) + 29 (SecurityOrigin.cpp:165) 3 com.apple.WebCore 0x00000003ba855c57 WebCore::SecurityOrigin::create(WebCore::URL const&) + 327 (SecurityOrigin.cpp:201) 4 com.apple.WebCore 0x00000003ba8e0b4f WebCore::ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(WebCore::URL&, WebCore::ContentSecurityPolicy::InsecureRequestType) const + 111 (ContentSecurityPolicy.cpp:826) 5 com.apple.WebCore 0x00000003ba8e0ab3 WebCore::ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(WebCore::ResourceRequest&, WebCore::ContentSecurityPolicy::InsecureRequestType) const + 67 (ContentSecurityPolicy.cpp:817) 6 com.apple.WebCore 0x00000003b9454948 WebCore::FetchLoader::start(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&) + 344 (FetchLoader.cpp:102) 7 com.apple.WebCore 0x00000003b9467b2f WebCore::FetchResponse::BodyLoader::start(WebCore::ScriptExecutionContext&, WebCore::FetchRequest const&) + 1103 (FetchResponse.cpp:335) 8 com.apple.WebCore 0x00000003b9467436 WebCore::FetchResponse::fetch(WebCore::ScriptExecutionContext&, WebCore::FetchRequest&, WTF::Function<void (WebCore::ExceptionOr<WebCore::FetchResponse&>&&)>&&) + 662 (FetchResponse.cpp:198) 9 com.apple.WebCore 0x00000003b9469e6f WebCore::WorkerGlobalScopeFetch::fetch(WebCore::WorkerGlobalScope&, WTF::Variant<WTF::RefPtr<WebCore::FetchRequest, WTF::DumbPtrTraits<WebCore::FetchRequest> >, WTF::String>&&, WebCore::FetchRequestInit&&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&) + 255 (WorkerGlobalScopeFetch.cpp:50) 10 com.apple.WebCore 0x00000003b92a8679 WebCore::jsWorkerGlobalScopePrototypeFunctionFetchBody(JSC::ExecState*, WebCore::JSWorkerGlobalScope*, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&, JSC::ThrowScope&) + 585 (JSWorkerGlobalScope.cpp:2727) 11 com.apple.WebCore 0x00000003b92a8b38 long long WebCore::IDLOperationReturningPromise<WebCore::JSWorkerGlobalScope>::call<&(WebCore::jsWorkerGlobalScopePrototypeFunctionFetchBody(JSC::ExecState*, WebCore::JSWorkerGlobalScope*, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&, JSC::ThrowScope&)), (WebCore::PromiseExecutionScope)1, (WebCore::CastedThisErrorBehavior)2>(JSC::ExecState&, char const*)::'lambda'(JSC::ExecState&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&)::operator()(JSC::ExecState&, WTF::Ref<WebCore::DeferredPromise, WTF::DumbPtrTraits<WebCore::DeferredPromise> >&&) const + 680 (JSDOMOperationReturningPromise.h:52) 12 com.apple.WebCore 0x00000003b92a87f0 JSC::JSValue WebCore::callPromiseFunction<(WebCore::PromiseExecutionScope)1, long long
ASSERTION FAILED: isMainThread() ./page/SecurityOrigin.cpp(150) : WebCore::SecurityOrigin::SecurityOrigin(const WebCore::URL &) 1 0x3b4db35fd WTFCrash 2 0x3a7ae4674 WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&) 3 0x3a7ae482d WebCore::SecurityOrigin::SecurityOrigin(WebCore::URL const&) 4 0x3a7ae4c57 WebCore::SecurityOrigin::create(WebCore::URL const&) 5 0x3a7b6fb4f WebCore::ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(WebCore::URL&, WebCore::ContentSecurityPolicy::InsecureRequestType) const 6 0x3a88089f6 WebCore::XMLHttpRequest::open(WTF::String const&, WebCore::URL const&, bool) 7 0x3a8808d69 WebCore::XMLHttpRequest::open(WTF::String const&, WTF::String const&, bool, WTF::String const&, WTF::String const&) 8 0x3a654d46a WebCore::jsXMLHttpRequestPrototypeFunctionOpen2Body(JSC::ExecState*, WebCore::JSXMLHttpRequest*, JSC::ThrowScope&) 9 0x3a654cd85 WebCore::jsXMLHttpRequestPrototypeFunctionOpenOverloadDispatcher(JSC::ExecState*, WebCore::JSXMLHttpRequest*, JSC::ThrowScope&) 10 0x3a65416be long long WebCore::IDLOperation<WebCore::JSXMLHttpRequest>::call<&(WebCore::jsXMLHttpRequestPrototypeFunctionOpenOverloadDispatcher(JSC::ExecState*, WebCore::JSXMLHttpRequest*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) 11 0x3a654144c WebCore::jsXMLHttpRequestPrototypeFunctionOpen(JSC::ExecState*)
Created attachment 336560 [details] WIP Patch
Created attachment 336561 [details] WIP Patch
Created attachment 336563 [details] WIP Patch
Created attachment 336567 [details] Previous patch
Another option would be to remove use of SecurityOrigin from ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(). For instance, m_insecureNavigationRequestsToUpgrade could be a HashSet<SecurityOriginData>.
(In reply to youenn fablet from comment #7) > Another option would be to remove use of SecurityOrigin from > ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(). > For instance, m_insecureNavigationRequestsToUpgrade could be a > HashSet<SecurityOriginData>. That was my initial proposal but Daniel did not like that as much.
(In reply to Chris Dumez from comment #8) > (In reply to youenn fablet from comment #7) > > Another option would be to remove use of SecurityOrigin from > > ContentSecurityPolicy::upgradeInsecureRequestIfNeeded(). > > For instance, m_insecureNavigationRequestsToUpgrade could be a > > HashSet<SecurityOriginData>. > > That was my initial proposal but Daniel did not like that as much. Some thoughts: - ContentSecurityPolicy seems to restrict its use of SecurityOrigin to a triple { protocol, host, port }. SecurityOrigin is (unfortunately?) much more than that. SecurityOriginData is just that triple. - SecurityOriginData is probably more efficient to construct and compare. - We usually try to do as much as possible in background threads so there should be some benefit in doing the reverse. - We are using ContentSecurityPolicy in NetworkProcess and will use it more and more as we want to move redirection handling in NetworkProcess. There is no real SecurityOrigin in NetworkProcess since we use SecurityOriginData for IPC. Having ContentSecurityPolicy use some form of SecurityOriginData might be more consistent. And we could envision handling redirections in a NetworkProcess background thread.
We had some discussion with Daniel. Here are some related points in that context: - We should try to stop using SecurityOrigin in NetworkProcess/StorageProcess and use it only in WebProcess+MainThread. - Using SecurityOriginData (as it is now, meaning dumb) as a replacement to SecurityOrigin is fine. That would lead me to think we should do it in that patch (smaller patch, more efficient). Daniel was not opposed to it but was not enthusiastic. - We should probably refrain from extending SecurityOriginData to some capabilities that SecurityOrigin has, like canRequest. Efforts have been made to put all security checks in one place and that is probably a good thing. We will need to see how we do this for NetworkProcess. It has a very specific case to handle (HTTP redirections) so might be fine with a dedicated approach based on exporting SecurityOriginData plus some additional NetworkResourceLoadParameters values. Daniel, please correct if I misinterpret your points of view.
I personally do not mind either way since I already wrote the more complicated patch. I'll wait for Daniel to comment.
Created attachment 336626 [details] Alternative patch using SecurityOriginData
(In reply to Chris Dumez from comment #12) > Created attachment 336626 [details] > Alternative patch using SecurityOriginData Here is the patch using SecurityOriginData instead based on Youenn's discussion with Daniel.
Comment on attachment 336626 [details] Alternative patch using SecurityOriginData View in context: https://bugs.webkit.org/attachment.cgi?id=336626&action=review > Source/WebCore/loader/DocumentWriter.cpp:158 > + HashSet<SecurityOriginData> insecureNavigationRequestsToUpgrade; insecureNavigationRequestsToUpgrade seems not totally correct. insecureNavigationRequestOriginsToUpgrade seems better, but maybe not worth the refactoring effort, since it would require changing takeNavigationRequestsToUpgrade and m_insecureNavigationRequestsToUpgrade? > Source/WebCore/page/csp/ContentSecurityPolicy.h:167 > + HashSet<SecurityOriginData>&& takeNavigationRequestsToUpgrade(); Why not HashSet<SecurityOriginData>?
Created attachment 336629 [details] Alternative patch using SecurityOriginData
Comment on attachment 336629 [details] Alternative patch using SecurityOriginData Ok, let's go with this one for now since Youenn prefers it and Daniel did not object. We can also reconsider later it turns out the other patch was better once Youenn is done with his refactoring.
(In reply to Chris Dumez from comment #16) > Comment on attachment 336629 [details] > Alternative patch using SecurityOriginData > > Ok, let's go with this one for now since Youenn prefers it and Daniel did > not object. We can also reconsider later it turns out the other patch was > better once Youenn is done with his refactoring. Right, we are currently not implementing request upgrade for ping loads. The plan is to beef up NetworkLoadChecker so that it does so through its own ContentSecurityPolicy. Ideally, we would make ContentSecurityPolicy to be SecurityOrigin free as part of making NetworkProcess SecurityOrigin free. As I see it now, the only refactoring bit is ContentSecurityPolicy constructor which would take a SecurityOriginData instead of a SecurityOrigin since it is only using it to get protocol/host/port.
Comment on attachment 336629 [details] Alternative patch using SecurityOriginData Clearing flags on attachment: 336629 Committed r230017: <https://trac.webkit.org/changeset/230017>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38935104>
> Right, we are currently not implementing request upgrade for ping loads. > The plan is to beef up NetworkLoadChecker so that it does so through its own > ContentSecurityPolicy. Filed https://bugs.webkit.org/show_bug.cgi?id=184098