• fpizlo@apple.com's avatar
    Frequent RELEASE_ASSERT crashes in Structure::checkOffsetConsistency on WebGL swizzler tests · d5cc6107
    fpizlo@apple.com authored
    https://bugs.webkit.org/show_bug.cgi?id=121661
    
    Reviewed by Mark Hahnenberg.
            
    This method shouldn't have been called from the concurrent JIT thread. That's hard to prevent
    so I added a return-early check using isCompilationThread().
            
    Here's why this makes sense. Structure has two ways to tell you about the layout of the objects
    it is describing: m_offset and the property table. Most structures only have m_offset and report
    null for the property table. If the property table is there, it will tell you additional
    information and that information subsumes m_offset - but the m_offset is still there. So, when
    we have a property table, we have to keep it in sync with the m_offset. There is a bunch of
    machinery to do this.
            
    Changing the property table only happens on the main thread.
            
    Because the machinery to change the property table is so complex, especially with respect to
    keeping it in sync with m_offset, we have the checkOffsetConsistency method. It's meant to be
    called at key points before and after changes to the property table or the offset.
    
    Most clients of Structure who care about object layout, including the concurrent thread, will
    want to know m_offset and not the property table. If they want the property table, they will
    already be super careful. The concurrent thread has special methods for this, like
    Structure::getConcurrently(), which uses fine-grained locking to ensure that it sees a coherent
    view of the property table.
            
    Adding locking to checkOffsetConsistency() is probably a bad idea since that method may be
    called when the relevant lock is already held. So, we'd have awkward recursive locking issues.
            
    But right now, the concurrent JIT thread may call a method, like Structure::outOfLineCapacity(),
    which has a call to checkOffsetConsistency(). The call to checkOffsetConsistency() is there
    because we have found that it helps quickly identify situations where the property table and
    m_offset get out of sync - mainly because code that changes either of those things will usually
    also want to know the outOfLineCapacity(). But Structure::outOfLineCapacity() doesn't *actually*
    need the property table; it uses the m_offset. The concurrent JIT is correct to call
    outOfLineCapacity(), and is right to do so without holding any locks (since in all cases where
    it calls outOfLineCapacity() it has already proven that m_offset is immutable). But because
    outOfLineCapacity() calls checkOffsetConsistency(), and checkOffsetConsistency() doesn't grab
    locks, and that same structure is having its property table modified by the main thread, we end
    up with these spurious assertion failures. FWIW, the structure isn't *actually* having *its*
    property table modified - instead what happens is that some downstream structure steals the
    property table and then starts adding things to it. The concurrent thread loads the property
    table before it's stolen, and hence the badness.
            
    I suspect there are other code paths that lead to the concurrent JIT calling some Structure
    method that it is fine and safe to call, but then that method calls checkOffsetConsistency(),
    and then you have a possible crash.
            
    The most sensible solution to this appears to be to make sure that checkOffsetConsistency() is
    aware of its uselessness to the concurrent JIT thread. This change makes it return early if
    it's in the concurrent JIT.
            
    * runtime/StructureInlines.h:
    (JSC::Structure::checkOffsetConsistency):
    
    
    
    git-svn-id: http://svn.webkit.org/repository/webkit/trunk@157645 268f45cc-cd09-0410-ab3c-d52691b4dbfc
    d5cc6107