Bug 28681

Summary:

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

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

CSS

Assignee:

Nate Chapin <japhet>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hyatt

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
crash report	none
patch	darin: review-
patch2	eric: review+

Eric Seidel (no email)

Reported 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...

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Nate Chapin

Comment 1 2009-08-24 12:57:45 PDT

Created attachment 38495 [details] patch

Eric Seidel (no email)

Comment 2 2009-08-24 13:13:07 PDT

Comment on attachment 38495 [details] patch Why wouldn't we just change currentStyle to be a RefPtr?

Nate Chapin

Comment 3 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.

Darin Adler

Comment 4 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.

Nate Chapin

Comment 5 2009-08-24 13:57:50 PDT

Created attachment 38498 [details] patch2

Eric Seidel (no email)

Comment 6 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().

Nate Chapin

Comment 7 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.

Note You need to log in before you can comment on or make changes to this bug.