Bug 145209 – Use modern for-loops in WebCore/svg.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Hunseop Jeong]

Use the modern loops in WebCore/svg.

[Hunseop Jeong]

Created attachment 253438 [details]
Patch

[Darin Adler]

Comment on attachment 253438 [details]
Patch

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

> Source/WebCore/svg/SVGAnimatedPath.cpp:58
> +    for (auto& seg : result)

Should be named "segment" rather than "seg".

> Source/WebCore/svg/SVGPathByteStream.h:65
>      void append(SVGPathByteStream* other)
>      {
> -        for (DataIterator it = other->begin(); it != other->end(); ++it)
> -            append(*it);
> +        for (auto& stream : *other)
> +            append(stream);
>      }

Since this assumes the other is non-null, we should come back here and change that argument to be a reference rather than a pointer.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:98
> +    for (auto& reference : this->effectReferences(effect))

No need for "this->" any more, since the refactoring got rid of the local variable named effectReferences.

[Hunseop Jeong]

Created attachment 253517 [details]
Patch

[Hunseop Jeong]

Comment on attachment 253517 [details]
Patch

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

> Source/WebCore/svg/SVGAnimatedPath.cpp:59
> +        segment->animationStarted(byteStream.get(), &baseValue);

Changed the 'seg' to 'segment'. Done.

> Source/WebCore/svg/SVGPathByteStream.h:64
> +            append(stream);

Changed the argument to use the reference.

> Source/WebCore/svg/SVGPathUtilities.cpp:158
> +        result->append(*appendedByteStream);

I modify here, as the append() was modified to use the reference.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:99
> +        clearResultsRecursive(reference);

Removed 'this->'. Done.

[Darin Adler]

Comment on attachment 253517 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests, no new tests.

There is no reason to include a line like this in change log. If you keep this line, you use it to explain why there are no new tests. If you think it goes without saying, then please delete this line in future patches.

[WebKit Commit Bot]

Comment on attachment 253517 [details]
Patch

Clearing flags on attachment: 253517

Committed r184718: <http://trac.webkit.org/changeset/184718>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145277

[Jake Nielsen]

(In reply to comment #8)
> Re-opened since this is blocked by bug 145277

Looks like this caused some tests to start crashing:
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/4483/steps/layout-test/logs/stdio

[Alexey Proskuryakov]

So, rolling out.

[Alexey Proskuryakov]

In addition to assertions, this introduced a use-after-free detected by ASan and GuardMalloc.

==97888==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060006acac8 at pc 0x00011c7a3a0b bp 0x7fff50dc2100 sp 0x7fff50dc20f8
READ of size 8 at 0x6060006acac8 thread T0
==97888==WARNING: failed to fork external symbolizer (errno: 1)
    #0 0x11c7a3a0a in bool WTF::HashTraitsEmptyValueChecker<WTF::HashTraits<WebCore::SVGSVGElement*>, false>::isEmptyValue<WebCore::SVGSVGElement*>(WebCore::SVGSVGElement* const&) (WebCore.framework/Versions/A/WebCore+0x21ffa0a)
    #1 0x11c7a37dd in WTF::HashTable<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::isEmptyOrDeletedBucket(WebCore::SVGSVGElement* const&) (WebCore.framework/Versions/A/WebCore+0x21ff7dd)
    #2 0x11c7a67ac in WTF::HashTableConstIterator<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::skipEmptyBuckets() (WebCore.framework/Versions/A/WebCore+0x22027ac)
    #3 0x11c7a6bdd in WTF::HashTableConstIterator<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::operator++() (WebCore.framework/Versions/A/WebCore+0x2202bdd)
    #4 0x11c7a08bd in WTF::HashTableConstIteratorAdapter<WTF::HashTable<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >, WebCore::SVGSVGElement*>::operator++() (WebCore.framework/Versions/A/WebCore+0x21fc8bd)
    #5 0x11a913da7 in WebCore::SVGDocumentExtensions::dispatchSVGLoadEventToOutermostSVGElements() (WebCore.framework/Versions/A/WebCore+0x36fda7)
    #6 0x11a637dfa in WebCore::Document::implicitClose() (WebCore.framework/Versions/A/WebCore+0x93dfa)
    #7 0x11a636dfb in WebCore::FrameLoader::checkCompleted() (WebCore.framework/Versions/A/WebCore+0x92dfb)
    #8 0x11ac77123 in WebCore::CachedResourceLoader::loadDone(WebCore::CachedResource*, bool) (WebCore.framework/Versions/A/WebCore+0x6d3123)
    #9 0x11a6dece1 in WebCore::SubresourceLoader::notifyDone() (WebCore.framework/Versions/A/WebCore+0x13ace1)
    #10 0x11a6ddd40 in WebCore::SubresourceLoader::didFinishLoading(double) (WebCore.framework/Versions/A/WebCore+0x139d40)
    #11 0x117d42555 in void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double)>(IPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double)) (WebKit.framework/WebKit+0x93c555)
    #12 0x117d417ca in WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) (WebKit.framework/WebKit+0x93b7ca)
    #13 0x11777ea8a in WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) (WebKit.framework/WebKit+0x378a8a)
    #14 0x1175ed5d6 in IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) (WebKit.framework/WebKit+0x1e75d6)
    #15 0x1175f3ddc in IPC::Connection::dispatchOneMessage() (WebKit.framework/WebKit+0x1edddc)
    #16 0x119c0dcf4 in WTF::RunLoop::performWork() (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xd10cf4)
    #17 0x119c0eb9e in WTF::RunLoop::performWork(void*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xd11b9e)
    #18 0x7fff82c1ca00 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x80a00)
    #19 0x7fff82c0eb8c in __CFRunLoopDoSources0 (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x72b8c)
    #20 0x7fff82c0e1be in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x721be)
    #21 0x7fff82c0dbd7 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x71bd7)
    #22 0x7fff90e7156e in RunCurrentEventLoopInMode (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x3256e)
    #23 0x7fff90e712e9 in ReceiveNextEventCommon (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x322e9)
    #24 0x7fff90e7112a in _BlockUntilNextEventMatchingListInModeWithFilter (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x3212a)
    #25 0x7fff8e94198a in _DPSNextEvent (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x9198a)
    #26 0x7fff8e940f37 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x90f37)
    #27 0x7fff8e936bd2 in -[NSApplication run] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x86bd2)
    #28 0x7fff8e8b3323 in NSApplicationMain (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x3323)
    #29 0x7fff87bf1957 in _xpc_objc_main (/usr/lib/system/libxpc.dylib+0x16957)
    #30 0x7fff87bf305f in xpc_main (/usr/lib/system/libxpc.dylib+0x1805f)
    #31 0x10ee3c929 in ?? (WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.Development.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development+0x100001929)
    #32 0x7fff90d395c8 in start (/usr/lib/system/libdyld.dylib+0x35c8)
    #33 0x0  (<unknown module>)
 
0x6060006acac8 is located 8 bytes inside of 64-byte region [0x6060006acac0,0x6060006acb00)
freed by thread T0 here:
    #0 0x10ee92b89 in wrap_free (/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/7.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x42b89)
    #1 0x119c420d0 in bmalloc::Deallocator::deallocateSlowCase(void*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xd450d0)
    #2 0x11c7a3732 in WTF::HashTable<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::rehash(unsigned int, WebCore::SVGSVGElement**) (WebCore.framework/Versions/A/WebCore+0x21ff732)
    #3 0x11c7a3119 in WTF::HashTable<WebCore::SVGSVGElement*, WebCore::SVGSVGElement*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::add(WebCore::SVGSVGElement* const&) (WebCore.framework/Versions/A/WebCore+0x21ff119)
    #4 0x11c79ffbd in WTF::HashSet<WebCore::SVGSVGElement*, WTF::PtrHash<WebCore::SVGSVGElement*>, WTF::HashTraits<WebCore::SVGSVGElement*> >::add(WebCore::SVGSVGElement* const&) (WebCore.framework/Versions/A/WebCore+0x21fbfbd)
    #5 0x11a865bb7 in WebCore::SVGDocumentExtensions::addTimeContainer(WebCore::SVGSVGElement*) (WebCore.framework/Versions/A/WebCore+0x2c1bb7)
    #6 0x11c883936 in WebCore::SVGSVGElement::insertedInto(WebCore::ContainerNode&) (WebCore.framework/Versions/A/WebCore+0x22df936)
    #7 0x11ad354c9 in WebCore::ChildNodeInsertionNotifier::notifyNodeInsertedIntoDocument(WebCore::Node&) (WebCore.framework/Versions/A/WebCore+0x7914c9)
    #8 0x11ad346fb in WebCore::ChildNodeInsertionNotifier::notify(WebCore::Node&) (WebCore.framework/Versions/A/WebCore+0x7906fb)
    #9 0x11ad2e246 in WebCore::ContainerNode::updateTreeAfterInsertion(WebCore::Node&) (WebCore.framework/Versions/A/WebCore+0x78a246)
    #10 0x11ad2ddcc in WebCore::ContainerNode::appendChild(WTF::PassRefPtr<WebCore::Node>, int&) (WebCore.framework/Versions/A/WebCore+0x789dcc)
    #11 0x11c07aae5 in WebCore::Node::appendChild(WTF::PassRefPtr<WebCore::Node>, int&) (WebCore.framework/Versions/A/WebCore+0x1ad6ae5)
    #12 0x11a7440b1 in WebCore::JSNode::appendChild(JSC::ExecState*) (WebCore.framework/Versions/A/WebCore+0x1a00b1)
    #13 0x11a743ef3 in WebCore::jsNodePrototypeFunctionAppendChild(JSC::ExecState*) (WebCore.framework/Versions/A/WebCore+0x19fef3)
    #14 0x3bc07d601027  (<unknown module>)
    #15 0x1199daa03 in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xadda03)
    #16 0x1199daa03 in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xadda03)
    #17 0x1199dab6e in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xaddb6e)
    #18 0x1199daa6e in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xadda6e)
    #19 0x1199d4ed5 in vmEntryToJavaScript (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xad7ed5)
    #20 0x1197e197f in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x8e497f)
    #21 0x118f6f25a in JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x7225a)
    #22 0x118f6ee71 in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x71e71)
    #23 0x119244291 in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, JSC::JSValue*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x347291)
    #24 0x11b7feb07 in WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, JSC::JSValue*) (WebCore.framework/Versions/A/WebCore+0x125ab07)
    #25 0x11a78590e in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) (WebCore.framework/Versions/A/WebCore+0x1e190e)
    #26 0x11b1fc18e in WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow, 16ul>&) (WebCore.framework/Versions/A/WebCore+0xc5818e)
    #27 0x11a5f90c1 in WebCore::EventTarget::fireEventListeners(WebCore::Event*) (WebCore.framework/Versions/A/WebCore+0x550c1)
    #28 0x11b1d8297 in WebCore::EventContext::handleLocalEvents(WebCore::Event&) const (WebCore.framework/Versions/A/WebCore+0xc34297)
    #29 0x11b1da113 in WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&, WebCore::WindowEventContext&) (WebCore.framework/Versions/A/WebCore+0xc36113)

[Hunseop Jeong]

Comment on attachment 253517 [details]
Patch

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

> Source/WebCore/svg/SVGDocumentExtensions.cpp:120
>      Vector<RefPtr<SVGSVGElement>> timeContainers;
>      timeContainers.appendRange(m_timeContainers.begin(), m_timeContainers.end());
>  
> -    auto end = timeContainers.end();
> -    for (auto it = timeContainers.begin(); it != end; ++it) {
> -        SVGSVGElement* outerSVG = (*it).get();
> -        if (!outerSVG->isOutermostSVGSVGElement())
> +    for (auto& container : m_timeContainers) {
> +        if (!container->isOutermostSVGSVGElement())

My mistake,,, I used the 'm_timeContainers' instead of the 'timeContainers'.
I will change it and then make the new patch.
Thanks for your check.

[Hunseop Jeong]

Created attachment 253579 [details]
Patch

[Hunseop Jeong]

I checked debug build and tested the fuilure test in https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/4483/steps/layout-test/logs/stdio. All passed.

[WebKit Commit Bot]

Comment on attachment 253579 [details]
Patch

Clearing flags on attachment: 253579

Committed r184844: <http://trac.webkit.org/changeset/184844>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.