Bug 62116 – REGRESSION(r87980): Crash in RenderTextControl::setInnerText() / ieForbidsInsertHTML

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

TextFieldInputElement::innerTextElement() return 0 though it specifies the ID value of a shadow element which was appended to the <input> in the <input> factory method.

We can't assume attach() is called after insertedIntoDocument().

[Kent Tamura]

http://code.google.com/p/chromium/issues/detail?id=84872

[Kent Tamura]

Created attachment 96072 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 96072 [details]
Patch

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

I think this approach is wrong. We should not be special-casing id handling inside shadow DOM. Or maybe I just don't understand why we would want to do that?

> Source/WebCore/dom/Element.cpp:961
> +    if (hasID() && m_attributeMap && treeScope() == document()) {

Why do we have to have a special way of handling shadow DOM nodes here? Other than working around the bug you're fixing? I don't think this is right.

[Dimitri Glazkov (Google)]

Comment on attachment 96072 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +         - insertedIntoDocument() for HTMLInputElement and shadow nodes were

This seems like the problem we need to be fixing.

[Kent Tamura]

Comment on attachment 96072 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +         - insertedIntoDocument() for HTMLInputElement and shadow nodes were
> 
> This seems like the problem we need to be fixing.

Yeah, probably this behavior is not intended.  I'll try to fix it today.

>> Source/WebCore/dom/Element.cpp:961
>> +    if (hasID() && m_attributeMap && treeScope() == document()) {
> 
> Why do we have to have a special way of handling shadow DOM nodes here? Other than working around the bug you're fixing? I don't think this is right.

I think this is a right change though it might be a good way to fix this bug. We had a similar problem in https://bugs.webkit.org/show_bug.cgi?id=61986#c25 .

ID registration/unregistration is handled in insertedIntoDocument()/removedFromDocument() because IDs are managed by a document.  Now ShadowRoot also manges subordinate IDs.  So ID registration/unregistration for ShadowRoot should be handled when an element is inserted into or removed from a ShadowRoot.  I think it's very straightforward behavior.

[Kent Tamura]

(In reply to comment #5)
> it might be a good way to fix this bug.

it might not be a ...

[Kent Tamura]

(In reply to comment #5)
> (From update of attachment 96072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96072&action=review
> 
> >> Source/WebCore/ChangeLog:11
> >> +         - insertedIntoDocument() for HTMLInputElement and shadow nodes were
> > 
> > This seems like the problem we need to be fixing.
> 
> Yeah, probably this behavior is not intended.  I'll try to fix it today.

It seems to be hard.  My simple change to fix the attach-before-insertedIntoDocument issue made another 7 layout test regressions.

[Kent Tamura]

(In reply to comment #5)
> >> Source/WebCore/dom/Element.cpp:961
> >> +    if (hasID() && m_attributeMap && treeScope() == document()) {
> > 
> > Why do we have to have a special way of handling shadow DOM nodes here? Other than working around the bug you're fixing? I don't think this is right.
> 
> I think this is a right change though it might be a good way to fix this bug. We had a similar problem in https://bugs.webkit.org/show_bug.cgi?id=61986#c25 .
> 
> ID registration/unregistration is handled in insertedIntoDocument()/removedFromDocument() because IDs are managed by a document.  Now ShadowRoot also manges subordinate IDs.  So ID registration/unregistration for ShadowRoot should be handled when an element is inserted into or removed from a ShadowRoot.  I think it's very straightforward behavior.

Roland, Morita-san,
do you have any comments on ID in ShadowRoot?

[Kent Tamura]

*** Bug 62109 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

*** Bug 62030 has been marked as a duplicate of this bug. ***

[Kent Tamura]

It's not good to keep the crash.  So I have decided to revert a part of r87980.
Let's discuss the ID issue in a separate bug.

[Ryosuke Niwa]

Renaming the bug so that this bug shows up when people search for ieForbidsInsertHTML.

[Kent Tamura]

Landed the revert: http://trac.webkit.org/changeset/88216

[Roland Steiner]

(In reply to comment #8)
> (In reply to comment #5)
> > ID registration/unregistration is handled in insertedIntoDocument()/removedFromDocument() because IDs are managed by a document.  Now ShadowRoot also manges subordinate IDs.  So ID registration/unregistration for ShadowRoot should be handled when an element is inserted into or removed from a ShadowRoot.  I think it's very straightforward behavior.

I also think the above is correct.

The cleanest solution IMHO would be that a node ALWAYS registers its ID with its TreeScope when it's inserted into the tree. To this end, we would need another TreeScope sub-class to contain the nodes that are associated with the Document, but !inDocument(). (This specific sub-class could even just do a no-op on ID registration).

However, this is a larger refactoring, probably outside the immediate scope of this bug.