MapDataImpl::add() does find(), then add(). This is twice the amount of work we should be doing.
Created attachment 252626 [details] Patch idea
Regressions: Unexpected crashes (1) js/map-grow-with-holes.html [ Crash ]
Comment on attachment 252626 [details] Patch idea View in context: https://bugs.webkit.org/attachment.cgi?id=252626&action=review > Source/JavaScriptCore/runtime/MapDataInlines.h:99 > + if (!ensureSpaceForAppend(exec, owner)) { When extending the backing store, MapDataImpl rehashes itself (replaceAndPackBackingStore). At that time, we execute `ptr->value = m_entries[ptr->value].key().get().asInt32();`. So, if the map contains the key-value pair (key, m_size) and if the m_size is out of the range of the previous backing store (before extending it), SEGV occcurs.
Heh, that's what I get for not even compiling before I upload :|
Created attachment 252644 [details] Patch
Comment on attachment 252644 [details] Patch Thanks for wrapping that up!
Created attachment 252650 [details] Patch
Comment on attachment 252650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252650&action=review > Source/JavaScriptCore/runtime/MapDataInlines.h:100 > + return &m_entries[location->value]; s/location/result.iterator/
Created attachment 252656 [details] Patch
Comment on attachment 252656 [details] Patch Attachment 252656 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6431934037098496 New failing tests: js/regress/basic-set.html inspector/model/parse-script-syntax-tree.html js/set-foreach-calls-back-with-right-args.html inspector/css/matched-style-properties.html fast/storage/serialized-script-value.html inspector/css/pseudo-element-matches.html inspector/console/console-api.html js/basic-map.html js/map-foreach-calls-back-with-right-args.html inspector/test-harness-trivially-works.html js/array-from.html inspector/css/selector-specificity.html fast/dom/Window/window-postmessage-clone.html js/basic-set.html inspector/event-listener-set.html inspector/protocol-promise-result.html js/symbol-in-map.html
Created attachment 252662 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 252656 [details] Patch Attachment 252656 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6582692757897216 New failing tests: js/regress/basic-set.html inspector/model/parse-script-syntax-tree.html http/tests/inspector/css/bad-mime-type.html inspector/css/matched-style-properties.html fast/storage/serialized-script-value.html inspector/css/pseudo-element-matches.html js/symbol-in-map.html js/basic-map.html js/map-foreach-calls-back-with-right-args.html inspector/test-harness-trivially-works.html js/array-from.html inspector/css/selector-specificity.html js/set-foreach-calls-back-with-right-args.html js/basic-set.html inspector/event-listener-set.html inspector/protocol-promise-result.html
Created attachment 252664 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 252722 [details] Patch
Created attachment 252732 [details] Patch
Comment on attachment 252732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252732&action=review > Source/JavaScriptCore/runtime/MapDataInlines.h:178 > RELEASE_ASSERT(newCapacity > 0); ASSERT please.
Committed r184009: <http://trac.webkit.org/changeset/184009>
Comment on attachment 252732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252732&action=review > Source/JavaScriptCore/runtime/MapDataInlines.h:200 > + for (auto ptr = m_valueKeyedTable.begin(); ptr != m_valueKeyedTable.end(); ++ptr) { Modern for loop would make these for loops nicer to read.
Comment on attachment 252732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252732&action=review >> Source/JavaScriptCore/runtime/MapDataInlines.h:200 >> + for (auto ptr = m_valueKeyedTable.begin(); ptr != m_valueKeyedTable.end(); ++ptr) { > > Modern for loop would make these for loops nicer to read. I think this code predates us being allowed to use them. Is it worth rewriting as part of this patch? I'm torn between doing it (as i'm changing the lines any way) and not doing it (unrelated to the fix?)
I think this is causing the following crash on ASan builds: ==98628==ERROR: AddressSanitizer: SEGV on unknown address 0x0000bbadbeef (pc 0x00010bbb9cc0 bp 0x00011da687b0 sp 0x00011da687a0 T13) #0 0x10bbb9cbf in 2015-05-11 16:19:43.470 atos[98896:1972233] Metadata.framework [Error]: couldn't get the client port (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x291cbf) #1 0x10c2927a4 in WTFCrash (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x96a7a4) #2 0x10bcf7b7e in JSC::MapDataImpl<JSC::JSMap::Entry, JSC::JSMapIterator>::copyBackingStore(JSC::CopyVisitor&, JSC::CopyToken) (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x3cfb7e) #3 0x10bb96736 in JSC::CopyVisitor::visitItem(JSC::CopyWorklistItem) (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x26e736) #4 0x10b93aab0 in JSC::CopyVisitor::copyFromShared() (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x12ab0) #5 0x10b931abd in JSC::GCThread::gcThreadMain() (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x9abd) #6 0x10b93196d in WTF::threadEntryPoint(void*) (/Volumes/Data/slave/yosemite-asan-production-wk1-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x996d) #7 0x7fff8e12f267 in WTF::wtfThreadEntryPoint(void*) (/usr/lib/system/libsystem_pthread.dylib+0x3267) #8 0x7fff8e12f1e4 in _pthread_body (/usr/lib/system/libsystem_pthread.dylib+0x31e4) #9 0x7fff8e12d41c in _pthread_start (/usr/lib/system/libsystem_pthread.dylib+0x141c)
The regression is tracked as <rdar://problem/20908178>. Rolling out.
Re-opened since this is blocked by bug 144900
(In reply to comment #21) > The regression is tracked as <rdar://problem/20908178>. Rolling out. Gah, sorry for disappearing was trapped in my real job :D Thanks for rolling it out.
Comment on attachment 252732 [details] Patch r- as it was breaking asan. Alexey: how do i do an asan build?
Our hash map implementation is changed.