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+

Description Cameron Zwarich (cpst) 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.

Comment 1 Cameron Zwarich (cpst) 2008-12-15 14:13:40 PST

<rdar://problem/6402499>

Comment 2 Cameron Zwarich (cpst) 2008-12-15 14:27:02 PST

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

Comment 3 Cameron Zwarich (cpst) 2008-12-15 16:43:58 PST

This regressed in r38407:

http://trac.webkit.org/changeset/38407

Comment 4 Cameron Zwarich (cpst) 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.

Comment 5 Cameron Zwarich (cpst) 2008-12-15 23:05:17 PST

I am having a lot of trouble with this.

Comment 6 Cameron Zwarich (cpst) 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?

Comment 7 Cameron Zwarich (cpst) 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.

Comment 8 Cameron Zwarich (cpst) 2008-12-16 02:54:09 PST

Created attachment 26049 [details]
Proposed patch

Comment 9 Darin Adler 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

Comment 10 Cameron Zwarich (cpst) 2008-12-16 14:39:23 PST

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