We recursively grab a lock in the DFGBytecodeParser causing us to deadlock
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Nov 2016 23:03:41 +0000 (23:03 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Nov 2016 23:03:41 +0000 (23:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164650

Reviewed by Geoffrey Garen.

JSTests:

* stress/dont-dead-lock-put-by-val-as-put-by-id.js: Added.
(ident):
(let.o.set foo):
(foo):

Source/JavaScriptCore:

Some code was incorrectly holding a lock when recursively calling
back into the bytecode parser's via inlining a put_by_val as a put_by_id.
This can cause a deadlock if the inlinee CodeBlock is something we're
already holding a lock for. I've changed the range of the lock holder
to be as narrow as possible.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@208614 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/dont-dead-lock-put-by-val-as-put-by-id.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

index bf64ae1..91d9d57 100644 (file)
@@ -1,3 +1,15 @@
+2016-11-11  Saam Barati  <sbarati@apple.com>
+
+        We recursively grab a lock in the DFGBytecodeParser causing us to deadlock
+        https://bugs.webkit.org/show_bug.cgi?id=164650
+
+        Reviewed by Geoffrey Garen.
+
+        * stress/dont-dead-lock-put-by-val-as-put-by-id.js: Added.
+        (ident):
+        (let.o.set foo):
+        (foo):
+
 2016-11-11  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, rolling out r208584.
diff --git a/JSTests/stress/dont-dead-lock-put-by-val-as-put-by-id.js b/JSTests/stress/dont-dead-lock-put-by-val-as-put-by-id.js
new file mode 100644 (file)
index 0000000..5bfdd39
--- /dev/null
@@ -0,0 +1,17 @@
+function ident() { return "foo"; }
+noInline(ident);
+
+let o = {
+    set foo(x) {
+        foo(false);
+    }
+};
+
+function foo(cond) {
+    if (cond)
+        o[ident()] = 20;
+}
+
+for (let i = 0; i < 10000; i++) {
+    foo(true);
+}
index bc9729c..ccf9f44 100644 (file)
@@ -1,3 +1,19 @@
+2016-11-11  Saam Barati  <sbarati@apple.com>
+
+        We recursively grab a lock in the DFGBytecodeParser causing us to deadlock
+        https://bugs.webkit.org/show_bug.cgi?id=164650
+
+        Reviewed by Geoffrey Garen.
+
+        Some code was incorrectly holding a lock when recursively calling
+        back into the bytecode parser's via inlining a put_by_val as a put_by_id.
+        This can cause a deadlock if the inlinee CodeBlock is something we're
+        already holding a lock for. I've changed the range of the lock holder
+        to be as narrow as possible.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+
 2016-11-11  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, rolling out r208584.
index 835e499..26bbb6d 100644 (file)
@@ -4295,34 +4295,40 @@ bool ByteCodeParser::parseBlock(unsigned limit)
             bool isDirect = opcodeID == op_put_by_val_direct;
             bool compiledAsPutById = false;
             {
-                ConcurrentJITLocker locker(m_inlineStackTop->m_profiledBlock->m_lock);
-                ByValInfo* byValInfo = m_inlineStackTop->m_byValInfos.get(CodeOrigin(currentCodeOrigin().bytecodeIndex));
-                // FIXME: When the bytecode is not compiled in the baseline JIT, byValInfo becomes null.
-                // At that time, there is no information.
-                if (byValInfo 
-                    && byValInfo->stubInfo
-                    && !byValInfo->tookSlowPath
-                    && !m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadIdent)
-                    && !m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadType)
-                    && !m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell)) {
-                    compiledAsPutById = true;
-                    unsigned identifierNumber = m_graph.identifiers().ensure(byValInfo->cachedId.impl());
-                    UniquedStringImpl* uid = m_graph.identifiers()[identifierNumber];
+                unsigned identifierNumber;
+                PutByIdStatus putByIdStatus;
+                {
+                    ConcurrentJITLocker locker(m_inlineStackTop->m_profiledBlock->m_lock);
+                    ByValInfo* byValInfo = m_inlineStackTop->m_byValInfos.get(CodeOrigin(currentCodeOrigin().bytecodeIndex));
+                    // FIXME: When the bytecode is not compiled in the baseline JIT, byValInfo becomes null.
+                    // At that time, there is no information.
+                    if (byValInfo 
+                        && byValInfo->stubInfo
+                        && !byValInfo->tookSlowPath
+                        && !m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadIdent)
+                        && !m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadType)
+                        && !m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell)) {
+                        compiledAsPutById = true;
+                        identifierNumber = m_graph.identifiers().ensure(byValInfo->cachedId.impl());
+                        UniquedStringImpl* uid = m_graph.identifiers()[identifierNumber];
+
+                        if (Symbol* symbol = byValInfo->cachedSymbol.get()) {
+                            FrozenValue* frozen = m_graph.freezeStrong(symbol);
+                            addToGraph(CheckCell, OpInfo(frozen), property);
+                        } else {
+                            ASSERT(!uid->isSymbol());
+                            addToGraph(CheckStringIdent, OpInfo(uid), property);
+                        }
 
-                    if (Symbol* symbol = byValInfo->cachedSymbol.get()) {
-                        FrozenValue* frozen = m_graph.freezeStrong(symbol);
-                        addToGraph(CheckCell, OpInfo(frozen), property);
-                    } else {
-                        ASSERT(!uid->isSymbol());
-                        addToGraph(CheckStringIdent, OpInfo(uid), property);
-                    }
+                        putByIdStatus = PutByIdStatus::computeForStubInfo(
+                            locker, m_inlineStackTop->m_profiledBlock,
+                            byValInfo->stubInfo, currentCodeOrigin(), uid);
 
-                    PutByIdStatus putByIdStatus = PutByIdStatus::computeForStubInfo(
-                        locker, m_inlineStackTop->m_profiledBlock,
-                        byValInfo->stubInfo, currentCodeOrigin(), uid);
+                    }
+                }
 
+                if (compiledAsPutById)
                     handlePutById(base, identifierNumber, value, putByIdStatus, isDirect);
-                }
             }
 
             if (!compiledAsPutById) {