Bug 194671

Summary:

Web Inspector: Occasional crash under WebCore::CSSStyleSheet::item called from Inspector

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Joseph Pecoraro <joepeck>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
[PATCH] Proposed Fix	hi: review+
[PATCH] For Landing	none

Description Joseph Pecoraro 2019-02-14 14:00:13 PST

Occasional crash under WebCore::CSSStyleSheet::item called from Inspector

Unsure how to reproduce, but crash logs have:

----
Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BREAKPOINT (SIGTRAP)
Exception Codes:       0x0000000000000002, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY 

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff51490a02 WebCore::CSSStyleSheet::item(unsigned int) + 370
1   com.apple.WebCore             	0x00007fff520acf1a void WebCore::InspectorCSSOMWrappers::collect<WebCore::CSSStyleSheet>(WebCore::CSSStyleSheet*) + 74
2   com.apple.WebCore             	0x00007fff520ad34c WebCore::InspectorCSSOMWrappers::collectDocumentWrappers(WebCore::ExtensionStyleSheets&) + 332
3   com.apple.WebCore             	0x00007fff5243b383 WebCore::InspectorCSSAgent::buildObjectForRule(WebCore::StyleRule*, WebCore::StyleResolver&, WebCore::Element&) + 67
4   com.apple.WebCore             	0x00007fff52437efa WebCore::InspectorCSSAgent::buildArrayForMatchedRuleList(WTF::Vector<WTF::RefPtr<WebCore::StyleRule, WTF::DumbPtrTraits<WebCore::StyleRule> >, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::StyleResolver&, WebCore::Element&, WebCore::PseudoId) + 298
5   com.apple.WebCore             	0x00007fff5243755c WebCore::InspectorCSSAgent::getMatchedStylesForNode(WTF::String&, int, bool const*, bool const*, WTF::RefPtr<WTF::JSONImpl::ArrayOf<Inspector::Protocol::CSS::RuleMatch>, WTF::DumbPtrTraits<WTF::JSONImpl::ArrayOf<Inspector::Protocol::CSS::RuleMatch> > >&, WTF::RefPtr<WTF::JSONImpl::ArrayOf<Inspector::Protocol::CSS::PseudoIdMatches>, WTF::DumbPtrTraits<WTF::JSONImpl::ArrayOf<Inspector::Protocol::CSS::PseudoIdMatches> > >&, WTF::RefPtr<WTF::JSONImpl::ArrayOf<Inspector::Protocol::CSS::InheritedStyleEntry>, WTF::DumbPtrTraits<WTF::JSONImpl::ArrayOf<Inspector::Protocol::CSS::InheritedStyleEntry> > >&) + 252
6   com.apple.JavaScriptCore      	0x00007fff47f06a95 Inspector::CSSBackendDispatcher::getMatchedStylesForNode(long, WTF::RefPtr<WTF::JSONImpl::Object, WTF::DumbPtrTraits<WTF::JSONImpl::Object> >&&) + 965
7   com.apple.JavaScriptCore      	0x00007fff47f06274 Inspector::CSSBackendDispatcher::dispatch(long, WTF::String const&, WTF::Ref<WTF::JSONImpl::Object, WTF::DumbPtrTraits<WTF::JSONImpl::Object> >&&) + 564
8   com.apple.JavaScriptCore      	0x00007fff47f02e3d Inspector::BackendDispatcher::dispatch(WTF::String const&) + 2349
9   com.apple.WebKit              	0x00007fff533178ed WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&) + 6055
...
----

Comment 1 Joseph Pecoraro 2019-02-14 14:00:22 PST

<rdar://problem/47628191>

Comment 2 Joseph Pecoraro 2019-02-14 14:15:31 PST

Created attachment 362059 [details]
[PATCH] Proposed Fix

Comment 3 Devin Rousso 2019-02-14 14:19:41 PST

Comment on attachment 362059 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=362059&action=review

rs=me

> Source/WebCore/ChangeLog:10
> +        (WebCore::CSSStyleSheet::item):

I think you could add more of the explanation from the bug comments in the ChangeLog.  Right now, what you have here doesn't really explain "how" this "might" happen, and what you investigated to arrive at this point.

> Source/WebCore/css/CSSStyleSheet.cpp:234
> +    if (m_childRuleCSSOMWrappers.size() != ruleCount)

NIT: I think it's "smarter" to only expand if we don't have enough room, not if we don't have exactly the right amount of room.

    if (m_childRuleCSSOMWrappers.size() < ruleCount)

Comment 4 Joseph Pecoraro 2019-02-14 14:58:21 PST

Comment on attachment 362059 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=362059&action=review

>> Source/WebCore/css/CSSStyleSheet.cpp:234
>> +    if (m_childRuleCSSOMWrappers.size() != ruleCount)
> 
> NIT: I think it's "smarter" to only expand if we don't have enough room, not if we don't have exactly the right amount of room.
> 
>     if (m_childRuleCSSOMWrappers.size() < ruleCount)

Fair enough.

Comment 5 Joseph Pecoraro 2019-02-14 15:01:25 PST

Created attachment 362066 [details]
[PATCH] For Landing

Comment 6 WebKit Commit Bot 2019-02-14 15:40:05 PST

Comment on attachment 362066 [details]
[PATCH] For Landing

Clearing flags on attachment: 362066

Committed r241567: <https://trac.webkit.org/changeset/241567>