Created attachment 261448 [details] crashing test Null dereference loading Blink layout test http/tests/websocket/construct-in-detached-frame.html. Stack trace: Crashed Thread: 0 Dispatch queue: com.apple.main-thread Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000338 Exception Note: EXC_CORPSE_NOTIFY VM Regions Near 0x338: --> __TEXT 00000001072d6000-00000001072d8000 [ 8K] r-x/rwx SM=COW /Users/USER/*/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.Development.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development Application Specific Information: CRASHING TEST: temp-tests/http/tests/websocket/construct-in-detached-frame.html Global Trace Buffer (reverse chronological seconds): 36.717854 CFNetwork 0x00007fff88d43b97 Explicitly setting CF cookie storage singleton 36.718200 CFNetwork 0x00007fff88d8f211 Explicitly setting cookie storage singleton Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000010ffb4b91 WebCore::WebSocket::connect(WTF::String const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, int&) + 673 (memory:2635) 1 com.apple.WebCore 0x000000010ffb487b WebCore::WebSocket::create(WebCore::ScriptExecutionContext&, WTF::String const&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul> const&, int&) + 427 (StdLibExtras.h:366) 2 com.apple.WebCore 0x000000010f979302 WebCore::JSWebSocketConstructor::constructJSWebSocket1(JSC::ExecState*) + 322 (JSWebSocket.cpp:154) 3 com.apple.JavaScriptCore 0x000000010e9afde8 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 360 (LLIntSlowPaths.cpp:1107) 4 com.apple.JavaScriptCore 0x000000010e9b686b llint_entry + 22948 5 com.apple.JavaScriptCore 0x000000010e9b0ce4 vmEntryToJavaScript + 299 6 com.apple.JavaScriptCore 0x000000010e8712d9 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169 (JITCode.cpp:82) 7 com.apple.JavaScriptCore 0x000000010e857a10 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 10448 (Interpreter.cpp:945) 8 com.apple.JavaScriptCore 0x000000010e56a4c5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 469 (Completion.cpp:104) 9 com.apple.WebCore 0x000000010fccd8ec WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) + 284 (JSMainThreadExecState.h:62) 10 com.apple.WebCore 0x000000010fccdb29 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) + 41 (ScriptController.cpp:180) 11 com.apple.WebCore 0x000000010fcd3aac WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) + 316 (ScriptElement.cpp:309) 12 com.apple.WebCore 0x000000010fcd2756 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 1046 (StdLibExtras.h:366) 13 com.apple.WebCore 0x000000010f4cf5eb WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) + 347 (ScriptElement.h:58) 14 com.apple.WebCore 0x000000010f4cf440 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) + 48 (HTMLScriptRunner.cpp:191) 15 com.apple.WebCore 0x000000010f472466 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() + 86 (StdLibExtras.h:366) 16 com.apple.WebCore 0x000000010f47252d WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) + 93 (HTMLDocumentParser.cpp:214) 17 com.apple.WebCore 0x000000010f4720c3 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 595 (HTMLDocumentParser.cpp:259) 18 com.apple.WebCore 0x000000010f472ddd WebCore::HTMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) + 669 (DocumentParser.h:71) 19 com.apple.WebCore 0x000000010f21561c WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&) + 92 (StdLibExtras.h:366) 20 com.apple.WebCore 0x000000010f27568b WebCore::DocumentWriter::end() + 43 (RefPtr.h:71) 21 com.apple.WebCore 0x000000010f25d9ec WebCore::DocumentLoader::finishedLoading(double) + 268 (ResourceErrorBase.h:42) 22 com.apple.WebCore 0x000000010f08e179 WebCore::CachedResource::checkNotify() + 153 (CachedResourceClientWalker.h:51) 23 com.apple.WebCore 0x000000010f08a433 WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 227 (CachedRawResource.cpp:104) 24 com.apple.WebCore 0x000000010fe05501 WebCore::SubresourceLoader::didFinishLoading(double) + 1153 (ResourceLoader.h:154) 25 com.apple.WebKit 0x000000010d94b98d WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) + 561 (HandleMessage.h:16) 26 com.apple.WebKit 0x000000010d7251f1 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 127 (memory:2636) 27 com.apple.WebKit 0x000000010d727b4a IPC::Connection::dispatchOneMessage() + 126 (memory:2656) 28 com.apple.JavaScriptCore 0x000000010eb69985 WTF::RunLoop::performWork() + 437 (functional:1742) 29 com.apple.JavaScriptCore 0x000000010eb69d32 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39) 30 com.apple.CoreFoundation 0x00007fff949e2c01 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 31 com.apple.CoreFoundation 0x00007fff949d4b1c __CFRunLoopDoSources0 + 556 32 com.apple.CoreFoundation 0x00007fff949d403f __CFRunLoopRun + 927 33 com.apple.CoreFoundation 0x00007fff949d3a38 CFRunLoopRunSpecific + 296 34 com.apple.HIToolbox 0x00007fff88e673bd RunCurrentEventLoopInMode + 235 35 com.apple.HIToolbox 0x00007fff88e67153 ReceiveNextEventCommon + 432 36 com.apple.HIToolbox 0x00007fff88e66f93 _BlockUntilNextEventMatchingListInModeWithFilter + 71 37 com.apple.AppKit 0x00007fff870b81e7 _DPSNextEvent + 1076 38 com.apple.AppKit 0x00007fff8748490d -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454 39 com.apple.AppKit 0x00007fff870ae0b8 -[NSApplication run] + 682 40 com.apple.AppKit 0x00007fff87030396 NSApplicationMain + 1176 41 libxpc.dylib 0x00007fff8c70ff70 _xpc_objc_main + 793 42 libxpc.dylib 0x00007fff8c7116bf xpc_main + 494 43 com.apple.WebKit.WebContent.Development 0x00000001072d7424 main + 409 (XPCServiceMain.Development.mm:187) 44 libdyld.dylib 0x00007fff93aa15ad start + 1
<rdar://problem/22748858>
Created attachment 262354 [details] Patch
Created attachment 262355 [details] Patch
Comment on attachment 262355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262355&action=review r=me with changes. > Source/WebCore/ChangeLog:3 > + Check Frame pointers are not null before getting Should be on 1 line. > Source/WebCore/ChangeLog:8 > + This is a merge of Blink change 187441, The description should be after the "Reviewed by" line and before the 'Test:' line. > Source/WebCore/Modules/websockets/WebSocket.cpp:244 > + shouldBypassMainWorldContentSecurityPolicy = document.frame() != nullptr && document.frame()->script().shouldBypassMainWorldContentSecurityPolicy(); document.frame() && > Source/WebCore/page/EventSource.cpp:88 > bool shouldBypassMainWorldContentSecurityPolicy = false; This code is duplicated. We may want to factor it in a function and call it in both WebSocket.cpp and here. e.g. bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext*); We could have it in ContentSecurityPolicy.h like Blink. > Source/WebCore/page/EventSource.cpp:91 > + shouldBypassMainWorldContentSecurityPolicy = document.frame() != nullptr && document.frame()->script().shouldBypassMainWorldContentSecurityPolicy(); document.frame() && > LayoutTests/ChangeLog:3 > + Check Frame pointers are not null before getting Should be on 1 line. > LayoutTests/http/tests/websocket/construct-in-detached-frame.html:11 > + testIframe.parentNode.removeChild(testIframe); testIframe.remove() should suffice > LayoutTests/http/tests/websocket/construct-in-detached-frame.html:14 > +function done() Please indent with the rest. > LayoutTests/http/tests/websocket/resources/construct-in-detached-frame.html:8 > +new webSocketClass('ws://127.0.0.1/'); Please indent. > LayoutTests/http/tests/websocket/resources/construct-in-detached-frame.html:10 > +parentWindow.console.log(e.message); Please indent.
Created attachment 262444 [details] Patch
Comment on attachment 262444 [details] Patch How about my earlier suggestion? > Source/WebCore/page/EventSource.cpp:88 > bool shouldBypassMainWorldContentSecurityPolicy = false; This code is duplicated. We may want to factor it in a function and call it in both WebSocket.cpp and here. e.g. bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext*); We could have it in ContentSecurityPolicy.h like Blink.
Comment on attachment 262444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262444&action=review > Source/WebCore/ChangeLog:3 > + Check Frame pointers are not null before getting shouldBypassMainWorldContentSecurityPolicy via those pointers. We usually use a shorter title (e.g. Fix null pointer dereference in WebSocket::connect()), then use the longer description in the Changelog description. > Source/WebCore/ChangeLog:9 > + This is a merge of Blink change 187441, This is a merge of Blink r187441:
Comment on attachment 262444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262444&action=review >> Source/WebCore/page/EventSource.cpp:88 >> bool shouldBypassMainWorldContentSecurityPolicy = false; > > This code is duplicated. We may want to factor it in a function and call it in both WebSocket.cpp and here. e.g. > bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext*); > > We could have it in ContentSecurityPolicy.h like Blink. Sure, where should I create this file?
(In reply to comment #8) > Comment on attachment 262444 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262444&action=review > > >> Source/WebCore/page/EventSource.cpp:88 > >> bool shouldBypassMainWorldContentSecurityPolicy = false; > > > > This code is duplicated. We may want to factor it in a function and call it in both WebSocket.cpp and here. e.g. > > bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext*); > > > > We could have it in ContentSecurityPolicy.h like Blink. > > Sure, where should I create this file? Forget this comment, please.
Created attachment 262461 [details] Patch
Comment on attachment 262461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262461&action=review > Source/WebCore/Modules/websockets/WebSocket.cpp:241 > + bool shouldBypassMainWorldContentSecurityPolicy = scriptExecutionContext()->contentSecurityPolicy()->shouldBypassMainWorldContentSecurityPolicy(*scriptExecutionContext()); ... = ContentSecurityPolicy::shouldBypassMainWorldContentSecurityPolicy(*scriptExecutionContext()); > Source/WebCore/page/ContentSecurityPolicy.cpp:1784 > +bool ContentSecurityPolicy::shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext& context) const It can be reused in other places as well. At least XMLHttpRequest::open(). > Source/WebCore/page/ContentSecurityPolicy.cpp:1787 > + Document& document = downcast<Document>(context); auto& > Source/WebCore/page/ContentSecurityPolicy.h:133 > + bool shouldBypassMainWorldContentSecurityPolicy(ScriptExecutionContext&) const; Should be static. > Source/WebCore/page/EventSource.cpp:88 > + bool shouldBypassMainWorldContentSecurityPolicy = context.contentSecurityPolicy()->shouldBypassMainWorldContentSecurityPolicy(context); ... = ContentSecurityPolicy::shouldBypassMainWorldContentSecurityPolicy(context); > LayoutTests/ChangeLog:7 > + Extra line.
Created attachment 262469 [details] Patch
Comment on attachment 262469 [details] Patch Looks good.
Comment on attachment 262469 [details] Patch Clearing flags on attachment: 262469 Committed r190588: <http://trac.webkit.org/changeset/190588>
All reviewed patches have been landed. Closing bug.
Hey Jiewen, thanks for fixing this. Later down in WebSocket::connect() there is a call to document.frame()->loader().mixedContentChecker(). I don't see why that doesn't crash now, since it occurs if (is<Document>(*scriptExecutionContext()), the same condition as the null dereference of frame up above. That was "safe" when I added it because document.frame() was assumed to be nonnull up above, but clearly that was wrong and is now no longer the case. I guess if it's not crashing (is it returning early on an error path?), then it might not be a problem, but it looks dangerous in light of this change... do we need to add a check to make sure frame is not null there? If so, we're going to have to rethink how to gain access to the mixed content checker, since we can't just skip that check.
(In reply to comment #16) > Hey Jiewen, thanks for fixing this. > > Later down in WebSocket::connect() there is a call to > document.frame()->loader().mixedContentChecker(). I don't see why that > doesn't crash now, since it occurs if > (is<Document>(*scriptExecutionContext()), the same condition as the null > dereference of frame up above. That was "safe" when I added it because > document.frame() was assumed to be nonnull up above, but clearly that was > wrong and is now no longer the case. > > I guess if it's not crashing (is it returning early on an error path?), then > it might not be a problem, but it looks dangerous in light of this change... > do we need to add a check to make sure frame is not null there? If so, we're > going to have to rethink how to gain access to the mixed content checker, > since we can't just skip that check. You are right. That's something I am missing. Let me dig into it.
I filed bug #149864 so we don't forget.
(In reply to comment #18) > I filed bug #149864 so we don't forget. Then, let us continue our discussion on the new thread. Should we consider this bug is closed?
Yup! I think it's easiest to move on to a new bug when we discover an issue after a patch has landed.