Steps for reproduction: 1) Navigate to http://www.weather.com/weather/map/44118 -- (the US Zip Code at the end doesn't matter, it can be any valid zip) 2) Show Web Inspector 3) Change the Selector Box below the main weather image to something else, other than "Select Another Map." Crash will ensue. Stack Trace from the r67838 Nightly: Thread 0 Crashed: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x0000000100d929b6 WebCore::CSSStyleSelector::loadPendingImages() + 150 1 com.apple.WebCore 0x0000000100d952b6 WebCore::CSSStyleSelector::styleForElement(WebCore::Element*, WebCore::RenderStyle*, bool, bool, bool) + 1414 2 com.apple.WebCore 0x00000001014b9f72 WebCore::Node::styleForRenderer() + 82 3 com.apple.WebCore 0x00000001014bbf8f WebCore::Node::createRendererIfNeeded() + 143 4 com.apple.WebCore 0x0000000100ec3940 WebCore::Element::attach() + 32 5 com.apple.WebCore 0x0000000100fbfc41 WebCore::HTMLLIElement::attach() + 17 6 com.apple.WebCore 0x0000000100f893a2 WTF::PassRefPtr<WebCore::Element> WebCore::HTMLConstructionSite::attach<WebCore::Element>(WebCore::ContainerNode*, WTF::PassRefPtr<WebCore::Element>) + 178 7 com.apple.WebCore 0x0000000100f8853b WebCore::HTMLConstructionSite::attachToCurrent(WTF::PassRefPtr<WebCore::Element>) + 43 8 com.apple.WebCore 0x0000000100f888cb WebCore::HTMLConstructionSite::insertHTMLElement(WebCore::AtomicHTMLToken&) + 59 9 com.apple.WebCore 0x0000000100ffe5c8 WebCore::HTMLTreeBuilder::processStartTagForInBody(WebCore::AtomicHTMLToken&) + 776 10 com.apple.WebCore 0x0000000100fff99e WebCore::HTMLTreeBuilder::processStartTag(WebCore::AtomicHTMLToken&) + 910 11 com.apple.WebCore 0x0000000101003fc1 WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken&) + 17 12 com.apple.WebCore 0x0000000101004045 WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&) + 37 13 com.apple.WebCore 0x0000000100f8dd14 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 132 14 com.apple.WebCore 0x0000000100f8f0aa WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) + 138 15 com.apple.WebCore 0x0000000100dbb1e7 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, int, bool) + 471 16 com.apple.WebCore 0x0000000100debe34 WebCore::DocumentLoader::commitData(char const*, int) + 132 17 com.apple.WebKit 0x0000000100a3044d -[WebHTMLRepresentation receivedData:withDataSource:] + 493 18 com.apple.WebKit 0x0000000100a0110b -[WebDataSource(WebInternal) _receivedData:] + 75 19 com.apple.WebKit 0x0000000100a1b62f WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) + 111 20 com.apple.WebCore 0x0000000100de9a26 WebCore::DocumentLoader::commitLoad(char const*, int) + 150 -------------------------------------- Reproducible in a release build of r67988, though the stack trace is a little different. So I took a look in the code and discovered the issue. In WebCore::CSSStyleSelector::loadPendingImages(): ... case CSSPropertyBackgroundImage: { for (FillLayer* backgroundLayer = m_style->accessBackgroundLayers(); backgroundLayer; backgroundLayer = backgroundLayer->next()) { if (backgroundLayer->image() && backgroundLayer->image()->isPendingImage()) { CSSImageValue* imageValue = static_cast<StylePendingImage*>(backgroundLayer->image())->cssImageValue(); -->backgroundLayer->setImage(imageValue->cachedImage(cachedResourceLoader)); } } break; } ... As it is running through, at the point just before it crashes m_style becomes null after the arrowed statement is executed. resulting in the crash when m_style is dereferenced at the arrow in WebCore::CSSStyleSelector::styleForElement: ... adjustRenderStyle(style(), e); // Start loading images referenced by this style. loadPendingImages(); // If we have first-letter pseudo style, do not share this style -->if (m_style->hasPseudoStyle(FIRST_LETTER)) m_style->setUnique(); ... At first I thought it was just a loop running an extra iteration when it shouldn't, but now I am unsure. So I'll sit back and let the experts fix it.
<rdar://problem/8471963>
Classic case of modifying the mPendingImages hash while enumerating, via this stack: #0 0x00000001032931e1 in WebCore::CSSStyleSelector::initForStyleResolve (this=0x10905d000, e=0x1395f2830, parentStyle=0x0, pseudoID=WebCore::NOPSEUDO) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/css/CSSStyleSelector.cpp:849 #1 0x0000000103272f27 in WebCore::CSSStyleSelector::styleForElement (this=0x10905d000, e=0x1395f2830, defaultParent=0x0, allowSharing=true, resolveForRootDefault=false, matchVisitedPseudoClass=false) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/css/CSSStyleSelector.cpp:1165 #2 0x000000010340b9a0 in WebCore::Element::recalcStyle (this=0x1395f2830, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:929 #3 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x1395f0630, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #4 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x1395ee990, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #5 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x1395ee220, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #6 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x1395c4890, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #7 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x1395a19d0, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #8 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x1395a5960, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #9 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x1395a0540, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #10 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x1395a5c90, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #11 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x139544d20, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #12 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x139553b30, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #13 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x13953a0f0, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #14 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x139553ff0, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #15 0x000000010340c03c in WebCore::Element::recalcStyle (this=0x137bb9580, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Element.cpp:997 #16 0x00000001032e5d87 in WebCore::Document::recalcStyle (this=0x10908ca00, change=WebCore::Node::NoChange) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Document.cpp:1493 #17 0x00000001032e5a8a in WebCore::Document::updateStyleIfNeeded (this=0x10908ca00) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Document.cpp:1537 #18 0x00000001032dec58 in WebCore::Document::updateStyleForAllDocuments () at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/dom/Document.cpp:1553 #19 0x0000000103b70f9c in WebCore::ScriptController::executeScript (this=0x12c02aec8, sourceCode=@0x7fff5fbfc010, shouldAllowXSS=WebCore::DoNotAllowXSS) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/bindings/ScriptControllerBase.cpp:64 #20 0x0000000103b7106d in WebCore::ScriptController::executeScript (this=0x12c02aec8, script=@0x7fff5fbfc100, forceUserGesture=false, shouldAllowXSS=WebCore::DoNotAllowXSS) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/bindings/ScriptControllerBase.cpp:49 #21 0x00000001023734bb in WebInspectorClient::sendMessageToFrontend (this=0x10aec9e90, message=@0x7fff5fbfc140) at /Volumes/InternalData/Development/webkit/OpenSource/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp:119 #22 0x00000001036616a9 in WebCore::InspectorFrontend::updateResource (this=0x138022c90, resource=@0x7fff5fbfc2a0) at /Volumes/InternalData/Development/webkit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/InspectorFrontend.cpp:314 #23 0x000000010366e7b4 in WebCore::InspectorResource::updateScriptObject (this=0x13a102bd0, frontend=0x138022c90) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/inspector/InspectorResource.cpp:330 #24 0x000000010360d0b3 in WebCore::InspectorController::didLoadResourceFromMemoryCache (this=0x12c015c00, loader=0x109195600, cachedResource=0x136647260) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/inspector/InspectorController.cpp:942 #25 0x0000000103472954 in WebCore::FrameLoader::loadedResourceFromMemoryCache (this=0x12c007650, resource=0x136647260) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/loader/FrameLoader.cpp:3026 #26 0x0000000103165084 in WebCore::CachedResourceLoader::checkCacheObjectStatus (this=0x137b95b10, resource=0x136647260) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/loader/CachedResourceLoader.cpp:364 #27 0x0000000103165bd9 in WebCore::CachedResourceLoader::requestResource (this=0x137b95b10, type=WebCore::CachedResource::ImageResource, url=@0x7fff5fbfcb90, charset=@0x7fff5fbfcab0, isPreload=false) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/loader/CachedResourceLoader.cpp:274 #28 0x0000000103165e78 in WebCore::CachedResourceLoader::requestImage (this=0x137b95b10, url=@0x7fff5fbfcb90) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/loader/CachedResourceLoader.cpp:137 #29 0x0000000103203089 in WebCore::CSSImageValue::cachedImage (this=0x139563400, loader=0x137b95b10, url=@0x7fff5fbfcb90) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/css/CSSImageValue.cpp:74 #30 0x0000000103203197 in WebCore::CSSImageValue::cachedImage (this=0x139563400, loader=0x137b95b10) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/css/CSSImageValue.cpp:64 #31 0x000000010324e022 in WebCore::CSSStyleSelector::loadPendingImages (this=0x10905d000) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/css/CSSStyleSelector.cpp:6847 #32 0x0000000103273a42 in WebCore::CSSStyleSelector::styleForElement (this=0x10905d000, e=0x13a102580, defaultParent=0x0, allowSharing=true, resolveForRootDefault=false, matchVisitedPseudoClass=false) at /Volumes/InternalData/Development/webkit/OpenSource/WebCore/css/CSSStyleSelector.cpp:1357
Should WebInspectorClient::sendMessageToFrontend() really be synchronous?
Executing script causes Document::updateStyleForAllDocuments() to run.
We're re-entering CSSStyleSelector::styleForElement() here, which is terrible.
*** Bug 46666 has been marked as a duplicate of this bug. ***
Created attachment 68993 [details] Patch
Attachment 68993 [details] did not build on win: Build output: http://queues.webkit.org/results/4002180
Hmm, maybe this should go into the cross-platform file. Would still appreciate a review of the approach, though.
Comment on attachment 68993 [details] Patch This is likely to break inspector/ tests. We rely upon WebInspector.dispatchMessageFromBackend's (WebInspector.dispatch's) timer that is delaying the dispatch job. See inspector.js for the code. Your change is good, especially accompanied with this unnecessary timer removal. But our testing harness uses pending message counter to make sure all async jobs are dispatched (same dispatch method in js). In addition to your change you would need to notify front-end once your queue is empty. We do this in chromium. ... or if the tests are passing, feel free to land it. I'll have to understand why they are passing though.
Pavel: are you suggesting I remove the setTimeout stuff in WebInspector.dispatch()? If so, do we need to ensure that all platforms are doing async dispatch in their native code?
(In reply to comment #11) > Pavel: are you suggesting I remove the setTimeout stuff in WebInspector.dispatch()? If so, do we need to ensure that all platforms are doing async dispatch in their native code? Not only that. Ideally, these things should happen: - All ports establish async message dispatch in native layer - setTimeout goes away from the WebInspector.dispatch - This new message pump should notify front-end each time it has done processing the queued messages + there were no new messages scheduled (See TestController.js's runAfterPendingDispatches, it needs to exec its callback after all the message interactions have been settled) You timer code seems to be WebCore-based, hence it can be implemented in the InspectorClient.h to cover all the ports (as a short term solution). Or you could introduce InspectorClientLocal that ports would inherit from (as is implemented in case of InspectorFrontendClient). TestController can be notified similarly to the way we call WebInspector.dispatchMessageFromBackend.
Pavel: any progress here?
Created attachment 71020 [details] [PATCH] Proposed fix.
Created attachment 71021 [details] [PATCH] Same with the comment typo fixed.
Comment on attachment 71021 [details] [PATCH] Same with the comment typo fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=71021&action=review > WebCore/inspector/InspectorClient.cpp:61 > + // As a result we might re-enter CSSStyleSelector::styleForElement() which is terrible. It seems wrong to me that Document::updateStyleForAllDocuments() is called in ScriptController::executeScript, can we fix that place instead?
Comment on attachment 71021 [details] [PATCH] Same with the comment typo fixed. looks good to me
(In reply to comment #16) > (From update of attachment 71021 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71021&action=review > > > WebCore/inspector/InspectorClient.cpp:61 > > + // As a result we might re-enter CSSStyleSelector::styleForElement() which is terrible. > > It seems wrong to me that Document::updateStyleForAllDocuments() is called in ScriptController::executeScript, can we fix that place instead? Need to talk to Dimitri. But this change is anyways good - it removes copypaste.
(In reply to comment #18) > (In reply to comment #16) > > (From update of attachment 71021 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=71021&action=review > > > > > WebCore/inspector/InspectorClient.cpp:61 > > > + // As a result we might re-enter CSSStyleSelector::styleForElement() which is terrible. > > > > It seems wrong to me that Document::updateStyleForAllDocuments() is called in ScriptController::executeScript, can we fix that place instead? > > Need to talk to Dimitri. But this change is anyways good - it removes copypaste. Yes, I like the refactoring but semantic change seems wrong. I'm OK with committing the refactoring without the change.
(In reply to comment #16) > It seems wrong to me that Document::updateStyleForAllDocuments() is called in ScriptController::executeScript, can we fix that place instead? Bug 46761.
Comment on attachment 71021 [details] [PATCH] Same with the comment typo fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=71021&action=review Thanks for taking care of this! > WebCore/ChangeLog:8 > + Web Inspector: [crash] when Inspector Open in CSSStyleSelector::loadPendingImages(). > + https://bugs.webkit.org/show_bug.cgi?id=46224 > + > + * CMakeLists.txt: There should be a summary here of what the problem was, and what changes you made to fix it. Specifically I'd like to see mentioned that fact that calling through doDispatchMessageOnFrontendPage() makes this asynchronous, by virtue of the timer in Inspector.js. That is not at all apparent from these changes. > WebKit/cf/ChangeLog:8 > + Web Inspector: [crash] when Inspector Open in CSSStyleSelector::loadPendingImages(). > + https://bugs.webkit.org/show_bug.cgi?id=46224 > + > + * WebCoreSupport/WebInspectorClientCF.cpp: Again, these changelogs need some explanatory comments.
> Specifically I'd like to see mentioned that fact that calling through doDispatchMessageOnFrontendPage() makes this asynchronous, by virtue of the timer in Inspector.js. That is not at all apparent from these changes. ChangeLog does not say that since the call is still synchronous. All I did was factor out 3 copy-pastes and switch from using executeInWorld that re-enters styles for no good reason to evaluate. This change should save us from crash while we are waiting for comments from Dimitri on why styles update is necessary in executeInWorld.
(In reply to comment #22) > > Specifically I'd like to see mentioned that fact that calling through doDispatchMessageOnFrontendPage() makes this asynchronous, by virtue of the timer in Inspector.js. That is not at all apparent from these changes. > > ChangeLog does not say that since the call is still synchronous. All I did was factor out 3 copy-pastes and switch from using executeInWorld that re-enters styles for no good reason to evaluate. This change should save us from crash while we are waiting for comments from Dimitri on why styles update is necessary in executeInWorld. So how do we know that some random JS in the inspector won't cause a call to updateStyleForAllDocuments()? This seems like a fragile way to fix the bug.
(In reply to comment #23) > (In reply to comment #22) > > > Specifically I'd like to see mentioned that fact that calling through doDispatchMessageOnFrontendPage() makes this asynchronous, by virtue of the timer in Inspector.js. That is not at all apparent from these changes. > > > > ChangeLog does not say that since the call is still synchronous. All I did was factor out 3 copy-pastes and switch from using executeInWorld that re-enters styles for no good reason to evaluate. This change should save us from crash while we are waiting for comments from Dimitri on why styles update is necessary in executeInWorld. > > So how do we know that some random JS in the inspector won't cause a call to updateStyleForAllDocuments()? This seems like a fragile way to fix the bug. The first line we hit in inspector is setTimeout, so there is no synchronous continuation at all. So we are safe to some extent. The bullet-proof way of fixing it (involving changes to testing harness) will need much more time while the title says 'reproducible crash'...
Comment on attachment 71021 [details] [PATCH] Same with the comment typo fixed. I filed a bug for async messaging: https://bugs.webkit.org/show_bug.cgi?id=47830. Am going to land this patch as is since it prevents from the actual crash. (Using Simon's r+ for that).
Comment on attachment 71021 [details] [PATCH] Same with the comment typo fixed. Clearing flags on attachment: 71021 Committed r69968: <http://trac.webkit.org/changeset/69968>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/69968 might have broken Qt Windows 32-bit Release