Bug 20857 – REGRESSION (r36427): ASSERTION FAILED: m_refCount >= 0 in RegisterID::deref()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Matt Lilek]

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)

[Matt Lilek]

Created attachment 23442 [details]
Full crashlog

[Matt Lilek]

This doesn't crash in the latest nightly (r36403).

[Geoffrey Garen]

Matt, should we close this bug?

[Matt Lilek]

(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.

[Geoffrey Garen]

Sorry! I forgot that nightly == release build. Back up to P1. We definitely want to fix this.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

Sorry, I meant func, which is protected by a RefPtr<RegisterID> called protectFunc, not protoFunc.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

Created attachment 23494 [details]
Reduction

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

    o = new Constructor[m](m);

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Maciej Stachowiak]

Comment on attachment 23496 [details]
Different proposed patch

r=me

[Cameron Zwarich (cpst)]

Landed in r36528.