Attachment 339393 Details for Bug 185238 – Fixes the crash

[patch] Fixes the crash

fix185238.patch (text/plain), 15.64 KB, created by Ryosuke Niwa on 2018-05-03 00:11:27 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-05-03 00:11:27 PDT

Size: 15.64 KB

patch

obsolete

>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 231031)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-05-02  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Using image map inside a shadow tree results hits a release assert in DocumentOrderedMap::add
>+        https://bugs.webkit.org/show_bug.cgi?id=185238
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/images/imagemap-in-nested-shadow-tree-expected.txt: Added.
>+        * fast/images/imagemap-in-nested-shadow-tree.html: Added.
>+        * fast/images/imagemap-in-shadow-tree-expected.txt: Added.
>+        * fast/images/imagemap-in-shadow-tree.html: Added.
>+
> 2018-04-25  Carlos Alberto Lopez Perez  <clopez@igalia.com>
> 
>         [WPE][Debug] Test gardening of EME related tests.
>Index: LayoutTests/fast/images/imagemap-in-shadow-tree-expected.txt
>===================================================================
>--- LayoutTests/fast/images/imagemap-in-shadow-tree-expected.txt	(nonexistent)
>+++ LayoutTests/fast/images/imagemap-in-shadow-tree-expected.txt	(working copy)
>@@ -0,0 +1,5 @@
>+This tests activing an image map area inside a shadow tree. WebKit should not hit any assertions.
>+To manually test, click on green box on the left upper quadrant.
>+
>+PASS
>+
>Index: LayoutTests/fast/images/imagemap-in-shadow-tree.html
>===================================================================
>--- LayoutTests/fast/images/imagemap-in-shadow-tree.html	(nonexistent)
>+++ LayoutTests/fast/images/imagemap-in-shadow-tree.html	(working copy)
>@@ -0,0 +1,71 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script src="../../resources/ui-helper.js"></script>
>+<p>This tests activing an image map area inside a shadow tree. WebKit should not hit any assertions.<br>
>+To manually test, click on green box on the left upper quadrant.</p>
>+<div id="result"></div>
>+<script>
>+
>+const host = document.createElement('div');
>+document.body.appendChild(host);
>+
>+const shadowRoot = host.attachShadow({mode: 'closed'});
>+shadowRoot.innerHTML = `
>+<img src="resources/green-400x400.png" width="400" height="400" usemap="#imagemap" onload="startTest()">
>+<map name="imagemap">
>+    <area id="area" shape="rect" coords="0,0,200,200" href="#" onclick="didClick(event)" tabindex="0">
>+</map>`;
>+const rect = host.getBoundingClientRect();
>+
>+function startTest()
>+{
>+    if (!window.testRunner)
>+        return;
>+    UIHelper.activateAt(rect.x + 100, rect.y + 100).then(check);
>+}
>+
>+let didClickWasCalled = false;
>+function didClick(event)
>+{
>+    event.preventDefault();
>+    didClickWasCalled = true;
>+}
>+
>+function check()
>+{
>+    let result = '';
>+    if (!didClickWasCalled)
>+        result = 'FAIL - JavaScript was not executed';
>+    else if (shadowRoot.activeElement != shadowRoot.getElementById('area'))
>+        result = 'FAIL - The element was not focused';
>+    else
>+        result = 'PASS';
>+    document.getElementById('result').textContent = result;
>+    if (window.testRunner)
>+        testRunner.notifyDone();
>+}
>+
>+if (window.testRunner) {
>+    setTimeout(() => testRunner.notifyDone(), 3000);
>+    testRunner.waitUntilDone();
>+    testRunner.dumpAsText();
>+} else {
>+    host.onclick = (event) => {
>+        event.preventDefault();
>+
>+        const isInUpperLeftQuadrant = rect.x < event.pageX && event.pageX < rect.x + 200
>+            && rect.y < event.pageY && event.pageY < rect.y + 200;
>+        if (!isInUpperLeftQuadrant) {
>+            alert('Please click on the upper left quadrant of the green box.');
>+            return;
>+        }
>+
>+        check();
>+    };
>+}
>+
>+</script>
>+</body>
>+</head>
>+</html>
>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 231294)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,5 +1,44 @@
> 2018-05-02  Ryosuke Niwa  <rniwa@webkit.org>
> 
>+        Using image map inside a shadow tree results hits a release assert in DocumentOrderedMap::add
>+        https://bugs.webkit.org/show_bug.cgi?id=185238
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The bug was caused by DocumentOrderedMap for the image elements with usemap being stored in Document
>+        even if those image elements were in a shadow tree. Fixed the bug by moving the map to TreeScope.
>+
>+        Test: fast/images/imagemap-in-nested-shadow-tree.html
>+              fast/images/imagemap-in-shadow-tree.html
>+
>+        * dom/Document.cpp:
>+        (WebCore::Document::addImageElementByUsemap): Moved to TreeScope.
>+        (WebCore::Document::removeImageElementByUsemap): Ditto.
>+        (WebCore::Document::imageElementByUsemap const): Ditto.
>+        * dom/Document.h:
>+        * dom/TreeScope.cpp:
>+        (WebCore::TreeScope::destroyTreeScopeData): Clear m_imagesByUsemap as well as m_elementsByName.
>+        (WebCore::TreeScope::getImageMap const): Removed the code to parse usemap. RenderImage::imageMap()
>+        which used to call this function with the raw value of the usemap content attribute now calls it
>+        via HTMLImageElement::associatedMapElement(), which uses the parsed usemap.
>+        (WebCore::TreeScope::addImageElementByUsemap): Moved from Document.
>+        (WebCore::TreeScope::removeImageElementByUsemap): Ditto.
>+        (WebCore::TreeScope::imageElementByUsemap const): Ditto.
>+        * dom/TreeScope.h:
>+        * html/HTMLImageElement.cpp:
>+        (WebCore::HTMLImageElement::parseAttribute):
>+        (WebCore::HTMLImageElement::insertedIntoAncestor): This image element can be associated with a map element
>+        if it's connected to a document.
>+        (WebCore::HTMLImageElement::removedFromAncestor):
>+        (WebCore::HTMLImageElement::associatedMapElement const):
>+        * html/HTMLImageElement.h:
>+        * html/HTMLMapElement.cpp:
>+        (WebCore::HTMLMapElement::imageElement):
>+        * rendering/RenderImage.cpp:
>+        (WebCore::RenderImage::imageMap const):
>+
>+2018-05-02  Ryosuke Niwa  <rniwa@webkit.org>
>+
>         Remove superfluous check for a null attribute value check in Element::removeAttributeInternal
>         https://bugs.webkit.org/show_bug.cgi?id=185227
> 
>Index: Source/WebCore/dom/Document.cpp
>===================================================================
>--- Source/WebCore/dom/Document.cpp	(revision 231031)
>+++ Source/WebCore/dom/Document.cpp	(working copy)
>@@ -743,21 +743,6 @@
>     m_elementsByAccessKey.clear();
> }
> 
>-void Document::addImageElementByUsemap(const AtomicStringImpl& name, HTMLImageElement& element)
>-{
>-    return m_imagesByUsemap.add(name, element, *this);
>-}
>-
>-void Document::removeImageElementByUsemap(const AtomicStringImpl& name, HTMLImageElement& element)
>-{
>-    return m_imagesByUsemap.remove(name, element);
>-}
>-
>-HTMLImageElement* Document::imageElementByUsemap(const AtomicStringImpl& name) const
>-{
>-    return m_imagesByUsemap.getElementByUsemap(name, *this);
>-}
>-
> ExceptionOr<SelectorQuery&> Document::selectorQueryForString(const String& selectorString)
> {
>     if (selectorString.isEmpty())
>Index: Source/WebCore/dom/Document.h
>===================================================================
>--- Source/WebCore/dom/Document.h	(revision 231031)
>+++ Source/WebCore/dom/Document.h	(working copy)
>@@ -379,10 +379,6 @@
>     Element* getElementByAccessKey(const String& key);
>     void invalidateAccessKeyMap();
> 
>-    void addImageElementByUsemap(const AtomicStringImpl&, HTMLImageElement&);
>-    void removeImageElementByUsemap(const AtomicStringImpl&, HTMLImageElement&);
>-    HTMLImageElement* imageElementByUsemap(const AtomicStringImpl&) const;
>-
>     ExceptionOr<SelectorQuery&> selectorQueryForString(const String&);
>     void clearSelectorQueryCache();
> 
>Index: Source/WebCore/dom/TreeScope.cpp
>===================================================================
>--- Source/WebCore/dom/TreeScope.cpp	(revision 231031)
>+++ Source/WebCore/dom/TreeScope.cpp	(working copy)
>@@ -35,6 +35,7 @@
> #include "FrameView.h"
> #include "HTMLAnchorElement.h"
> #include "HTMLFrameOwnerElement.h"
>+#include "HTMLImageElement.h"
> #include "HTMLLabelElement.h"
> #include "HTMLMapElement.h"
> #include "HitTestResult.h"
>@@ -52,7 +53,7 @@
> namespace WebCore {
> 
> struct SameSizeAsTreeScope {
>-    void* pointers[8];
>+    void* pointers[9];
> };
> 
> COMPILE_ASSERT(sizeof(TreeScope) == sizeof(SameSizeAsTreeScope), treescope_should_stay_small);
>@@ -82,7 +83,9 @@
> void TreeScope::destroyTreeScopeData()
> {
>     m_elementsById = nullptr;
>+    m_elementsByName = nullptr;
>     m_imageMapsByName = nullptr;
>+    m_imagesByUsemap = nullptr;
>     m_labelsByForAttribute = nullptr;
> }
> 
>@@ -250,17 +253,32 @@
>     m_imageMapsByName->remove(*name, imageMap);
> }
> 
>-HTMLMapElement* TreeScope::getImageMap(const String& url) const
>+HTMLMapElement* TreeScope::getImageMap(const AtomicString& name) const
> {
>-    if (!m_imageMapsByName)
>+    if (!m_imageMapsByName || !name.impl())
>         return nullptr;
>-    auto hashPosition = url.find('#');
>-    if (hashPosition == notFound)
>+    return m_imageMapsByName->getElementByMapName(*name.impl(), *this);
>+}
>+
>+void TreeScope::addImageElementByUsemap(const AtomicStringImpl& name, HTMLImageElement& element)
>+{
>+    if (!m_imagesByUsemap)
>+        m_imagesByUsemap = std::make_unique<DocumentOrderedMap>();
>+    return m_imagesByUsemap->add(name, element, *this);
>+}
>+
>+void TreeScope::removeImageElementByUsemap(const AtomicStringImpl& name, HTMLImageElement& element)
>+{
>+    if (!m_imagesByUsemap)
>+        return;
>+    m_imagesByUsemap->remove(name, element);
>+}
>+
>+HTMLImageElement* TreeScope::imageElementByUsemap(const AtomicStringImpl& name) const
>+{
>+    if (!m_imagesByUsemap)
>         return nullptr;
>-    String name = url.substring(hashPosition + 1);
>-    if (name.isEmpty())
>-        return nullptr;
>-    return m_imageMapsByName->getElementByMapName(*AtomicString(name).impl(), *this);
>+    return m_imagesByUsemap->getElementByUsemap(name, *this);
> }
> 
> void TreeScope::addLabel(const AtomicStringImpl& forAttributeValue, HTMLLabelElement& element)
>Index: Source/WebCore/dom/TreeScope.h
>===================================================================
>--- Source/WebCore/dom/TreeScope.h	(revision 231031)
>+++ Source/WebCore/dom/TreeScope.h	(working copy)
>@@ -37,6 +37,7 @@
> class ContainerNode;
> class Document;
> class Element;
>+class HTMLImageElement;
> class HTMLLabelElement;
> class HTMLMapElement;
> class LayoutPoint;
>@@ -80,8 +81,12 @@
> 
>     void addImageMap(HTMLMapElement&);
>     void removeImageMap(HTMLMapElement&);
>-    HTMLMapElement* getImageMap(const String& url) const;
>+    HTMLMapElement* getImageMap(const AtomicString&) const;
> 
>+    void addImageElementByUsemap(const AtomicStringImpl&, HTMLImageElement&);
>+    void removeImageElementByUsemap(const AtomicStringImpl&, HTMLImageElement&);
>+    HTMLImageElement* imageElementByUsemap(const AtomicStringImpl&) const;
>+
>     // For accessibility.
>     bool shouldCacheLabelsByForAttribute() const { return !!m_labelsByForAttribute; }
>     void addLabel(const AtomicStringImpl& forAttributeValue, HTMLLabelElement&);
>@@ -124,6 +129,7 @@
>     std::unique_ptr<DocumentOrderedMap> m_elementsById;
>     std::unique_ptr<DocumentOrderedMap> m_elementsByName;
>     std::unique_ptr<DocumentOrderedMap> m_imageMapsByName;
>+    std::unique_ptr<DocumentOrderedMap> m_imagesByUsemap;
>     std::unique_ptr<DocumentOrderedMap> m_labelsByForAttribute;
> 
>     std::unique_ptr<IdTargetObserverRegistry> m_idTargetObserverRegistry;
>Index: Source/WebCore/html/HTMLImageElement.cpp
>===================================================================
>--- Source/WebCore/html/HTMLImageElement.cpp	(revision 231031)
>+++ Source/WebCore/html/HTMLImageElement.cpp	(working copy)
>@@ -32,6 +32,7 @@
> #include "HTMLFormElement.h"
> #include "HTMLParserIdioms.h"
> #include "HTMLPictureElement.h"
>+#include "HTMLMapElement.h"
> #include "HTMLSourceElement.h"
> #include "HTMLSrcsetParser.h"
> #include "Logging.h"
>@@ -211,12 +212,12 @@
>         selectImageSource();
>     else if (name == usemapAttr) {
>         if (isConnected() && !m_parsedUsemap.isNull())
>-            document().removeImageElementByUsemap(*m_parsedUsemap.impl(), *this);
>+            treeScope().removeImageElementByUsemap(*m_parsedUsemap.impl(), *this);
> 
>         m_parsedUsemap = parseHTMLHashNameReference(value);
> 
>         if (isConnected() && !m_parsedUsemap.isNull())
>-            document().addImageElementByUsemap(*m_parsedUsemap.impl(), *this);
>+            treeScope().addImageElementByUsemap(*m_parsedUsemap.impl(), *this);
>     } else if (name == compositeAttr) {
>         // FIXME: images don't support blend modes in their compositing attribute.
>         BlendMode blendOp = BlendModeNormal;
>@@ -320,7 +321,7 @@
>     Node::InsertedIntoAncestorResult insertNotificationRequest = HTMLElement::insertedIntoAncestor(insertionType, parentOfInsertedTree);
> 
>     if (insertionType.connectedToDocument && !m_parsedUsemap.isNull())
>-        document().addImageElementByUsemap(*m_parsedUsemap.impl(), *this);
>+        treeScope().addImageElementByUsemap(*m_parsedUsemap.impl(), *this);
> 
>     if (is<HTMLPictureElement>(parentNode())) {
>         setPictureElement(&downcast<HTMLPictureElement>(*parentNode()));
>@@ -341,7 +342,7 @@
>         m_form->removeImgElement(this);
> 
>     if (removalType.disconnectedFromDocument && !m_parsedUsemap.isNull())
>-        document().removeImageElementByUsemap(*m_parsedUsemap.impl(), *this);
>+        treeScope().removeImageElementByUsemap(*m_parsedUsemap.impl(), *this);
> 
>     if (is<HTMLPictureElement>(parentNode()))
>         setPictureElement(nullptr);
>@@ -484,6 +485,11 @@
>     return m_parsedUsemap.impl() == &name;
> }
> 
>+HTMLMapElement* HTMLImageElement::associatedMapElement() const
>+{
>+    return treeScope().getImageMap(m_parsedUsemap);
>+}
>+
> const AtomicString& HTMLImageElement::alt() const
> {
>     return attributeWithoutSynchronization(altAttr);
>Index: Source/WebCore/html/HTMLImageElement.h
>===================================================================
>--- Source/WebCore/html/HTMLImageElement.h	(revision 231031)
>+++ Source/WebCore/html/HTMLImageElement.h	(working copy)
>@@ -32,6 +32,7 @@
> namespace WebCore {
> 
> class HTMLFormElement;
>+class HTMLMapElement;
> 
> struct ImageCandidate;
> 
>@@ -63,6 +64,7 @@
>     void setLoadManually(bool loadManually) { m_imageLoader.setLoadManually(loadManually); }
> 
>     bool matchesUsemap(const AtomicStringImpl&) const;
>+    HTMLMapElement* associatedMapElement() const;
> 
>     WEBCORE_EXPORT const AtomicString& alt() const;
> 
>Index: Source/WebCore/html/HTMLMapElement.cpp
>===================================================================
>--- Source/WebCore/html/HTMLMapElement.cpp	(revision 231031)
>+++ Source/WebCore/html/HTMLMapElement.cpp	(working copy)
>@@ -80,7 +80,7 @@
> {
>     if (m_name.isEmpty())
>         return nullptr;
>-    return document().imageElementByUsemap(*m_name.impl());
>+    return treeScope().imageElementByUsemap(*m_name.impl());
> }
> 
> void HTMLMapElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
>Index: Source/WebCore/rendering/RenderImage.cpp
>===================================================================
>--- Source/WebCore/rendering/RenderImage.cpp	(revision 231031)
>+++ Source/WebCore/rendering/RenderImage.cpp	(working copy)
>@@ -656,8 +656,10 @@
> 
> HTMLMapElement* RenderImage::imageMap() const
> {
>-    HTMLImageElement* image = is<HTMLImageElement>(element()) ? downcast<HTMLImageElement>(element()) : nullptr;
>-    return image ? image->treeScope().getImageMap(image->attributeWithoutSynchronization(usemapAttr)) : nullptr;
>+    auto* imageElement = element();
>+    if (!imageElement || !is<HTMLImageElement>(imageElement))
>+        return nullptr;
>+    return downcast<HTMLImageElement>(imageElement)->associatedMapElement();
> }
> 
> bool RenderImage::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, HitTestAction hitTestAction)

Actions: View | Formatted Diff | Diff

Attachments on bug 185238: 339393 | 339394 | 339456