Bug 22869 - REGRESSION (r38407): http://news.cnet.com/8301-13579_3-9953533-37.html crashes
Summary: REGRESSION (r38407): http://news.cnet.com/8301-13579_3-9953533-37.html crashes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Cameron Zwarich (cpst)
URL: http://news.cnet.com/8301-13579_3-995...
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2008-12-15 14:12 PST by Cameron Zwarich (cpst)
Modified: 2008-12-16 14:39 PST (History)
4 users (show)

See Also:


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

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