Bug 28681

Summary: Element::recalcStyle() accesses freed RenderStyles (fast/css/rem-dynamic-scaling.html)
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: CSSAssignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
crash report
none
patch
darin: review-
patch2 eric: review+

Description Eric Seidel (no email) 2009-08-24 12:38:36 PDT 
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...
Comment 1 Nate Chapin 2009-08-24 12:57:45 PDT 
Created attachment 38495 [details]
patch
Comment 2 Eric Seidel (no email) 2009-08-24 13:13:07 PDT 
Comment on attachment 38495 [details]
patch

Why wouldn't we just change currentStyle to be a RefPtr?
Comment 3 Nate Chapin 2009-08-24 13:15:33 PDT 
(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.
Comment 4 Darin Adler 2009-08-24 13:25:59 PDT 
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.
Comment 5 Nate Chapin 2009-08-24 13:57:50 PDT 
Created attachment 38498 [details]
patch2
Comment 6 Eric Seidel (no email) 2009-08-24 14:09:03 PDT 
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().
Comment 7 Nate Chapin 2009-08-24 14:21:45 PDT 
(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.