Bug 46224 - Reproducible Crash when Inspector Open in CSSStyleSelector::loadPendingImages()
: Reproducible Crash when Inspector Open in CSSStyleSelector::loadPendingImages()
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P1 Normal
Assigned To:
: http://www.weather.com/weather/map/44118
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2010-09-21 15:27 PST by
Modified: 2010-10-18 11:06 PST (History)


Attachments
Patch (5.06 KB, patch)
2010-09-27 17:09 PST, Simon Fraser (smfr)
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Proposed fix. (16.11 KB, patch)
2010-10-18 04:32 PST, Pavel Feldman
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Same with the comment typo fixed. (16.11 KB, patch)
2010-10-18 04:40 PST, Pavel Feldman
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-09-21 15:27:50 PST
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.
------- Comment #1 From 2010-09-23 15:59:41 PST -------
<rdar://problem/8471963>
------- Comment #2 From 2010-09-27 15:36:53 PST -------
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
------- Comment #3 From 2010-09-27 15:40:44 PST -------
Should WebInspectorClient::sendMessageToFrontend() really be synchronous?
------- Comment #4 From 2010-09-27 15:46:30 PST -------
Executing script causes Document::updateStyleForAllDocuments() to run.
------- Comment #5 From 2010-09-27 15:47:41 PST -------
We're re-entering CSSStyleSelector::styleForElement() here, which is terrible.
------- Comment #6 From 2010-09-27 17:03:17 PST -------
*** Bug 46666 has been marked as a duplicate of this bug. ***
------- Comment #7 From 2010-09-27 17:09:03 PST -------
Created an attachment (id=68993) [details]
Patch
------- Comment #8 From 2010-09-27 19:19:12 PST -------
Attachment 68993 [details] did not build on win:
Build output: http://queues.webkit.org/results/4002180
------- Comment #9 From 2010-09-27 19:42:03 PST -------
Hmm, maybe this should go into the cross-platform file. Would still appreciate a review of the approach, though.
------- Comment #10 From 2010-09-28 11:02:15 PST -------
(From update of attachment 68993 [details])
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.
------- Comment #11 From 2010-09-28 12:07:09 PST -------
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?
------- Comment #12 From 2010-09-28 12:54:07 PST -------
(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.
------- Comment #13 From 2010-10-15 15:16:18 PST -------
Pavel: any progress here?
------- Comment #14 From 2010-10-18 04:32:37 PST -------
Created an attachment (id=71020) [details]
[PATCH] Proposed fix.
------- Comment #15 From 2010-10-18 04:40:12 PST -------
Created an attachment (id=71021) [details]
[PATCH] Same with the comment typo fixed.
------- Comment #16 From 2010-10-18 05:58:31 PST -------
(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?
------- Comment #17 From 2010-10-18 06:04:51 PST -------
(From update of attachment 71021 [details])
looks good to me
------- Comment #18 From 2010-10-18 06:11:52 PST -------
(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.
------- Comment #19 From 2010-10-18 06:17:43 PST -------
(In reply to comment #18)
> (In reply to comment #16)
> > (From update of attachment 71021 [details] [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.
------- Comment #20 From 2010-10-18 08:15:09 PST -------
(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 #21 From 2010-10-18 08:19:44 PST -------
(From update of attachment 71021 [details])
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.
------- Comment #22 From 2010-10-18 08:37:14 PST -------
> 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.
------- Comment #23 From 2010-10-18 09:03:29 PST -------
(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.
------- Comment #24 From 2010-10-18 09:10:04 PST -------
(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 #25 From 2010-10-18 09:44:41 PST -------
(From update of attachment 71021 [details])
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 #26 From 2010-10-18 10:09:33 PST -------
(From update of attachment 71021 [details])
Clearing flags on attachment: 71021

Committed r69968: <http://trac.webkit.org/changeset/69968>
------- Comment #27 From 2010-10-18 10:09:41 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #28 From 2010-10-18 11:06:14 PST -------
http://trac.webkit.org/changeset/69968 might have broken Qt Windows 32-bit Release