Title: [157645] trunk/Source/_javascript_Core
- Revision
- 157645
- Author
- [email protected]
- Date
- 2013-10-18 13:21:33 -0700 (Fri, 18 Oct 2013)
Log Message
Frequent RELEASE_ASSERT crashes in Structure::checkOffsetConsistency on WebGL swizzler tests
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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (157644 => 157645)
--- trunk/Source/_javascript_Core/ChangeLog 2013-10-18 20:19:33 UTC (rev 157644)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-10-18 20:21:33 UTC (rev 157645)
@@ -1,3 +1,61 @@
+2013-10-18 Filip Pizlo <[email protected]>
+
+ Frequent RELEASE_ASSERT crashes in Structure::checkOffsetConsistency on WebGL swizzler tests
+ 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):
+
2013-10-18 Daniel Bates <[email protected]>
Add SPI to disable the garbage collector timer
Modified: trunk/Source/_javascript_Core/runtime/StructureInlines.h (157644 => 157645)
--- trunk/Source/_javascript_Core/runtime/StructureInlines.h 2013-10-18 20:19:33 UTC (rev 157644)
+++ trunk/Source/_javascript_Core/runtime/StructureInlines.h 2013-10-18 20:21:33 UTC (rev 157645)
@@ -232,6 +232,13 @@
return true;
}
+ // We cannot reliably assert things about the property table in the concurrent
+ // compilation thread. It is possible for the table to be stolen and then have
+ // things added to it, which leads to the offsets being all messed up. We could
+ // get around this by grabbing a lock here, but I think that would be overkill.
+ if (isCompilationThread())
+ return true;
+
RELEASE_ASSERT(numberOfSlotsForLastOffset(m_offset, m_inlineCapacity) == propertyTable->propertyStorageSize());
unsigned totalSize = propertyTable->propertyStorageSize();
RELEASE_ASSERT((totalSize < inlineCapacity() ? 0 : totalSize - inlineCapacity()) == numberOfOutOfLineSlotsForLastOffset(m_offset));
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
