Bug 20857

Summary: REGRESSION (r36427): ASSERTION FAILED: m_refCount >= 0 in RegisterID::deref()
Product: WebKit Reporter: Matt Lilek <dev+webkit>
Component: JavaScriptCoreAssignee: Cameron Zwarich (cpst) <zwarich>
Severity: Normal CC: ggaren, mjs, zwarich
Priority: P1 Keywords: HasReduction, Regression
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://www.debka.com/headline.php?hid=5579
Description Flags
Full crashlog
Proposed patch
Different proposed patch mjs: review+

Description Matt Lilek 2008-09-15 08:40:37 PDT
Loading http://www.debka.com/headline.php?hid=5579 hits the following ASSERT:

ASSERTION FAILED: m_refCount >= 0
(/Users/mlilek/Documents/WebKit/JavaScriptCore/VM/RegisterID.h:92 void JSC::RegisterID::deref())

Thread 0 Crashed:
0   com.apple.JavaScriptCore      	0x004aa0da JSC::RegisterID::deref() + 82 (RegisterID.h:92)
1   com.apple.JavaScriptCore      	0x004ad15f WTF::RefPtr<JSC::RegisterID>::~RefPtr() + 31 (RefPtr.h:50)
2   com.apple.JavaScriptCore      	0x0047fcd5 JSC::CodeGenerator::emitConstruct(JSC::RegisterID*, JSC::RegisterID*, JSC::ArgumentsNode*) + 749 (CodeGenerator.cpp:1129)
3   com.apple.JavaScriptCore      	0x004808c3 JSC::NewExprNode::emitCode(JSC::CodeGenerator&, JSC::RegisterID*) + 167 (nodes.cpp:412)
Comment 1 Matt Lilek 2008-09-15 08:41:21 PDT
Created attachment 23442 [details]
Full crashlog
Comment 2 Matt Lilek 2008-09-15 08:44:31 PDT
This doesn't crash in the latest nightly (r36403).
Comment 3 Geoffrey Garen 2008-09-15 12:18:48 PDT
Matt, should we close this bug?
Comment 4 Matt Lilek 2008-09-15 13:49:50 PDT
(In reply to comment #3)
> Matt, should we close this bug?

Thats up to you and whether you think this is worth investigating.  I'll bump this down to a P2 since it only  happens in a debug build.
Comment 5 Geoffrey Garen 2008-09-15 13:52:00 PDT
Sorry! I forgot that nightly == release build. Back up to P1. We definitely want to fix this.
Comment 6 Cameron Zwarich (cpst) 2008-09-16 15:10:46 PDT
I'll assign this to myself. The problem is that in the CodeGenerator::emitConstruct() function, the local RefPtr<RegisterID> funcProto has a negative count after it is deref'd. I don't quite understand why just yet, but hopefully it won't be too hard to fix.
Comment 7 Cameron Zwarich (cpst) 2008-09-16 15:22:04 PDT
Sorry, I meant func, which is protected by a RefPtr<RegisterID> called protectFunc, not protoFunc.
Comment 8 Cameron Zwarich (cpst) 2008-09-16 16:14:56 PDT
This is sort of strange. On the offending call, we have the following RegisterIDs:

func: 27
protoFunc: 26
callFrame[0]: 27

When it is allocating the new temporary for protoFunc, it somehow thinks that the register with index 26 is the last one in m_temporaries, which has no ref count, so it removes it and appends a new one. The next time, it still thinks that the register with index 26 is the last one, but it has a ref count, so it makes a new one with index 27.

I think the bug here is likely not in the CodeGenerator::emitConstruct() logic, but rather in the fact that the last temporary in m_temporaries isn't the one with index 26, but I'll try to make a reduction and see. Thankfully, debka.com seems fairly old school.
Comment 9 Cameron Zwarich (cpst) 2008-09-16 17:48:07 PDT
Created attachment 23494 [details]

Here's a reduction. It comes from code of the form

    o = new Constructor[m](m);
Comment 10 Cameron Zwarich (cpst) 2008-09-16 18:09:31 PDT
This is caused by the same incorrect behaviour of CodeGenerator::finalDestination() as in bug 20340. An updated version of that patch fixes this bug as well. I'll post it here for review.
Comment 11 Cameron Zwarich (cpst) 2008-09-16 19:27:34 PDT
Created attachment 23495 [details]
Proposed patch

Here is a patch that fixes this bug, as well as bug 20340. It passes all tests and all changes it makes to SunSpider codegen are positive. If this is reviewed, I will put the reduction here as a test case in fast/js/codegen-temporaries.

Just doing this for NewExprNode::emitCode() caused a lot of tests to fail, so it seems that if we are going to fix this, we need to fix it everywhere.
Comment 12 Cameron Zwarich (cpst) 2008-09-16 19:51:22 PDT
Created attachment 23496 [details]
Different proposed patch

Maciej and I agreed that the usage of finalDestination() after my patch is now unclear. Here is an easier way to fix this particular bug, but not bug 20340.
Comment 13 Maciej Stachowiak 2008-09-16 19:55:09 PDT
Comment on attachment 23496 [details]
Different proposed patch

Comment 14 Cameron Zwarich (cpst) 2008-09-16 20:29:40 PDT
Landed in r36528.