Bug 20857 - REGRESSION (r36427): ASSERTION FAILED: m_refCount >= 0 in RegisterID::deref()
Summary: REGRESSION (r36427): ASSERTION FAILED: m_refCount >= 0 in RegisterID::deref()
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://www.debka.com/headline.php?hid...
Keywords: HasReduction, Regression
Depends on:
Reported: 2008-09-15 08:40 PDT by Matt Lilek
Modified: 2008-09-16 20:29 PDT (History)
3 users (show)

See Also:

Full crashlog (36.31 KB, text/plain)
2008-09-15 08:41 PDT, Matt Lilek
no flags Details
Reduction (75 bytes, text/html)
2008-09-16 17:48 PDT, Cameron Zwarich (cpst)
no flags Details
Proposed patch (17.49 KB, patch)
2008-09-16 19:27 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Different proposed patch (2.65 KB, patch)
2008-09-16 19:51 PDT, Cameron Zwarich (cpst)
mjs: review+
Details | Formatted Diff | Diff

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