Bug 147290 – Crash happens when calling removeEventListener for an SVG element which has an instance inside a <defs> element of shadow tree

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Created attachment 257510 [details]
test case (will crash)

1. Open the attached test case.

Result: WebKit crashes with the following call stack.

0   com.apple.JavaScriptCore      	0x0000000113384a17 WTFCrash + 39
1   com.apple.WebCore             	0x0000000116ab1e7a WebCore::SVGElement::removeEventListener(WTF::AtomicString const&, WebCore::EventListener*, bool) + 554 (SVGElement.cpp:620)
2   com.apple.WebCore             	0x0000000115d5c316 WebCore::jsNodePrototypeFunctionRemoveEventListener(JSC::ExecState*) + 678 (JSNode.cpp:872)
3   ???                           	0x00004539c5601028 0 + 76114426859560
4   com.apple.JavaScriptCore      	0x000000011311304e llint_entry + 25874
5   com.apple.JavaScriptCore      	0x000000011310c8f9 vmEntryToJavaScript + 361
6   com.apple.JavaScriptCore      	0x0000000112f6936c JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 252
7   com.apple.JavaScriptCore      	0x0000000112f4d7d8 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1464
8   com.apple.JavaScriptCore      	0x000000011299c84e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 190
9   com.apple.JavaScriptCore      	0x000000011299c8b3 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 83

[Said Abou-Hallawa]

<rdar://problem/21915355>

[Alexey Proskuryakov]

This stack shows an assertion failure, but sounds like there is also a crash in release mode - is that correct?

The attached test case doesn't crash in shipping Safari/WebKit for me.

[Said Abou-Hallawa]

(In reply to comment #2)
> This stack shows an assertion failure, but sounds like there is also a crash
> in release mode - is that correct?
> 
> The attached test case doesn't crash in shipping Safari/WebKit for me.

Yes the above is an assertion failure. Here is the crashing call stack which I got when opening the attached text case in the latest Safari

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff896746f4 WebCore::EventListenerMap::removeFirstEventListenerCreatedFromMarkup(WTF::AtomicString const&) + 20
1   com.apple.WebCore             	0x00007fff89efdca2 WebCore::SVGElement::removeEventListener(WTF::AtomicString const&, WebCore::EventListener*, bool) + 290
2   com.apple.WebCore             	0x00007fff89250578 WebCore::jsNodePrototypeFunctionRemoveEventListener(JSC::ExecState*) + 424
3   ???                           	0x00003a8434e01028 0 + 64339497193512
4   com.apple.JavaScriptCore      	0x00007fff8548a376 llint_entry + 22925
5   com.apple.JavaScriptCore      	0x00007fff854847d9 vmEntryToJavaScript + 326
6   com.apple.JavaScriptCore      	0x00007fff853b3419 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169
7   com.apple.JavaScriptCore      	0x00007fff84f884ad JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 493
8   com.apple.JavaScriptCore      	0x00007fff85125507 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 71
9   com.apple.WebCore             	0x00007fff8924f41a WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 1002
10  com.apple.WebCore             	0x00007fff896778db WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow, 16ul>&) + 635
11  com.apple.WebCore             	0x00007fff8915a390 WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 224
12  com.apple.WebCore             	0x00007fff891b0414 WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>) + 260
13  com.apple.WebCore             	0x00007fff891c251b WebCore::DOMWindow::dispatchLoadEvent() + 347
14  com.apple.WebCore             	0x00007fff89185dd4 WebCore::Document::implicitClose() + 324
15  com.apple.WebCore             	0x00007fff89185853 WebCore::FrameLoader::checkCompleted() + 275
16  com.apple.WebCore             	0x00007fff89184624 WebCore::FrameLoader::finishedParsing() + 100
17  com.apple.WebCore             	0x00007fff891833c1 WebCore::Document::finishedParsing() + 417
18  com.apple.WebCore             	0x00007fff8915b9aa WebCore::DocumentWriter::end() + 58
19  com.apple.WebCore             	0x00007fff8914d55c WebCore::DocumentLoader::finishedLoading(double) + 268
20  com.apple.WebCore             	0x00007fff891e8809 WebCore::CachedResource::checkNotify() + 153
21  com.apple.WebCore             	0x00007fff894abe43 WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 227
22  com.apple.WebCore             	0x00007fff891e86a1 WebCore::SubresourceLoader::didFinishLoading(double) + 1153
23  com.apple.WebKit              	0x00007fff8ec57d0d WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) + 561
24  com.apple.WebKit              	0x00007fff8ea9b458 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 102
25  com.apple.WebKit              	0x00007fff8ea9d984 IPC::Connection::dispatchOneMessage() + 114
26  com.apple.JavaScriptCore      	0x00007fff85570b45 WTF::RunLoop::performWork() + 437
27  com.apple.JavaScriptCore      	0x00007fff85571222 WTF::RunLoop::performWork(void*) + 34
28  com.apple.CoreFoundation      	0x00007fff8adf8ac1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
29  com.apple.CoreFoundation      	0x00007fff8adeb0bc __CFRunLoopDoSources0 + 556
30  com.apple.CoreFoundation      	0x00007fff8adea5df __CFRunLoopRun + 927
31  com.apple.CoreFoundation      	0x00007fff8ade9fd8 CFRunLoopRunSpecific + 296
32  com.apple.HIToolbox           	0x00007fff8b6a86a1 RunCurrentEventLoopInMode + 235
33  com.apple.HIToolbox           	0x00007fff8b6a8437 ReceiveNextEventCommon + 432
34  com.apple.HIToolbox           	0x00007fff8b6a8277 _BlockUntilNextEventMatchingListInModeWithFilter + 71
35  com.apple.AppKit              	0x00007fff8d6e106f _DPSNextEvent + 1076
36  com.apple.AppKit              	0x00007fff8daae597 -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 423
37  com.apple.AppKit              	0x00007fff8d6d6efa -[NSApplication run] + 682
38  com.apple.AppKit              	0x00007fff8d6590b2 NSApplicationMain + 1176
39  libxpc.dylib                  	0x00007fff91d08fa0 _xpc_objc_main + 793
40  libxpc.dylib                  	0x00007fff91d0a6ef xpc_main + 494
41  com.apple.WebKit.WebContent   	0x0000000105b79b4a 0x105b79000 + 2890
42  libdyld.dylib                 	0x00007fff950075ad start + 1

[Said Abou-Hallawa]

This crash happens under these conditions:

1. An element 'parent' is defined to be referenced later 
2. A <defs> element 'parent-defs' is included inside 'parent'
3. An element 'child' is defined inside 'parent-defs'
4. A visible <use> element 'use-parent' references 'parent'
5. A addEventListener is made for 'child' before the onload fires
6. A removeEventListener is made for 'child' after the onload fires

Here is the explanation of why the crash happens in this case:

1. Because addEventListener() was called before onload fires, the event is only added to the element itself. No instances are created before the onload event.
2. SVGUseElement::updateShadowTree() is called as part of resolving the style of 'use-parent'. It calls cloneTarget() and then calls transferEventListenersToShadowTree().
3. 'use-parent' references 'parent' so the whole 'parent' tree is cloned as a shadow tree for 'use-parent'
4. 'parent-defs' is a child of 'parent' so it is cloned and its subtree which includes the cloned 'child'
5. The cloned 'child' is added to the instances()  of 'child' and its correspondingElement is set to 'child'
6. After finishing the cloning, removeDisallowedElementsFromSubtree() is called which removes the cloned 'parent-defs' from the shadow subtree. The cloned 'parent-defs' includes the clone of 'child' which is now inaccessible from 'use-parent'.
7. When transferEventListenersToShadowTree() is called we fail to transfer the EventListener from the 'child' to the cloned 'child' since its parent cloned 'parent-defs' was removed from the shadow tree of the 'parent' element. Yet the 'child' instances() list still contains the cloned 'child'
8. When SVGElement::removeEventListener() is called for 'child' 'element', it loops through its instances() list and calls Node::removeEventListener() for each instance. We fail to removeEventListener() for the cloned 'child' since addEventListener() was never called for it. So we end up crashing when calling EventListenerMap::removeFirstEventListenerCreatedFromMarkup() since instance->eventTargetData() is null. The only way to get it valid is to call addEventListener() which did not happen.

[Said Abou-Hallawa]

Created attachment 257579 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 257579 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        are disallowed and removed. Make sure, when disallowing  an element in the

Double space between "disallowing" and "an"

[Said Abou-Hallawa]

Created attachment 257595 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 257595 [details]
Patch

Rejecting attachment 257595 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 257595, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5146182305185792

[Said Abou-Hallawa]

Created attachment 257606 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 257606 [details]
Patch

Clearing flags on attachment: 257606

Committed r187463: <http://trac.webkit.org/changeset/187463>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

rdar://problem/21915355

[Alexey Proskuryakov]

This caused memory corruption on SVG tests, detected by GuardMalloc and ASan.

[Alexey Proskuryakov]

Rolled out manually in r187486.

[Daniel Bates]

Re-opening bug per comment #14.

[Said Abou-Hallawa]

Created attachment 257657 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #13)
> This caused memory corruption on SVG tests, detected by GuardMalloc and ASan.

The memory corruption happened because pf calling descendantsOfType<SVGElement>(*element) after calling element->parentNode()->removeChild(element). removeChild() causes the element data to be garbage-collected so when firstChild() is called, a crash happens.

The fix is to call setCorrespondingElement() for the SVGElement descendants of element before removing the element from its parent children list. Also there is no need to call descendant.setCorrespondingElement(nullptr) if descendant.correspondingElement() is null. setCorrespondingElement() calls ensureSVGRareData() which allocates SVGElementRareData if it is null. All these descendants will be garbage-collected later anyway.

[Said Abou-Hallawa]

Created attachment 257664 [details]
Patch

[Said Abou-Hallawa]

This guardMalloc memory corruption actually emphasizes the original crash more and explains it better. We were not only trying to removeEventListener from an element which addEventListener was never called for it but we were referencing an abandoned child element. And the memory of this element is going to be claimed later by the garbage collector. So the type of crash depends on when the JavaScript call is made to WebCore to removeEventListener(). It can be referencing a null pointer or it can be referencing a freed memory.

[Daniel Bates]

Comment on attachment 257664 [details]
Patch

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

> Source/WebCore/svg/SVGUseElement.cpp:336
> +        for (auto& descendant : descendantsOfType<SVGElement>(*element)) {
> +            if (descendant.correspondingElement())
> +                descendant.setCorrespondingElement(nullptr);
> +        }

We should consider nullifying the corresponding element in general when we remove an SVG child (see ContainerNode::removeChild()). Notice that we currently invalidate the instance associated with the remove node in SVGElement::childrenChanged().

[Darin Adler]

(In reply to comment #17)
> Also
> there is no need to call descendant.setCorrespondingElement(nullptr) if
> descendant.correspondingElement() is null. setCorrespondingElement() calls
> ensureSVGRareData() which allocates SVGElementRareData if it is null.

I think this should be fixed by enhancing the setCorrespondingElement function to handle setting to null when it’s already null without allocating rare data, instead of working around it here.

[Said Abou-Hallawa]

(In reply to comment #20)
> Comment on attachment 257664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257664&action=review
> 
> > Source/WebCore/svg/SVGUseElement.cpp:336
> > +        for (auto& descendant : descendantsOfType<SVGElement>(*element)) {
> > +            if (descendant.correspondingElement())
> > +                descendant.setCorrespondingElement(nullptr);
> > +        }
> 
> We should consider nullifying the corresponding element in general when we
> remove an SVG child (see ContainerNode::removeChild()). Notice that we
> currently invalidate the instance associated with the remove node in
> SVGElement::childrenChanged().

I agree we should have a general solution which works for all the cases when the child is removed. However removeChild() is very generic and although SVGElement::childrenChanged() is called from the ContainerNode::removeChild(), knowing whether removing the child from the parent children tree will cause this child to be freed or not seems to need more investigation. I will log a separate bug to track this issue down.

[Said Abou-Hallawa]

(In reply to comment #21)
> (In reply to comment #17)
> > Also
> > there is no need to call descendant.setCorrespondingElement(nullptr) if
> > descendant.correspondingElement() is null. setCorrespondingElement() calls
> > ensureSVGRareData() which allocates SVGElementRareData if it is null.
> 
> I think this should be fixed by enhancing the setCorrespondingElement
> function to handle setting to null when it’s already null without allocating
> rare data, instead of working around it here.

Good suggestion I am going to do it and update the patch.

[Said Abou-Hallawa]

Created attachment 257672 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 257672 [details]
Patch

Clearing flags on attachment: 257672

Committed r187504: <http://trac.webkit.org/changeset/187504>

[Said Abou-Hallawa]

All reviewed patches have been landed.  Closing bug.