Bug 135211 – ScriptController::updateDocument ASSERT mutating map while iterating map

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

> 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. =/

[Geoffrey Garen]

Can we just copy and iterate the copy?

[Joseph Pecoraro]

(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?

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Oliver Hunt]

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?

[Oliver Hunt]

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?

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

(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?

[Joseph Pecoraro]

(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(...).

[WebKit Commit Bot]

Comment on attachment 235386 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 235386

Committed r171505: <http://trac.webkit.org/changeset/171505>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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?

[Joseph Pecoraro]

(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.

[Alexey Proskuryakov]

> 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.