Bug 28681 – Element::recalcStyle() accesses freed RenderStyles (fast/css/rem-dynamic-scaling.html)

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

[Eric Seidel (no email)]

Created attachment 38493 [details]
crash report

Element::recalcStyle() accesses freed RenderStyles (fast/css/rem-dynamic-scaling.html)

Nate just pointed this out to me.  This was crashing for Chromium, but will crash for all builds too.

To reproduce:
run-webkit-tests --debug --guard fast/css/rem-dynamic-scaling.html
or
MallocScribble=1 DumpRenderTree LayoutTests/fast/css/rem-dynamic-scaling.html

http://code.google.com/p/chromium/issues/detail?id=16850 was the chromium bug which spurred this investigation.


I suspect we should just stuff currentStyle in a RefPtr...

[Nate Chapin]

Created attachment 38495 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 38495 [details]
patch

Why wouldn't we just change currentStyle to be a RefPtr?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 38495 [details])
> Why wouldn't we just change currentStyle to be a RefPtr?

No good reason that I'm aware of.  This is my first time really messing around with RefPtr's, so I sort of guessed. :-)

I'll get a new patch ready.

[Darin Adler]

Comment on attachment 38495 [details]
patch

Since currentStyle is already a local variable, it's much cleaner to just change its type to a RefPtr instead of using the protect idiom.

Please do that.

[Nate Chapin]

Created attachment 38498 [details]
patch2

[Eric Seidel (no email)]

Comment on attachment 38498 [details]
patch2

We should probably add a comment as to why it's held in a RefPtr.  I believe you're a committer now, so you could add that when landing.

Something like:
// Ref the currentStyle in case it's deleted when we call setStyle().

[Nate Chapin]

(In reply to comment #6)
> (From update of attachment 38498 [details])
> We should probably add a comment as to why it's held in a RefPtr.  I believe
> you're a committer now, so you could add that when landing.
> 
> Something like:
> // Ref the currentStyle in case it's deleted when we call setStyle().

Done and committed at r47729.