Bug 52777 – rareNonInheritedData mutated without access() calls

• Status: NEW
• Component: Layout and Rendering

[Mihai Parparita]

rareNonInheritedData mutated without access() calls

[Mihai Parparita]

Created attachment 79536 [details]
Patch

[Mihai Parparita]

We're seeing a few crashes in Chromium of the form:

Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000019 )
0x0222abd4 - renderstyle.cpp:1054]	WebCore::RenderStyle::getShadowVerticalExtent(WebCore::ShadowData const *,int &,int &)
0x022c0394 - inlineflowbox.cpp:715]	WebCore::InlineFlowBox::addBoxShadowVisualOverflow(WebCore::IntRect &)
0x022c08b9 - inlineflowbox.cpp:823]	WebCore::InlineFlowBox::computeOverflow(int,int,bool,WTF::HashMap<WebCore::InlineTextBox const *,std::pair<WTF::Vector<WebCore::SimpleFontData const *,0>,WebCore::GlyphOverflow>,WTF::PtrHash<WebCore::InlineTextBox const *>,WTF::HashTraits<WebCore::InlineTextBox const *>,WTF::HashTraits<std::pair<WTF::Vector<WebCore::SimpleFontData const *,0>,WebCore::GlyphOverflow> > > &)
0x022c0916 - inlineflowbox.cpp:837]	WebCore::InlineFlowBox::computeOverflow(int,int,bool,WTF::HashMap<WebCore::InlineTextBox const *,std::pair<WTF::Vector<WebCore::SimpleFontData const *,0>,WebCore::GlyphOverflow>,WTF::PtrHash<WebCore::InlineTextBox const *>,WTF::HashTraits<WebCore::InlineTextBox const *>,WTF::HashTraits<std::pair<WTF::Vector<WebCore::SimpleFontData const *,0>,WebCore::GlyphOverflow> > > &)
0x022d44ae - renderblocklinelayout.cpp:768]	WebCore::RenderBlock::layoutInlineChildren(bool,int &,int &)
0x0221b470 - renderblock.cpp:1230]	WebCore::RenderBlock::layoutBlock(bool,int)
0x0221b16c - renderblock.cpp:1128]	WebCore::RenderBlock::layout()
...

Which led me to investigate how rareNonInheritedData is used in RenderStyle. It looks like it should always be accessed via access() when mutating it, but there were a few places where this wasn't the case. I don't see how this could lead to null pointers (any ideas?), but it can't hurt to fix.

[Eric Seidel (no email)]

Comment on attachment 79536 [details]
Patch

How do we test this?  Should we remove the -> operator since it seems dangerous here.

[Mihai Parparita]

(In reply to comment #3)
> (From update of attachment 79536 [details])
> How do we test this? 

Not sure. RenderStyle::clone (which is how you'd end up with two RenderStyle instances that share a rare data) is mainly used for animation keyframes and the cancel button for input type="search", and the content property doesn't do anything there.

> Should we remove the -> operator since it seems dangerous here.

-> is still useful when doing reads.