Bug 28681 - Element::recalcStyle() accesses freed RenderStyles (fast/css/rem-dynamic-scaling.html)
Summary: Element::recalcStyle() accesses freed RenderStyles (fast/css/rem-dynamic-scal...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P1 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-24 12:38 PDT by Eric Seidel (no email)
Modified: 2009-08-24 14:21 PDT (History)
1 user (show)

See Also:


Attachments
crash report (25.13 KB, text/plain)
2009-08-24 12:38 PDT, Eric Seidel (no email)
no flags Details
patch (1.39 KB, patch)
2009-08-24 12:57 PDT, Nate Chapin
darin: review-
Details | Formatted Diff | Diff
patch2 (2.39 KB, patch)
2009-08-24 13:57 PDT, Nate Chapin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.