Bug 72441 - Dynamically created ShadowRoot doesn’t suppress the rendering of light children
Summary: Dynamically created ShadowRoot doesn’t suppress the rendering of light children
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 72352
  Show dependency treegraph
 
Reported: 2011-11-15 16:25 PST by Dominic Cooney
Modified: 2011-12-08 20:16 PST (History)
6 users (show)

See Also:


Attachments
suppress light children. (3.57 KB, patch)
2011-11-16 22:21 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
use lazy reattach (4.36 KB, patch)
2011-12-05 22:51 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
revert it to the original patch (3.56 KB, patch)
2011-12-08 05:25 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2011-11-15 16:25:42 PST
<h1>Hello!</h1>

var h = document.querySelector('h1');
var s = window.internals.ensureShadowRoot(h);
s.appendChild(document.createTextNode('Goodbye!'));

I would expect to see

Goodbye!

but instead I see

Hello!Goodbye!
Comment 1 Hayato Ito 2011-11-16 22:21:09 PST
Created attachment 115524 [details]
suppress light children.
Comment 2 Dominic Cooney 2011-11-17 00:54:38 PST
Comment on attachment 115524 [details]
suppress light children.

Yay reftests—this is awesome.
Comment 3 Hayato Ito 2011-11-20 18:26:36 PST
Ping reviewers. Morrita-san, could you take a look?
Comment 4 Hajime Morrita 2011-12-04 20:25:59 PST
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().
Comment 5 Hayato Ito 2011-12-05 22:51:29 PST
Created attachment 117991 [details]
use lazy reattach
Comment 6 Hayato Ito 2011-12-05 22:58:41 PST
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.
Comment 7 Hayato Ito 2011-12-05 23:21:19 PST
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.
Comment 8 Hajime Morrita 2011-12-06 01:05:36 PST
(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 9 WebKit Review Bot 2011-12-06 02:01:40 PST
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
Comment 10 Hayato Ito 2011-12-06 04:06:48 PST
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
Comment 11 Hayato Ito 2011-12-06 05:40:58 PST
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
Comment 12 Hayato Ito 2011-12-06 05:53:06 PST
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.
Comment 13 Dimitri Glazkov (Google) 2011-12-06 08:29:09 PST
(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.
Comment 14 Kent Tamura 2011-12-06 17:41:04 PST
(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?
Comment 15 Dominic Cooney 2011-12-06 18:16:16 PST
(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.
Comment 16 Kent Tamura 2011-12-06 21:43:15 PST
(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.
Comment 17 Hayato Ito 2011-12-07 00:28:13 PST
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.
Comment 18 Kent Tamura 2011-12-07 00:42:23 PST
I guess just removing "m_validationMessage = nullptr;" in HTMLFormControlElement::detach() solves the problem.
Comment 19 Hayato Ito 2011-12-07 02:01:12 PST
(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.
Comment 20 Hayato Ito 2011-12-08 05:25:08 PST
Created attachment 118367 [details]
revert it to the original patch
Comment 21 Hayato Ito 2011-12-08 05:29:53 PST
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 22 Ryosuke Niwa 2011-12-08 16:35:44 PST
Comment on attachment 118367 [details]
revert it to the original patch

Seems sane.
Comment 23 WebKit Review Bot 2011-12-08 20:16:42 PST
Comment on attachment 118367 [details]
revert it to the original patch

Clearing flags on attachment: 118367

Committed r102423: <http://trac.webkit.org/changeset/102423>
Comment 24 WebKit Review Bot 2011-12-08 20:16:48 PST
All reviewed patches have been landed.  Closing bug.