Using image map inside a shadow tree results hits a release assert in DocumentOrderedMap::add

[Ryosuke Niwa]

Using image map results in a release assertion inside DocumentOrderedMap::add while inserting an image element with usemap attribute.

e.g.

ASSERTION FAILED: &element.treeScope() == &treeScope
dom/DocumentOrderedMap.cpp(52) : void WebCore::DocumentOrderedMap::add(const WTF::AtomicStringImpl &, WebCore::Element &, const WebCore::TreeScope &)
1   0x514f22079 WTFCrash
2   0x514f22099 WTFCrashWithSecurityImplication
3   0x50739c920 WebCore::DocumentOrderedMap::add(WTF::AtomicStringImpl const&, WebCore::Element&, WebCore::TreeScope const&)
4   0x507547135 WebCore::TreeScope::addImageElementByUsemap(WTF::AtomicStringImpl const&, WebCore::HTMLImageElement&)
5   0x507745d83 WebCore::HTMLImageElement::insertedIntoAncestor(WebCore::Node::InsertionType, WebCore::ContainerNode&)
6   0x507322034 WebCore::notifyNodeInsertedIntoDocument(WebCore::ContainerNode&, WebCore::Node&, WebCore::TreeScopeChange, WTF::Vector<WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >, 11ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&)
7   0x507321ec2 WebCore::notifyChildNodeInserted(WebCore::ContainerNode&, WebCore::Node&)
8   0x50731f825 void WebCore::executeNodeInsertionWithScriptAssertion<WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&)::$_4>(WebCore::ContainerNode&, WebCore::Node&, WebCore::ContainerNode::ChildChangeSource, WebCore::ReplacedAllChildren, WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&)::$_4)
9   0x50731cd1b WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&)
10  0x50731f784 WebCore::ContainerNode::appendChild(WebCore::Node&)

[Ryosuke Niwa]

Created attachment 339393 [details]
Fixes the crash

[Ryosuke Niwa]

Created attachment 339394 [details]
Fixed the ordering

[EWS Watchlist]

Attachment 339394 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLImageElement.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

<rdar://problem/39503288>

[Antti Koivisto]

Comment on attachment 339394 [details]
Fixed the ordering

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

> Source/WebCore/dom/Document.cpp:-749
> -void Document::addImageElementByUsemap(const AtomicStringImpl& name, HTMLImageElement& element)
> -{
> -    return m_imagesByUsemap.add(name, element, *this);
> -}

Is there anything else in Document that should be in TreeScope?

> Source/WebCore/dom/Document.h:-385
> -    void addImageElementByUsemap(const AtomicStringImpl&, HTMLImageElement&);
> -    void removeImageElementByUsemap(const AtomicStringImpl&, HTMLImageElement&);
> -    HTMLImageElement* imageElementByUsemap(const AtomicStringImpl&) const;
> -

You forgot to remove the Document::m_imagesByUsemap member.

> Source/WebCore/dom/TreeScope.cpp:59
>  struct SameSizeAsTreeScope {
> -    void* pointers[8];
> +    void* pointers[9];
>  };
>  
>  COMPILE_ASSERT(sizeof(TreeScope) == sizeof(SameSizeAsTreeScope), treescope_should_stay_small);

Is this assert is even valuable? TreeScope is not a very high volume type in itself and vast majority of per-scope memory use is elsewhere.

> Source/WebCore/dom/TreeScope.cpp:86
> +    m_elementsByName = nullptr;

Ninja bugfix!

[Antti Koivisto]

r=me (cq- for not removing Document::m_imagesByUsemap)

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #5)
> Comment on attachment 339394 [details]
> Fixed the ordering
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339394&action=review
> 
> > Source/WebCore/dom/Document.cpp:-749
> > -void Document::addImageElementByUsemap(const AtomicStringImpl& name, HTMLImageElement& element)
> > -{
> > -    return m_imagesByUsemap.add(name, element, *this);
> > -}
> 
> Is there anything else in Document that should be in TreeScope?

We no longer have any DocumentOrderedMap there. We should probably rename it to TreeScopeOrderedMap once this patch is landed so that nobody will try to use it on Document in the future.

> > Source/WebCore/dom/Document.h:-385
> > -    void addImageElementByUsemap(const AtomicStringImpl&, HTMLImageElement&);
> > -    void removeImageElementByUsemap(const AtomicStringImpl&, HTMLImageElement&);
> > -    HTMLImageElement* imageElementByUsemap(const AtomicStringImpl&) const;
> > -
> 
> You forgot to remove the Document::m_imagesByUsemap member.

Oops, will fix.

> > Source/WebCore/dom/TreeScope.cpp:59
> >  struct SameSizeAsTreeScope {
> > -    void* pointers[8];
> > +    void* pointers[9];
> >  };
> >  
> >  COMPILE_ASSERT(sizeof(TreeScope) == sizeof(SameSizeAsTreeScope), treescope_should_stay_small);
> 
> Is this assert is even valuable? TreeScope is not a very high volume type in
> itself and vast majority of per-scope memory use is elsewhere.

It's created for every shadow root; e.g. every input element. I've seen quite a bit of time allocating & initializing these instance variables during Speedometer 2. We may need to re-think about the structure of these shadow-root related data structures, and make them more efficient.

[Ryosuke Niwa]

Created attachment 339456 [details]
Patch for landing

[EWS Watchlist]

Attachment 339456 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLImageElement.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 339456 [details]
Patch for landing

Clearing flags on attachment: 339456

Committed r231329: <https://trac.webkit.org/changeset/231329>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Reverted the unrelated change in https://trac.webkit.org/changeset/231334/webkit.