Bug 110854 - Unused Structure property tables waste 14MB on Membuster.
Summary: Unused Structure property tables waste 14MB on Membuster.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: InRadar, Performance
Depends on: 110882 111416 111447
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-26 00:27 PST by Andreas Kling
Modified: 2013-03-06 04:59 PST (History)
9 users (show)

See Also:


Attachments
Proposed patch (25.67 KB, patch)
2013-02-26 00:28 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch for landing (37.82 KB, patch)
2013-02-26 02:16 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch for landing (37.80 KB, patch)
2013-02-26 02:52 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
ews pls (37.80 KB, patch)
2013-02-26 05:05 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Come on EWS, not today. (37.99 KB, patch)
2013-02-26 08:13 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Same patch tweaked for Windows line endings (37.80 KB, patch)
2013-02-26 08:15 PST, Andreas Kling
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Add a cpp file, they said. It's not that hard, they said. (37.78 KB, patch)
2013-02-26 08:32 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch II: The Repatchening (52.18 KB, patch)
2013-03-04 13:09 PST, Andreas Kling
ggaren: review+
Details | Formatted Diff | Diff
Patch for lunar landing (52.23 KB, patch)
2013-03-04 15:25 PST, Andreas Kling
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Geoffrey not Geoff. (52.23 KB, patch)
2013-03-04 15:31 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-02-26 00:27:00 PST
<rdar://problem/13292104>
Comment 1 Andreas Kling 2013-02-26 00:28:39 PST
Created attachment 190221 [details]
Proposed patch

Lay it on me, fellas.
Comment 2 Filip Pizlo 2013-02-26 00:35:13 PST
Comment on attachment 190221 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190221&action=review

Basically, r=me, but with the slight changes.

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:280
> +inline PropertyTable* PropertyTable::create(JSGlobalData& globalData, unsigned initialCapacity)
> +{
> +    PropertyTable* table = new (NotNull, allocateCell<PropertyTable>(globalData.heap)) PropertyTable(globalData, initialCapacity);
> +    table->finishCreation(globalData);
> +    return table;
> +}
> +
> +inline PropertyTable* PropertyTable::clone(JSGlobalData& globalData, JSCell* owner, const PropertyTable& other)
> +{
> +    PropertyTable* table = new (NotNull, allocateCell<PropertyTable>(globalData.heap)) PropertyTable(globalData, owner, other);
> +    table->finishCreation(globalData);
> +    return table;
> +}
> +
> +inline PropertyTable* PropertyTable::clone(JSGlobalData& globalData, JSCell* owner, unsigned initialCapacity, const PropertyTable& other)
> +{
> +    PropertyTable* table = new (NotNull, allocateCell<PropertyTable>(globalData.heap)) PropertyTable(globalData, owner, initialCapacity, other);
> +    table->finishCreation(globalData);
> +    return table;
> +}
> +
> +inline PropertyTable::PropertyTable(JSGlobalData& globalData, unsigned initialCapacity)
> +    : JSCell(globalData, globalData.propertyTableStructure.get())
> +    , m_indexSize(sizeForCapacity(initialCapacity))

While you're creating PropertyMapHashTable.cpp, you could out-of-line these methods.  They do no good being inline.

> Source/JavaScriptCore/runtime/Structure.cpp:78
> +const ClassInfo PropertyTable::s_info = { "PropertyTable", 0, 0, 0, CREATE_METHOD_TABLE(PropertyTable) };
> +
> +void PropertyTable::visitChildren(JSCell* cell, SlotVisitor& visitor)
> +{
> +    PropertyTable* thisObject = jsCast<PropertyTable*>(cell);
> +    ASSERT_GC_OBJECT_INHERITS(thisObject, &s_info);
> +    ASSERT(thisObject->structure()->typeInfo().overridesVisitChildren());
> +
> +    JSCell::visitChildren(thisObject, visitor);
> +
> +    PropertyTable::iterator end = thisObject->end();
> +    for (PropertyTable::iterator ptr = thisObject->begin(); ptr != end; ++ptr)
> +        visitor.append(&ptr->specificValue);
> +}
> +
>  #if DUMP_STRUCTURE_ID_STATISTICS
>  static HashSet<Structure*>& liveStructureSet = *(new HashSet<Structure*>);

These should be in PropertyMapHashTable.cpp.  Or just PropertyTable.cpp.  Your pick of name.  But I wouldn't pollute Structure.cpp with these things.

(I know, adding .cpp files to WebKit is soooper hard, but at least here you don't have to do gyp.  So one less build system.)

> Source/JavaScriptCore/runtime/StructureInlines.h:200
> +inline bool Structure::putWillGrowOutOfLineStorage()
> +{
> +    checkOffsetConsistency();
> +
> +    ASSERT(outOfLineCapacity() >= outOfLineSize());
> +
> +    if (!m_propertyTable) {
> +        unsigned currentSize = numberOfOutOfLineSlotsForLastOffset(m_offset);
> +        ASSERT(outOfLineCapacity() >= currentSize);
> +        return currentSize == outOfLineCapacity();
> +    }
> +
> +    ASSERT(totalStorageCapacity() >= m_propertyTable->propertyStorageSize());
> +    if (m_propertyTable->hasDeletedOffset())
> +        return false;
> +
> +    ASSERT(totalStorageCapacity() >= m_propertyTable->size());
> +    return m_propertyTable->size() == totalStorageCapacity();

Might as well drop this method into Structure.cpp instead of having it be inline.  It meets the basic criteria for not being inline: it's not called often enough and it has a fair bit of code (multiple loads and branches).
Comment 3 EFL EWS Bot 2013-02-26 00:36:15 PST
Comment on attachment 190221 [details]
Proposed patch

Attachment 190221 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16744860
Comment 4 Early Warning System Bot 2013-02-26 00:39:33 PST
Comment on attachment 190221 [details]
Proposed patch

Attachment 190221 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16752800
Comment 5 Early Warning System Bot 2013-02-26 00:41:00 PST
Comment on attachment 190221 [details]
Proposed patch

Attachment 190221 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16747817
Comment 6 Andreas Kling 2013-02-26 02:14:23 PST
(In reply to comment #2)
> While you're creating PropertyMapHashTable.cpp, you could out-of-line these methods.  They do no good being inline.

Sure.

> These should be in PropertyMapHashTable.cpp.  Or just PropertyTable.cpp.  Your pick of name.  But I wouldn't pollute Structure.cpp with these things.
> 
> (I know, adding .cpp files to WebKit is soooper hard, but at least here you don't have to do gyp.  So one less build system.)

Source/JavaScriptCore/JavaScriptCore.gypi :(
But sure.

> > Source/JavaScriptCore/runtime/StructureInlines.h:200
> > +inline bool Structure::putWillGrowOutOfLineStorage()
> Might as well drop this method into Structure.cpp instead of having it be inline.  It meets the basic criteria for not being inline: it's not called often enough and it has a fair bit of code (multiple loads and branches).

I'm getting some wonky build failures in 'jsc' from out-of-lining that, not sure what's going on. Let's do separately.

More importantly, the patch I uploaded was missing a PropertyTable::destroy(JSCell*), I'm adding that and reuploading for EWS to chew etc.
Comment 7 Andreas Kling 2013-02-26 02:16:54 PST
Created attachment 190248 [details]
Patch for landing

...if EWS permits.
Comment 8 Andreas Kling 2013-02-26 02:52:30 PST
Created attachment 190251 [details]
Patch for landing

Not sure why it doesn't apply on EWS, let's try again.
Comment 9 Andreas Kling 2013-02-26 05:05:37 PST
Created attachment 190263 [details]
ews pls
Comment 10 Andreas Kling 2013-02-26 07:35:57 PST
Committed r144054: <http://trac.webkit.org/changeset/144054>
Comment 11 WebKit Review Bot 2013-02-26 07:42:33 PST
Re-opened since this is blocked by bug 110882
Comment 12 Andreas Kling 2013-02-26 07:56:37 PST
Reverted r144054 for reason:

broke builds

Committed r144056: <http://trac.webkit.org/changeset/144056>
Comment 13 Andreas Kling 2013-02-26 08:13:30 PST
Created attachment 190287 [details]
Come on EWS, not today.
Comment 14 Andreas Kling 2013-02-26 08:15:13 PST
Created attachment 190288 [details]
Same patch tweaked for Windows line endings

In case this is a CRLF problem, I tweaked the diff for JavaScriptCore.vcproj
Comment 15 WebKit Review Bot 2013-02-26 08:19:33 PST
Attachment 190288 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.gypi', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/runtime/JSGlobalData.cpp', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/PropertyMapHashTable.h', u'Source/JavaScriptCore/runtime/PropertyTable.cpp', u'Source/JavaScriptCore/runtime/Structure.cpp', u'Source/JavaScriptCore/runtime/Structure.h', u'Source/JavaScriptCore/runtime/StructureInlines.h']" exit_code: 1
Source/JavaScriptCore/runtime/PropertyTable.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 EFL EWS Bot 2013-02-26 08:23:15 PST
Comment on attachment 190288 [details]
Same patch tweaked for Windows line endings

Attachment 190288 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16778169
Comment 17 Early Warning System Bot 2013-02-26 08:27:33 PST
Comment on attachment 190288 [details]
Same patch tweaked for Windows line endings

Attachment 190288 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16773224
Comment 18 Early Warning System Bot 2013-02-26 08:28:45 PST
Comment on attachment 190288 [details]
Same patch tweaked for Windows line endings

Attachment 190288 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16769267
Comment 19 Andreas Kling 2013-02-26 08:32:42 PST
Created attachment 190293 [details]
Add a cpp file, they said. It's not that hard, they said.
Comment 20 WebKit Review Bot 2013-02-26 08:34:56 PST
Attachment 190293 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.gypi', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/runtime/JSGlobalData.cpp', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/PropertyMapHashTable.h', u'Source/JavaScriptCore/runtime/PropertyTable.cpp', u'Source/JavaScriptCore/runtime/Structure.cpp', u'Source/JavaScriptCore/runtime/Structure.h', u'Source/JavaScriptCore/runtime/StructureInlines.h']" exit_code: 1
Source/JavaScriptCore/runtime/PropertyTable.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 WebKit Review Bot 2013-02-26 10:45:17 PST
Comment on attachment 190293 [details]
Add a cpp file, they said. It's not that hard, they said.

Clearing flags on attachment: 190293

Committed r144074: <http://trac.webkit.org/changeset/144074>
Comment 22 WebKit Review Bot 2013-02-26 10:45:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 WebKit Review Bot 2013-02-26 11:58:44 PST
Re-opened since this is blocked by bug 110897
Comment 24 Brady Eidson 2013-02-26 11:58:52 PST
This is causing 20+ crashes and therefore early exits on Mac bots.
Comment 25 Andreas Kling 2013-02-27 00:29:46 PST
Okay, so if I understand correctly, this broke Structure::checkOffsetConsistency() in debug builds, because Structure::m_propertyTable may now get clear()ed out by one marking thread while this code executes in another. That means that the !m_propertyTable check is not enough to avoid dereferencing a null pointer.

I'm not sure what the best course of action is here. I assume we want to keep these sanity checks somehow. Geoff/Phil/Mark?

Example of a crashing thread:
Thread 11 Crashed:: JavaScriptCore::Marking
0   com.apple.JavaScriptCore      	0x0000000108abb814 JSC::WriteBarrierBase<JSC::PropertyTable>::operator->() const + 84 (WriteBarrier.h:116)
1   com.apple.JavaScriptCore      	0x0000000108abb478 JSC::Structure::checkOffsetConsistency() const + 184 (StructureInlines.h:210)
2   com.apple.JavaScriptCore      	0x0000000108abb5b9 JSC::Structure::outOfLineSize() const + 25 (Structure.h:190)
3   com.apple.JavaScriptCore      	0x0000000108aba80a JSC::Structure::outOfLineCapacity() const + 106 (Structure.h:176)
4   com.apple.JavaScriptCore      	0x0000000108dc33ca JSC::JSObject::visitButterfly(JSC::SlotVisitor&, JSC::Butterfly*, unsigned long) + 138 (JSObject.cpp:172)
5   com.apple.JavaScriptCore      	0x0000000108dba41f JSC::JSFinalObject::visitChildren(JSC::JSCell*, JSC::SlotVisitor&) + 463 (JSObject.cpp:249)
6   com.apple.JavaScriptCore      	0x0000000108ef678e JSC::visitChildren(JSC::SlotVisitor&, JSC::JSCell const*) + 206 (SlotVisitor.cpp:87)
7   com.apple.JavaScriptCore      	0x0000000108ef663f JSC::SlotVisitor::drain() + 255 (SlotVisitor.cpp:136)
8   com.apple.JavaScriptCore      	0x0000000108ef6cc9 JSC::SlotVisitor::drainFromShared(JSC::SlotVisitor::SharedDrainMode) + 1241 (SlotVisitor.cpp:226)
9   com.apple.JavaScriptCore      	0x0000000108ced9b7 JSC::GCThread::gcThreadMain() + 183 (GCThread.cpp:109)
10  com.apple.JavaScriptCore      	0x0000000108cedabd JSC::GCThread::gcThreadStartFunc(void*) + 29 (GCThread.cpp:136)
11  com.apple.JavaScriptCore      	0x0000000108fbd1c0 WTF::threadEntryPoint(void*) + 144 (Threading.cpp:70)
12  com.apple.JavaScriptCore      	0x0000000108fbdbb8 WTF::wtfThreadEntryPoint(void*) + 104 (ThreadingPthreads.cpp:197)
13  libsystem_c.dylib             	0x00007fff8a2b2742 _pthread_start + 327
14  libsystem_c.dylib             	0x00007fff8a29f181 thread_start + 13
Comment 26 Geoffrey Garen 2013-02-27 08:44:25 PST
> Okay, so if I understand correctly, this broke Structure::checkOffsetConsistency() in debug builds, because Structure::m_propertyTable may now get clear()ed out by one marking thread while this code executes in another.

My reading is that this bug only effects checkOffsetConsistency(). I think you could fix it by using a local temporary for m_propertyTable, instead of re-reading the data member. That way, you'll get null or non-null consistently throughout the function, which should be fine.

Perhaps you could beef up my reading by making a propertyTable() helper function, having that function ASSERT that the GC is not running, and then allowing checkOffsetConsistency() to call propertyTableUnsafe(), which does not ASSERT, or something like that.
Comment 27 Andreas Kling 2013-02-28 04:35:09 PST
(In reply to comment #26)
> > Okay, so if I understand correctly, this broke Structure::checkOffsetConsistency() in debug builds, because Structure::m_propertyTable may now get clear()ed out by one marking thread while this code executes in another.
> 
> My reading is that this bug only effects checkOffsetConsistency(). I think you could fix it by using a local temporary for m_propertyTable, instead of re-reading the data member. That way, you'll get null or non-null consistently throughout the function, which should be fine.
> 
> Perhaps you could beef up my reading by making a propertyTable() helper function, having that function ASSERT that the GC is not running, and then allowing checkOffsetConsistency() to call propertyTableUnsafe(), which does not ASSERT, or something like that.

Sure, could do something like this for checkOffsetConsistency():

inline const PropertyTable* Structure::propertyTableUnsafe() const
{
    return static_cast<const PropertyTable*>(*const_cast<Structure*>(this)->m_propertyTable.slot());
}

What can I assert in Structure::propertyTable() to verify that GC isn't in progress? Heap::isBusy()?
Comment 28 Geoffrey Garen 2013-02-28 10:01:24 PST
> inline const PropertyTable* Structure::propertyTableUnsafe() const
> {
>     return static_cast<const PropertyTable*>(*const_cast<Structure*>(this)->m_propertyTable.slot());
> }

Are you trying to get around multiple read problems in WriteBarrier<T>::get()? How about just "return m_propertyTable.get()", and fix WriteBarrier<T>::get() to put m_cell in a local variable, too, with a comment as to why.

> What can I assert in Structure::propertyTable() to verify that GC isn't in progress? Heap::isBusy()?

Yup!
Comment 29 Eric Seidel (no email) 2013-03-01 10:39:19 PST
Comment on attachment 190221 [details]
Proposed patch

Cleared Filip Pizlo's review+ from obsolete attachment 190221 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 30 Andreas Kling 2013-03-03 13:56:34 PST
(In reply to comment #28)
> > What can I assert in Structure::propertyTable() to verify that GC isn't in progress? Heap::isBusy()?
> 
> Yup!

Looks like this would necessitate passing a JSGlobalData& to propertyTable() everywhere, which gets pretty ugly. Do we still want to do it?
Comment 31 Geoffrey Garen 2013-03-04 09:41:41 PST
> Looks like this would necessitate passing a JSGlobalData& to propertyTable() everywhere, which gets pretty ugly. Do we still want to do it?

If that's too cumbersome, you can do this instead:

ASSERT(!globalObject() || !globalObject()->globalData()->heap.isBusy())

Every Structure has a pointer to the global object from which it originated, except for a few special permanent structures that don't belong to any global object.
Comment 32 Andreas Kling 2013-03-04 13:09:50 PST
Created attachment 191298 [details]
Patch II: The Repatchening

Yeah okay, let's try this again.
Comment 33 WebKit Review Bot 2013-03-04 13:13:31 PST
Attachment 191298 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.gypi', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/runtime/JSGlobalData.cpp', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/PropertyMapHashTable.h', u'Source/JavaScriptCore/runtime/PropertyTable.cpp', u'Source/JavaScriptCore/runtime/Structure.cpp', u'Source/JavaScriptCore/runtime/Structure.h', u'Source/JavaScriptCore/runtime/StructureInlines.h', u'Source/JavaScriptCore/runtime/WriteBarrier.h']" exit_code: 1
Source/JavaScriptCore/runtime/PropertyTable.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 Geoffrey Garen 2013-03-04 13:39:52 PST
Comment on attachment 191298 [details]
Patch II: The Repatchening

View in context: https://bugs.webkit.org/attachment.cgi?id=191298&action=review

r=me

> Source/JavaScriptCore/runtime/Structure.h:457
> +    // Should be accessed through propertyTable().

Would be nice to elaborate in this comment that what makes m_propertyTableUnsafe unsafe is that, during GC, it may be set to 0 by another thread.

> Source/JavaScriptCore/runtime/WriteBarrier.h:102
> +        // Copy cell to a local to avoid multiple-read issues. See <http://webkit.org/b/110854>)

Missing a parenthesis here.
Comment 35 Andreas Kling 2013-03-04 15:25:42 PST
Created attachment 191323 [details]
Patch for lunar landing
Comment 36 WebKit Review Bot 2013-03-04 15:29:49 PST
Comment on attachment 191323 [details]
Patch for lunar landing

Rejecting attachment 191323 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'validate-changelog', '--non-interactive', 191323, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Geoff Garen found in /mnt/git/webkit-commit-queue/Source/JavaScriptCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
/mnt/git/webkit-commit-queue/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/16903145
Comment 37 Andreas Kling 2013-03-04 15:31:34 PST
Created attachment 191324 [details]
Geoffrey not Geoff.
Comment 38 WebKit Review Bot 2013-03-04 19:07:31 PST
Comment on attachment 191324 [details]
Geoffrey not Geoff.

Clearing flags on attachment: 191324

Committed r144708: <http://trac.webkit.org/changeset/144708>
Comment 39 WebKit Review Bot 2013-03-04 19:07:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 40 WebKit Review Bot 2013-03-05 09:04:15 PST
Re-opened since this is blocked by bug 111447
Comment 41 Andreas Kling 2013-03-05 09:28:17 PST
0xbbadbeef
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000101dd797d JSC::PropertyTable::add(JSC::PropertyMapEntry const&, int&, JSC::PropertyTable::EffectOnPropertyOffset) + 509 (PropertyMapHashTable.h:374)
1   com.apple.JavaScriptCore      	0x0000000101dd4fa4 JSC::Structure::materializePropertyMap(JSC::JSGlobalData&) + 660 (Structure.cpp:265)
2   com.apple.JavaScriptCore      	0x0000000101dd55a8 JSC::Structure::addPropertyTransition(JSC::JSGlobalData&, JSC::Structure*, JSC::PropertyName, unsigned int, JSC::JSCell*, int&) + 744 (Structure.cpp:386)
3   com.apple.JavaScriptCore      	0x0000000101b49255 bool JSC::JSObject::putDirectInternal<(JSC::JSObject::PutMode)1>(JSC::JSGlobalData&, JSC::PropertyName, JSC::JSValue, unsigned int, JSC::PutPropertySlot&, JSC::JSCell*) + 901 (JSObject.h:1320)
4   com.apple.JavaScriptCore      	0x0000000101d3ed76 llint_slow_path_put_by_id + 262 (JSObject.h:1375)
5   com.apple.JavaScriptCore      	0x0000000101d46bb0 llint_op_put_by_id + 133
6   com.apple.JavaScriptCore      	0x0000000101c6fded JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 4253 (JSCJSValueInlines.h:363)
7   com.apple.JavaScriptCore      	0x0000000101b9533b JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) + 619 (Completion.cpp:75)
8   com.apple.WebCore             	0x0000000102fb141a WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) + 442 (JSMainThreadExecState.h:77)
9   com.apple.WebCore             	0x0000000102fb15a9 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) + 41 (ScriptController.cpp:158)
10  com.apple.WebCore             	0x0000000102fba854 WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) + 500 (ScriptValue.h:52)
11  com.apple.WebCore             	0x00000001028c5164 WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent(WebCore::PendingScript&) + 228 (HTMLScriptRunner.cpp:144)
12  com.apple.WebCore             	0x00000001028c5063 WebCore::HTMLScriptRunner::executeParsingBlockingScript() + 259 (HTMLScriptRunner.cpp:119)
13  com.apple.WebCore             	0x00000001028c5748 WebCore::HTMLScriptRunner::executeParsingBlockingScripts() + 24 (RefPtr.h:58)
14  com.apple.WebCore             	0x00000001028704d4 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() + 84 (PassRefPtr.h:68)
15  com.apple.WebCore             	0x0000000102870558 WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) + 88 (HTMLDocumentParser.cpp:287)
16  com.apple.WebCore             	0x0000000102870236 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 390 (HTMLDocumentParser.cpp:520)
17  com.apple.WebCore             	0x0000000102870b22 WebCore::HTMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>) + 498 (HTMLDocumentParser.cpp:732)
18  com.apple.WebCore             	0x00000001026189e5 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long) + 117 (PassRefPtr.h:68)
19  com.apple.WebCore             	0x000000010264d29c WebCore::DocumentLoader::commitData(char const*, unsigned long) + 508 (DocumentLoader.cpp:376)
20  com.apple.WebKit              	0x00000001021ab4a4 -[WebHTMLRepresentation receivedData:withDataSource:] + 100 (WebHTMLRepresentation.mm:186)
21  com.apple.WebKit              	0x000000010217e69d -[WebDataSource(WebInternal) _receivedData:] + 77 (WebDataSource.mm:216)
22  com.apple.WebKit              	0x00000001021964f7 WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) + 103 (WebFrameLoaderClient.mm:848)
23  com.apple.WebCore             	0x000000010264d340 WebCore::DocumentLoader::commitLoad(char const*, int) + 144 (RefCounted.h:148)
24  com.apple.WebCore             	0x0000000102d66ccb WebCore::MainResourceLoader::dataReceived(WebCore::CachedResource*, char const*, int) + 715 (RefCounted.h:148)
25  com.apple.WebCore             	0x00000001024ec472 WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient*) + 802 (HashTable.h:996)
26  com.apple.WebCore             	0x000000010317c78f WebCore::ThreadTimers::sharedTimerFiredInternal() + 175 (ThreadTimers.cpp:132)
27  com.apple.WebCore             	0x0000000103006cf3 _ZN7WebCoreL10timerFiredEP16__CFRunLoopTimerPv + 51 (SharedTimerMac.mm:167)
28  com.apple.CoreFoundation      	0x00007fff8b610934 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
29  com.apple.CoreFoundation      	0x00007fff8b610486 __CFRunLoopDoTimer + 534
30  com.apple.CoreFoundation      	0x00007fff8b5f0e11 __CFRunLoopRun + 1617
31  com.apple.CoreFoundation      	0x00007fff8b5f0486 CFRunLoopRunSpecific + 230
32  com.apple.Foundation          	0x00007fff884baf7b -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 267
33  DumpRenderTree                	0x0000000101a310be _ZL7runTestRKNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE + 1639 (DumpRenderTree.mm:1375)
34  DumpRenderTree                	0x0000000101a30852 dumpRenderTree(int, char const**) + 1727 (DumpRenderTree.mm:832)
35  DumpRenderTree                	0x0000000101a31427 main + 86 (DumpRenderTree.mm:925)
36  DumpRenderTree                	0x0000000101a25024 start + 52
Comment 42 Andreas Kling 2013-03-06 04:59:52 PST
Relanded in <https://trac.webkit.org/r144910>.

The issue was that transition->m_offset was not always initialized prior to calling transition->materializePropertyMap(). It was trivial to reproduce before with COLLECT_ON_EVERY_ALLOCATION.

Added a method to share the "take ownership of existing property map or clone it if it's pinned" logic between addPropertyTransition() and nonPropertyTransition().