Bug 22869

Summary:

REGRESSION (r38407): http://news.cnet.com/8301-13579_3-9953533-37.html crashes

Product:

WebKit

Reporter:

Cameron Zwarich (cpst) <zwarich>

Component:

JavaScriptCore

Assignee:

Cameron Zwarich (cpst) <zwarich>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, ismail, sam, webkit

Priority:

Keywords:

InRadar, NeedsReduction, Regression

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

URL:

http://news.cnet.com/8301-13579_3-9953533-37.html

Attachments:

Description	Flags
Stack trace	none
Proposed patch	darin: review+

Cameron Zwarich (cpst)

Reported 2008-12-15 14:12:37 PST

This was split off from bug 22797, because it is a separate issue at the same URL. The original crash was due to a bug in plugin code. However, if I reload this page repeatedly, I see another crash even with plugins disabled. I am going to try to narrow down the exact revision that caused this, because reducing it doesn't seem to be working out that well.

Attachments
Stack trace (10.93 KB, text/plain) 2008-12-15 16:58 PST, Cameron Zwarich (cpst)	no flags	Details
Proposed patch (deleted) 2008-12-16 02:54 PST, Cameron Zwarich (cpst)	darin: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Cameron Zwarich (cpst)

Comment 1 2008-12-15 14:13:40 PST

<rdar://problem/6402499>

Cameron Zwarich (cpst)

Comment 2 2008-12-15 14:27:02 PST

I have tracked this down to the range between the r38386 and r38492 nightlies.

Cameron Zwarich (cpst)

Comment 3 2008-12-15 16:43:58 PST

This regressed in r38407: http://trac.webkit.org/changeset/38407

Cameron Zwarich (cpst)

Comment 4 2008-12-15 16:58:03 PST

Created attachment 26039 [details] Stack trace This will crash with a lot of different stack traces, depending on the revision and build type, but I get this one pretty consistently on a local debug build of r38407 with MallocScribble enabled.

Cameron Zwarich (cpst)

Comment 5 2008-12-15 23:05:17 PST

I am having a lot of trouble with this.

Cameron Zwarich (cpst)

Comment 6 2008-12-16 01:35:51 PST

This is fixed by making Structure::m_nameInPrevious a RefPtr. Does anyone know why it isn't currently a RefPtr?

Cameron Zwarich (cpst)

Comment 7 2008-12-16 02:12:34 PST

I understand why this is a problem. Previously, m_nameInPrevious was being ref'd because it was put into the PropertyMap. Now, the PropertyMap is lazily created, so m_nameInPrevious is not ref'd until the PropertyMap is created. It would presumedly be possible to catch this with an assertion on UString::Rep::ref(), but there are a few places where ref counts of UString::Reps are deliberately zeroed out, so the assertion won't work until those are fixed.

Cameron Zwarich (cpst)

Comment 8 2008-12-16 02:54:09 PST

Created attachment 26049 [details] Proposed patch

Darin Adler

Comment 9 2008-12-16 09:26:35 PST

Comment on attachment 26049 [details] Proposed patch I'm really disappointed that we can't regression test this. I'd like to know more about the "few places where ref counts of UString::Reps are deliberately zeroed out" too. r=me

Cameron Zwarich (cpst)

Comment 10 2008-12-16 14:39:23 PST

Fixed in r39431. Darin, I'll make another bug about fixing UString ref counting.

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