e.g. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00007fff3af505ca WebCore::FontFaceSet::FontFaceSet(WebCore::Document&, WebCore::CSSFontFaceSet&) + 90 1 com.apple.WebCore 0x00007fff3af5027e WebCore::FontFaceSet::create(WebCore::Document&, WebCore::CSSFontFaceSet&) + 190 2 com.apple.WebCore 0x00007fff3af03be5 WebCore::CSSFontSelector::fontFaceSet() + 53 3 com.apple.WebCore 0x00007fff3a21ca1e WebCore::jsDocumentFonts(JSC::JSGlobalObject*, long long, JSC::PropertyName) + 110 4 com.apple.JavaScriptCore 0x00007fff3747d381 JSC::LLInt::performLLIntGetByID(JSC::Instruction const*, JSC::CodeBlock*, JSC::JSGlobalObject*, JSC::JSValue, JSC::Identifier const&, JSC::GetByIdModeMetadata&) + 1681 5 com.apple.JavaScriptCore 0x00007fff36b592bb llint_slow_path_get_by_id + 347 6 com.apple.JavaScriptCore 0x00007fff36d64c3a llint_entry + 36118 7 com.apple.JavaScriptCore 0x00007fff36d5bd2f vmEntryToJavaScript + 216 8 com.apple.JavaScriptCore 0x00007fff373992ea JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*) + 12426 9 com.apple.JavaScriptCore 0x00007fff375edce5 JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 245 10 com.apple.WebCore 0x00007fff3ade1ac4 WebCore::JSExecState::profiledEvaluate(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 84 11 com.apple.WebCore 0x00007fff3ade17f2 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) + 178 12 com.apple.WebCore 0x00007fff3ade1b48 WebCore::ScriptController::evaluateIgnoringException(WebCore::ScriptSourceCode const&) + 40 13 com.apple.WebCore 0x00007fff3b0d1060 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&) + 528 14 com.apple.WebCore 0x00007fff39ca3551 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 4369 15 com.apple.WebCore 0x00007fff3b3adcb9 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&) + 425 16 com.apple.WebCore 0x00007fff39c4ac35 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 2597 17 com.apple.WebCore 0x00007fff3b3a4336 WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >&&) + 1494 18 com.apple.WebCore 0x00007fff3b02f534 WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&) + 196 19 com.apple.WebCore 0x00007fff3b509eed WebCore::DocumentLoader::finishedLoading() + 669 20 com.apple.WebCore 0x00007fff3b59fe6f WebCore::CachedResource::checkNotify(WebCore::NetworkLoadMetrics const&) + 95 21 com.apple.WebCore 0x00007fff3b59deed WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*, WebCore::NetworkLoadMetrics const&) + 1117 22 com.apple.WebCore 0x00007fff3b56c8dc WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) + 1148 23 com.apple.WebKit 0x00007fff3d279fd5 WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&) + 227 24 com.apple.WebKit 0x00007fff3d3ca2da WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) + 310 25 com.apple.WebKit 0x00007fff3cda1370 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 696 26 com.apple.WebKit 0x00007fff3cda3ddc WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7, void>::call() + 214 27 com.apple.JavaScriptCore 0x00007fff379c6abd WTF::RunLoop::performWork() + 573 <rdar://problem/73661979>
Created attachment 419240 [details] Test
Created attachment 419983 [details] Patch This was introduced in r201799. The problem is that we are clearing the CSSFontSelector document when document's last reference is removed but still refeenced by other nodes. Since CSSFontSelector::fontFaceSet() hasn't been called before, it will try to create a FontFaceSet::create() with a nullptr document. This should assert in FontFaceSet::create() for debug builds. So, I think we should not clear the document in that case.
Comment on attachment 419983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419983&action=review > Source/WebCore/dom/Document.cpp:-809 > - m_fontSelector->clearDocument(); I don't think this is quite right. We probably still want to stop font loading timer, etc...
Created attachment 420110 [details] Patch
Comment on attachment 420110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420110&action=review > Source/WebCore/css/CSSFontSelector.cpp:357 > +void CSSFontSelector::clearDocument() > +{ It seems that this function is no longer needed? We can just call detach in ~CSSFontSelector. There is no need to call this function in ~Document since m_document is a WeakPtr.
By the way, can we add a test here? I don't think there is any security implication here.
(In reply to Ryosuke Niwa from comment #6) > By the way, can we add a test here? I don't think there is any security > implication here. Isn't this a use after free? We end up calling the Document::contextDocument() on an already freed Document.
Created attachment 420300 [details] Patch
(In reply to Carlos Garcia Campos from comment #7) > (In reply to Ryosuke Niwa from comment #6) > > By the way, can we add a test here? I don't think there is any security > > implication here. > > Isn't this a use after free? We end up calling the > Document::contextDocument() on an already freed Document. How could that be? The bug here is that we're clearing m_document in CSSFontSelector too early, and m_document is WeakPtr<Document>.
Comment on attachment 420300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420300&action=review > Source/WebCore/css/CSSFontSelector.cpp:340 > -void CSSFontSelector::clearDocument() > +void CSSFontSelector::detach() We're not really detaching from anything, are we? This is about freeing stuff & stopping timers so that document may be delete later. I'd call something like stopLoadingForDocumentTeardown or something. > Source/WebCore/dom/Document.cpp:-762 > - m_fontSelector->unregisterForInvalidationCallbacks(*this); Why are we removing the call to unregisterForInvalidationCallbacks?
Comment on attachment 420300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420300&action=review >> Source/WebCore/css/CSSFontSelector.cpp:340 >> +void CSSFontSelector::detach() > > We're not really detaching from anything, are we? > This is about freeing stuff & stopping timers so that document may be delete later. > I'd call something like stopLoadingForDocumentTeardown or something. I used detach for consistency with other similar functions, but I agree we are not detaching from the document because we still need it. I'll find a better name. >> Source/WebCore/dom/Document.cpp:-762 >> - m_fontSelector->unregisterForInvalidationCallbacks(*this); > > Why are we removing the call to unregisterForInvalidationCallbacks? Because clearDocument clears all the clients, so this is always called on an empty m_clients.
Created attachment 420440 [details] Patch
Hold on a sec, I'm checking if there is any security implication related to this crash or not.
Comment on attachment 420440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420440&action=review > Source/WebCore/css/CSSFontSelector.cpp:340 > -void CSSFontSelector::clearDocument() > +void CSSFontSelector::clear() clear is a very generic term. I would have preferred calling it something like stopLoading.
Created attachment 421291 [details] Patch for landing
Committed r273374: <https://commits.webkit.org/r273374> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421291 [details].