I think there is another case of iterating m_windowShells when it could be mutated in ScriptController::updateDocument: <http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r171485%20(5831)/fast/html/details-nested-1-crash-log.txt> ASSERTION FAILED: m_table /Volumes/Data/slave/mavericks-debug/build/WebKitBuild/Debug/usr/local/include/wtf/HashTable.h(210) : void WTF::HashTableConstIterator<WTF::RefPtr<WebCore::DOMWrapperWorld>, WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::PtrHash<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::KeyValuePairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::HashTraits<JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> > >::checkValidity() const [Key = WTF::RefPtr<WebCore::DOMWrapperWorld>, Value = WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> >, Extractor = WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> > >, HashFunctions = WTF::PtrHash<WTF::RefPtr<WebCore::DOMWrapperWorld> >, Traits = WTF::KeyValuePairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::HashTraits<JSC::Strong<WebCore::JSDOMWindowShell> > >, KeyTraits = WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> >] 1 0x10b5c5b90 WTFCrash 2 0x10ce7a242 WTF::HashTableConstIterator<WTF::RefPtr<WebCore::DOMWrapperWorld>, WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::PtrHash<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::KeyValuePairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::HashTraits<JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> > >::checkValidity() const 3 0x10e2684e9 WTF::HashTableConstIterator<WTF::RefPtr<WebCore::DOMWrapperWorld>, WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::PtrHash<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::KeyValuePairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::HashTraits<JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> > >::operator++() 4 0x10e2684b9 WTF::HashTableIterator<WTF::RefPtr<WebCore::DOMWrapperWorld>, WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::PtrHash<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::KeyValuePairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::HashTraits<JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> > >::operator++() 5 0x10e266419 WTF::HashTableIteratorAdapter<WTF::HashTable<WTF::RefPtr<WebCore::DOMWrapperWorld>, WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::PtrHash<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::KeyValuePairHashTraits<WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> >, WTF::HashTraits<JSC::Strong<WebCore::JSDOMWindowShell> > >, WTF::HashTraits<WTF::RefPtr<WebCore::DOMWrapperWorld> > >, WTF::KeyValuePair<WTF::RefPtr<WebCore::DOMWrapperWorld>, JSC::Strong<WebCore::JSDOMWindowShell> > >::operator++() 6 0x10e26b6bb WebCore::ScriptController::updateDocument() 7 0x10cd91b0b WebCore::Document::didBecomeCurrentDocumentInFrame() 8 0x10d0e847f WebCore::Frame::setDocument(WTF::PassRefPtr<WebCore::Document>) 9 0x10ce1dfb5 WebCore::DocumentWriter::begin(WebCore::URL const&, bool, WebCore::Document*) 10 0x10cde4caf WebCore::DocumentLoader::commitData(char const*, unsigned long) 11 0x10cde4779 WebCore::DocumentLoader::finishedLoading(double) 12 0x10cde9691 WebCore::DocumentLoader::maybeLoadEmpty() 13 0x10cde9802 WebCore::DocumentLoader::startLoadingMainResource() 14 0x10d0fcd59 WebCore::FrameLoader::continueLoadAfterWillSubmitForm() 15 0x10d0f92a2 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool) 16 0x10d1061b2 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>)::$_4::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool) const This is most likely caused by: <https://webkit.org/b/135178> JSDOMWindowShell leaks on pages with media elements We should be able to work around this in the same way that did by not iterating over the live map.
> We should be able to work around this in the same way that did by not iterating over the live map. Actually, this wants both the key and the value. =/
Can we just copy and iterate the copy?
(In reply to comment #1) > > We should be able to work around this in the same way that did by not iterating over the live map. > > Actually, this wants both the key and the value. =/ We can get the value (DOMWrapperWorld) from the value (JSDOMWindowShell) JSDOMWindowShell::world, so copyValuesToVector is fine. (In reply to comment #2) > Can we just copy and iterate the copy? Looks like we can, but that also seems like it would do more work. Would that be preferred?
Created attachment 235386 [details] [PATCH] Proposed Fix Saw no ASSERTs running "fast" tests with a debug WebKit. I'll do some more local testing experimenting with JSC_slowPathAllocsBetweenGCs.
Comment on attachment 235386 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review > Source/WebCore/ChangeLog:10 > + Avoid iterating over m_windowShells in more places. This prevents > + the possibility of a collection during JSC allocation which might > + cause a mutation to m_windowShells (HTMLMediaElement destruction). This is phrased poorly. It doesn't prevent the possibility of a collection. It avoids the mutation of m_windowShells during iteration. I'll rewrite it.
Comment on attachment 235386 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review > Source/WebCore/bindings/js/ScriptController.cpp:177 > + return windowShells; will this get happy move semantics?
Comment on attachment 235386 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review >> Source/WebCore/bindings/js/ScriptController.cpp:177 >> + return windowShells; > > will this get happy move semantics? will this get happy move semantics?
Comment on attachment 235386 [details] [PATCH] Proposed Fix Attachment 235386 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5984569622790144 New failing tests: media/track/add-and-remove-track.html
Created attachment 235390 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #7) > >> Source/WebCore/bindings/js/ScriptController.cpp:177 > >> + return windowShells; > > > > will this get happy move semantics? I expect it to. I shouldn't need to do anything special, right?
(In reply to comment #10) > (In reply to comment #7) > > >> Source/WebCore/bindings/js/ScriptController.cpp:177 > > >> + return windowShells; > > > > > > will this get happy move semantics? > > I expect it to. I shouldn't need to do anything special, right? Asking around, yes this should. I don't need to do anything special. To be explicit, we could WTF::move(...).
Comment on attachment 235386 [details] [PATCH] Proposed Fix Clearing flags on attachment: 235386 Committed r171505: <http://trac.webkit.org/changeset/171505>
All reviewed patches have been landed. Closing bug.
Comment on attachment 235386 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review > Source/WebCore/bindings/js/ScriptCachedFrameData.cpp:54 > + Vector<JSC::Strong<JSDOMWindowShell>> windowShells = scriptController.windowShells(); Can a new window shell be added to m_windowShells during iteration? If this happens, will it remain in a broken state forever, not being subject to the below processing?
(In reply to comment #14) > (From update of attachment 235386 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235386&action=review > > > Source/WebCore/bindings/js/ScriptCachedFrameData.cpp:54 > > + Vector<JSC::Strong<JSDOMWindowShell>> windowShells = scriptController.windowShells(); > > Can a new window shell be added to m_windowShells during iteration? If this happens, will it remain in a broken state forever, not being subject to the below processing? Not currently, otherwise I would have expected to see a crash/assert here before. I also don't think we ever want mutation of a Hash* during iteration, even if it is additive. I believe that in HashMap/HashSet iteration order is not guaranteed; so depending on that behavior would be bad. Please correct me if I'm wrong! We need to prevent against a removal of a window shell in m_windowShells during iteration because that can now happen at any time (GC of a media element). Creating a window shell still happens in only a few places.
> I believe that in HashMap/HashSet iteration order is not guaranteed; so depending on that behavior would be bad. Please correct me if I'm wrong! This is certainly correct, one can't iterate over a container that's changing. > We need to prevent against a removal of a window shell in m_windowShells during iteration because that can now happen at any time (GC of a media element). Creating a window shell still happens in only a few places. OK, sounds good. I'm always worried about such copy-to-iterate fixes, because they can be papering over a problem, leaving us with a different random misbehavior that's almost impossible to diagnose. If at all possible, it's desirable to at least to have an ASSERT that we don't have any new elements in the map after iteration.