Changeset 174856 in webkit

Timestamp:

Oct 17, 2014 6:12:46 PM (10 years ago)

Author:

mark.lam@apple.com

Message:

Web Process crash when starting the web inspector after r174025.
<​https://webkit.org/b/137340>

Reviewed by Filip Pizlo.

After r174025, we can generate a bad graph in the DFG fixup phase like so:

102:<!0:-> StoreBarrier(Check:KnownCell:@19, ..., bc#44)
60:<!0:-> PutStructure(Check:KnownCell:@19, ..., bc#44)
103:<!0:-> Check(Check:NotCell:@54, ..., bc#44)

^{-- PutByOffset's StoreBarrier has been elided and replaced
with a speculation check which can OSR exit.}

61:<!0:-> PutByOffset(Check:KnownCell:@19, ..., bc#44)

As a result, the structure change will get executed even if we end up OSR
exiting before the PutByOffset. In the baseline JIT code, the structure now
erroneously tells the put operation that there is a value in that property
slot when it is actually uninitialized (hence, the crash).

The fix is to insert the Check at the earliest point possible:

1. If the checked node is in the same bytecode as the PutByOffset, then the earliest point where we can insert the Check is right after the checked node.

2. If the checked node is from a preceding bytecode (before the PutByOffset), then the earliest point where we can insert the Check is at the start of the current bytecode.

Also reverted the workaround from r174749: ​https://webkit.org/b/137758.

Benchmark results appear to be a wash on aggregate.

• dfg/DFGFixupPhase.cpp:

(JSC::DFG::FixupPhase::indexOfNode):
(JSC::DFG::FixupPhase::indexOfFirstNodeOfExitOrigin):
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::insertCheck):

• dfg/DFGInsertionSet.h:

(JSC::DFG::InsertionSet::insertOutOfOrder):
(JSC::DFG::InsertionSet::insertOutOfOrderNode):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGFixupPhase.cpp (modified) (3 diffs)
• dfg/DFGInsertionSet.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-10-17  Mark Lam  <mark.lam@apple.com>
+
+        Web Process crash when starting the web inspector after r174025.
+        <https://webkit.org/b/137340>
+
+        Reviewed by Filip Pizlo.
+
+        After r174025, we can generate a bad graph in the DFG fixup phase like so:
+
+            102:<!0:-> StoreBarrier(Check:KnownCell:@19, ..., bc#44)
+            60:<!0:->  PutStructure(Check:KnownCell:@19, ..., bc#44)
+            103:<!0:-> Check(Check:NotCell:@54, ..., bc#44)
+                    // ^-- PutByOffset's StoreBarrier has been elided and replaced
+                    //     with a speculation check which can OSR exit.
+            61:<!0:->  PutByOffset(Check:KnownCell:@19, ..., bc#44)
+
+        As a result, the structure change will get executed even if we end up OSR
+        exiting before the PutByOffset.  In the baseline JIT code, the structure now
+        erroneously tells the put operation that there is a value in that property
+        slot when it is actually uninitialized (hence, the crash).
+
+        The fix is to insert the Check at the earliest point possible:
+
+        1. If the checked node is in the same bytecode as the PutByOffset, then
+           the earliest point where we can insert the Check is right after the
+           checked node.
+
+        2. If the checked node is from a preceding bytecode (before the PutByOffset),
+           then the earliest point where we can insert the Check is at the start
+           of the current bytecode.
+
+        Also reverted the workaround from r174749: https://webkit.org/b/137758.
+
+        Benchmark results appear to be a wash on aggregate.
+
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::indexOfNode):
+        (JSC::DFG::FixupPhase::indexOfFirstNodeOfExitOrigin):
+        (JSC::DFG::FixupPhase::fixupNode):
+        (JSC::DFG::FixupPhase::insertCheck):
+        * dfg/DFGInsertionSet.h:
+        (JSC::DFG::InsertionSet::insertOutOfOrder):
+        (JSC::DFG::InsertionSet::insertOutOfOrderNode):
+
 2014-10-10  Oliver Hunt  <oliver@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

@@ -86 +86 @@
         clearPhantomsAtEnd();
         m_insertionSet.execute(block);
+    }
+    
+    inline unsigned indexOfNode(Node* node, unsigned indexToSearchFrom)
+    {
+        unsigned index = indexToSearchFrom;
+        while (index) {
+            if (m_block->at(index) == node)
+                break;
+            index--;
+        }
+        ASSERT(m_block->at(index) == node);
+        return index;
+    }
+
+    inline unsigned indexOfFirstNodeOfExitOrigin(CodeOrigin& originForExit, unsigned indexToSearchFrom)
+    {
+        unsigned index = indexToSearchFrom;
+        ASSERT(m_block->at(index)->origin.forExit == originForExit);
+        while (index) {
+            index--;
+            if (m_block->at(index)->origin.forExit != originForExit) {
+                index++;
+                break;
+            }
+        }
+        ASSERT(m_block->at(index)->origin.forExit == originForExit);
+        return index;
     }
     
@@ -944 +971 @@
                 fixEdge<KnownCellUse>(node->child1());
             fixEdge<KnownCellUse>(node->child2());
-            insertStoreBarrier(m_indexInBlock, node->child2());
+            insertStoreBarrier(m_indexInBlock, node->child2(), node->child3());
             break;
         }
@@ -1711 +1738 @@
     {
         observeUseKindOnNode<useKind>(node);
-        m_insertionSet.insertNode(
+        CodeOrigin& checkedNodeOrigin = node->origin.forExit;
+        CodeOrigin& currentNodeOrigin = m_currentNode->origin.forExit;
+        if (currentNodeOrigin == checkedNodeOrigin) {
+            // The checked node is within the same bytecode. Hence, the earliest
+            // position we can insert the check is right after the checked node.
+            indexInBlock = indexOfNode(node, indexInBlock);
+            indexInBlock++;
+        } else {
+            // The checked node is from a preceding bytecode. Hence, the earliest
+            // position we can insert the check is at the start of the current
+            // bytecode.
+            indexInBlock = indexOfFirstNodeOfExitOrigin(currentNodeOrigin, indexInBlock);
+        }
+        m_insertionSet.insertOutOfOrderNode(
             indexInBlock, SpecNone, Check, m_currentNode->origin, Edge(node, useKind));
     }

trunk/Source/JavaScriptCore/dfg/DFGInsertionSet.h

@@ -117 +117 @@
     }
 
+    Node* insertOutOfOrder(const Insertion& insertion)
+    {
+        size_t targetIndex = insertion.index();
+        size_t entry = m_insertions.size();
+        if (entry) {
+            do {
+                entry--;
+                if (m_insertions[entry].index() <= targetIndex) {
+                    entry++;
+                    break;
+                }
+            } while (entry);
+        }
+        m_insertions.insert(entry, insertion);
+        return insertion.element();
+    }
+    
+    Node* insertOutOfOrder(size_t index, Node* element)
+    {
+        return insertOutOfOrder(Insertion(index, element));
+    }
+
+    template<typename... Params>
+    Node* insertOutOfOrderNode(size_t index, SpeculatedType type, Params... params)
+    {
+        return insertOutOfOrder(index, m_graph.addNode(type, params...));
+    }
+
     void execute(BasicBlock* block)
     {