Bug 72441

Created attachment 115524 [details] suppress light children.

Comment on attachment 115524 [details] suppress light children. Yay reftests—this is awesome.

Ping reviewers. Morrita-san, could you take a look?

Comment on attachment 115524 [details] suppress light children. View in context: https://bugs.webkit.org/attachment.cgi?id=115524&action=review > Source/WebCore/dom/Element.cpp:1271 > + } I prefer just call reattach() here because some Element subclasses have child attachment process and we need to preserve their customized behavior. Maybe we need a lazy version of reattach().

Created attachment 117991 [details] use lazy reattach

Thank you for the review. (In reply to comment #4) > (From update of attachment 115524 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115524&action=review > > > Source/WebCore/dom/Element.cpp:1271 > > + } > > I prefer just call reattach() here because some Element subclasses have child attachment process and we need to preserve their customized behavior. > Maybe we need a lazy version of reattach(). Done. I tried to name such a function 'lazyReattach', but 'lazyReattach' might be confusing name since reattach itself is not done lazily. Only attach() is done lazily. I named it 'Node::detachAndLazyAttach()', which is non-confusing, I guess.

One more comment. Maybe we also have to take care of the case when Node::removeShadowRoot() is called directly from JavaScript in a similar way. Let me address this case in another bug.

(In reply to comment #7) > One more comment. > Maybe we also have to take care of the case when Node::removeShadowRoot() is called directly from JavaScript in a similar way. Let me address this case in another bug. Well, actually I'm not sure if we are going to provide a way to remove existing shadow tree.

Comment on attachment 117991 [details] use lazy reattach Attachment 117991 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10695694 New failing tests: fast/forms/validation-message-on-listbox.html fast/forms/validation-message-on-menulist.html fast/forms/validation-message-on-radio.html fast/forms/validation-message-on-checkbox.html fast/forms/interactive-validation-crash-by-style-override.html fast/forms/interactive-validation-select-crash.html

It seems that a crash occurs in WebCore::ValidationMessage::buildBubbleTree. Let me investigate. (In reply to comment #9) > (From update of attachment 117991 [details]) > Attachment 117991 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10695694 > > New failing tests: > fast/forms/validation-message-on-listbox.html > fast/forms/validation-message-on-menulist.html > fast/forms/validation-message-on-radio.html > fast/forms/validation-message-on-checkbox.html > fast/forms/interactive-validation-crash-by-style-override.html > fast/forms/interactive-validation-select-crash.html

This happens as follows: 1. ValidationMessage::buildBubbleTree calls 'host->ensureShadowRoot()->appendChild(m_bubble.get(), ec)' host (shadow host) is a HTMLInputElement. void ValidationMessage::buildBubbleTree(Timer<ValidationMessage>*) { HTMLElement* host = toHTMLElement(m_element); Document* doc = host->document(); m_bubble = HTMLDivElement::create(doc); m_bubble->setShadowPseudoId("-webkit-validation-bubble"); // Need to force position:absolute because RenderMenuList doesn't assume it // contains non-absolute or non-fixed renderers as children. m_bubble->ensureInlineStyleDecl()->setProperty(CSSPropertyPosition, CSSValueAbsolute); ExceptionCode ec = 0; host->ensureShadowRoot()->appendChild(m_bubble.get(), ec); <--- HERE .... .... 2. Element(shadow host)::detach() is called by this patch while in ensureShadowRoot(). 3. HTMLInputElement::detach() is called. Then, HTMLFormControlElement::detach() is called, which causes m_validationMessage's deletion. void HTMLFormControlElement::detach() { m_validationMessage = nullptr; HTMLElement::detach(); } 4. m_validationMessage is deleted, then m_bubble is also deleted even while 1) is in progress. The relates stack trace is here: #0 0x0000000000a5f81e in WebCore::TreeShared<WebCore::ContainerNode>::deref (this=0x7fffeb27f130) at ../../third_party/WebKit/Source/WebCore/platform/TreeShared.h:75 #1 0x0000000000d0ad8e in WTF::derefIfNotNull<WebCore::HTMLElement> (ptr=0x7fffeb27f120) at ../../third_party/WebKit/Source/JavaScriptCore/wtf/PassRefPtr.h:52 #2 0x0000000000dc118e in WTF::RefPtr<WebCore::HTMLElement>::operator= (this=0x7fffeb273d58, optr=0x0) at ../../third_party/WebKit/Source/JavaScriptCore/wtf/RefPtr.h:135 #3 0x0000000000dc0ddc in WebCore::ValidationMessage::deleteBubbleTree (this=0x7fffeb273d40) at ../../third_party/WebKit/Source/WebCore/html/ValidationMessage.cpp:191 #4 0x0000000000dbf649 in WebCore::ValidationMessage::~ValidationMessage (this=0x7fffeb273d40, __in_chrg=<optimized out>) at ../../third_party/WebKit/Source/WebCore/html/ValidationMessage.cpp:61 #5 0x0000000000db796e in WTF::deleteOwnedPtr<WebCore::ValidationMessage> (ptr=0x7fffeb273d40) at ../../third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtrCommon.h:53 #6 0x0000000000db79ac in WTF::OwnPtr<WebCore::ValidationMessage>::clear (this=0x7fffeb33cc98) at ../../third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:99 #7 0x0000000000db6798 in WTF::OwnPtr<WebCore::ValidationMessage>::operator= (this=0x7fffeb33cc98) at ../../third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:72 #8 0x0000000000db3b9f in WebCore::HTMLFormControlElement::detach (this=0x7fffeb33cc00) at ../../third_party/WebKit/Source/WebCore/html/HTMLFormControlElement.cpp:80 #9 0x0000000000d165d4 in WebCore::HTMLInputElement::detach (this=0x7fffeb33cc00) at ../../third_party/WebKit/Source/WebCore/html/HTMLInputElement.cpp:901 #10 0x00000000018dcc87 in WebCore::Node::detachAndLazyAttach (this=0x7fffeb33cc00) at ../../third_party/WebKit/Source/WebCore/dom/Node.h:826 #11 0x00000000018dcc50 in WebCore::Node::detachAndLazyAttachIfAttached (this=0x7fffeb33cc00) at ../../third_party/WebKit/Source/WebCore/dom/Node.h:820 #12 0x00000000018d9bd7 in WebCore::Element::setShadowRoot (this=0x7fffeb33cc00, prpShadowRoot=..., ec=@0x7fffffffc20c) at ../../third_party/WebKit/Source/WebCore/dom/Element.cpp:1237 #13 0x00000000018d9c5a in WebCore::Element::ensureShadowRoot (this=0x7fffeb33cc00) at ../../third_party/WebKit/Source/WebCore/dom/Element.cpp:1246 #14 0x0000000000dc0157 in WebCore::ValidationMessage::buildBubbleTree (this=0x7fffeb273d40) at ../../third_party/WebKit/Source/WebCore/html/ValidationMessage.cpp:142 #15 0x0000000000dc1302 in WebCore::Timer<WebCore::ValidationMessage>::fired (this=0x7fffebc25af0) at ../../third_party/WebKit/Source/WebCore/platform/Timer.h:100 #16 0x0000000000f499ac in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x7ffff7ef93f0) at ../../third_party/WebKit/Source/WebCore/platform/ThreadTimers.cpp:115 #17 0x0000000000f498e3 in WebCore::ThreadTimers::sharedTimerFired () at ../../third_party/WebKit/Source/WebCore/platform/ThreadTimers.cpp:93 #18 0x0000000000cb1a56 in webkit_glue::WebKitPlatformSupportImpl::DoTimeout (this=0x7ffff7e5a400) at ../../webkit/glue/webkitplatformsupport_impl.h:130 (In reply to comment #10) > It seems that a crash occurs in WebCore::ValidationMessage::buildBubbleTree. Let me investigate. > > (In reply to comment #9) > > (From update of attachment 117991 [details] [details]) > > Attachment 117991 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/10695694 > > > > New failing tests: > > fast/forms/validation-message-on-listbox.html > > fast/forms/validation-message-on-menulist.html > > fast/forms/validation-message-on-radio.html > > fast/forms/validation-message-on-checkbox.html > > fast/forms/interactive-validation-crash-by-style-override.html > > fast/forms/interactive-validation-select-crash.html

So which should we stand for? Fix callee side: A. We must not call shadowHost's detach() while ensureShadowRoot() is in progress. Fix caller side: B. Change HTMLFormControlElement::detach so that it doesn't delete m_validation. I think A is a preferred to B since It's tough to force caller to obey difficult-to-express rule of B.

(In reply to comment #12) > So which should we stand for? > > Fix callee side: > A. We must not call shadowHost's detach() while ensureShadowRoot() is in progress. > > Fix caller side: > B. Change HTMLFormControlElement::detach so that it doesn't delete m_validation. > > I think A is a preferred to B since It's tough to force caller to obey difficult-to-express rule of B. Adding tkent@, since he wrote most of this code.

(In reply to comment #9) > New failing tests: > fast/forms/validation-message-on-listbox.html > fast/forms/validation-message-on-menulist.html > fast/forms/validation-message-on-radio.html > fast/forms/validation-message-on-checkbox.html > fast/forms/interactive-validation-crash-by-style-override.html > fast/forms/interactive-validation-select-crash.html These test assume both of the host's renderer and the shadow's renderer are shown. Is it a wrong assumption?

(In reply to comment #14) > (In reply to comment #9) > > New failing tests: > > fast/forms/validation-message-on-listbox.html > > fast/forms/validation-message-on-menulist.html > > fast/forms/validation-message-on-radio.html > > fast/forms/validation-message-on-checkbox.html > > fast/forms/interactive-validation-crash-by-style-override.html > > fast/forms/interactive-validation-select-crash.html > > These test assume both of the host's renderer and the shadow's renderer are shown. Is it a wrong assumption? That assumption is correct. I believe the change being discussed here is to reattach the host’s children when shadow DOM is applied.

(In reply to comment #15) > That assumption is correct. > > I believe the change being discussed here is to reattach the host’s children when shadow DOM is applied. ok. I confused "rendering of light children" and rendering of the host. I think B is also ok. We don't need to delete shadow tree on detach() in general.

Kent-san, as I discussed with Morita-san, we are thinking that B is our option since we have some difficulties to achieve A nicely. It'd be nice that shadowRoot's clients do not manipulate shadow tree in their detach(). Is this possible? (In reply to comment #16) > (In reply to comment #15) > > That assumption is correct. > > > > I believe the change being discussed here is to reattach the host’s children when shadow DOM is applied. > > ok. I confused "rendering of light children" and rendering of the host. > > I think B is also ok. We don't need to delete shadow tree on detach() in general.

I guess just removing "m_validationMessage = nullptr;" in HTMLFormControlElement::detach() solves the problem.

(In reply to comment #18) > I guess just removing "m_validationMessage = nullptr;" in HTMLFormControlElement::detach() solves the problem. Yeah, but that causes another flaky crash bug and/or the different rendering result if that is applied with this patch. Let me investigate further.

Created attachment 118367 [details] revert it to the original patch

I've reverted the patch to its original version. We have not found a good way to make a shadow host be reattached so that it does not cause any rendering issue or performance regression. Untile we have a better way to solve the issue, I think the original patch might be safe way to achieve it.

Comment on attachment 118367 [details] revert it to the original patch Seems sane.

Comment on attachment 118367 [details] revert it to the original patch Clearing flags on attachment: 118367 Committed r102423: <http://trac.webkit.org/changeset/102423>

All reviewed patches have been landed. Closing bug.