Bug 46224 – Reproducible Crash when Inspector Open in CSSStyleSelector::loadPendingImages()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alex Mathews]

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.

[Simon Fraser (smfr)]

<rdar://problem/8471963>

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

Should WebInspectorClient::sendMessageToFrontend() really be synchronous?

[Simon Fraser (smfr)]

Executing script causes Document::updateStyleForAllDocuments() to run.

[Simon Fraser (smfr)]

We're re-entering CSSStyleSelector::styleForElement() here, which is terrible.

[Simon Fraser (smfr)]

*** Bug 46666 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Created attachment 68993 [details]
Patch

[WebKit Review Bot]

Attachment 68993 [details] did not build on win:
Build output: http://queues.webkit.org/results/4002180

[Simon Fraser (smfr)]

Hmm, maybe this should go into the cross-platform file. Would still appreciate a review of the approach, though.

[Pavel Feldman]

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.

[Simon Fraser (smfr)]

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?

[Pavel Feldman]

(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.

[Simon Fraser (smfr)]

Pavel: any progress here?

[Pavel Feldman]

Created attachment 71020 [details]
[PATCH] Proposed fix.

[Pavel Feldman]

Created attachment 71021 [details]
[PATCH] Same with the comment typo fixed.

[Yury Semikhatsky]

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?

[Ilya Tikhonovsky]

Comment on attachment 71021 [details]
[PATCH] Same with the comment typo fixed.

looks good to me

[Pavel Feldman]

(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.

[Yury Semikhatsky]

(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.

[Simon Fraser (smfr)]

(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.

[Simon Fraser (smfr)]

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.

[Pavel Feldman]

> 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.

[Simon Fraser (smfr)]

(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.

[Pavel Feldman]

(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'...

[Pavel Feldman]

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).

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/69968 might have broken Qt Windows 32-bit Release