Bug 77514 – ShadowRoot's nodeType should be DOCUMENT_FRAGMENT_NODE.

• Component: DOM
• Status: RESOLVED FIXED

[Hayato Ito]

Current implementation is:
 shadowRoot.nodeType returns 'SHADOW_ROOT_NODE' and nodeName returns '#shadow-root'.

The latest spec says:
 The nodeType attribute of a ShadowRoot instance must return DOCUMENT_FRAGMENT_NODE. Accordingly, the nodeName attribute of a ShadowRoot instance must return "#document-fragment".

[Hayato Ito]

I filed the bug for spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15852

I think shadowRoot's nodeName should be '#shadow-root' unless there is a strong reason for ShadowRoot to have '#document-fragment' as nodeName.

[Hayato Ito]

I'ver renamed title of bug, focusing on only ShadowRoot.nodeType.

[Hayato Ito]

Created attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE

[Hajime Morrita]

Comment on attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE

Let's land this once cr-linux gets gtreen.

[Early Warning System Bot]

Comment on attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE

Attachment 125257 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11421127

[Hayato Ito]

It seems we need to update JSNodeCustom too. Let me fix it.

[WebKit Review Bot]

Comment on attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE

Attachment 125257 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11421143

New failing tests:
editing/deleting/delete-ligature-001.html
editing/pasteboard/copy-in-password-field.html
editing/deleting/delete-all-text-in-text-field-assertion.html

[Gustavo Noronha (kov)]

Comment on attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE

Attachment 125257 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11418185

[Gyuyoung Kim]

Comment on attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE

Attachment 125257 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11421140

[Hayato Ito]

Created attachment 125264 [details]
fix JSC.

[Hayato Ito]

Comment on attachment 125264 [details]
fix JSC.

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

> Source/WebCore/bindings/js/JSNodeCustom.cpp:-259
> -        case Node::SHADOW_ROOT_NODE:

I don't fully understand what we should do here.
I need help from IDL/binding experts.

> Source/WebCore/bindings/v8/custom/V8NodeCustom.cpp:169
>      default: break; // XPATH_NAMESPACE_NODE

The same here.

[Hayato Ito]

Created attachment 125276 [details]
Add ASSERT

[Hayato Ito]

I've decided to add ASSERT there after a discussion with haraken, morrita.
See the comment for the reason.

Let me watch the result of ews. If all bots get green, I'll land this patch.

(In reply to comment #11)
> (From update of attachment 125264 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125264&action=review
> 
> > Source/WebCore/bindings/js/JSNodeCustom.cpp:-259
> > -        case Node::SHADOW_ROOT_NODE:
> 
> I don't fully understand what we should do here.
> I need help from IDL/binding experts.
> 
> > Source/WebCore/bindings/v8/custom/V8NodeCustom.cpp:169
> >      default: break; // XPATH_NAMESPACE_NODE
> 
> The same here.

[Hayato Ito]

It seems I wrongly cleared the r+ flag on the previous patch. So let me set r? in the latest patch.

[Hayato Ito]

Created attachment 125282 [details]
Fix mac build hopefully

[WebKit Review Bot]

Comment on attachment 125282 [details]
Fix mac build hopefully

Attachment 125282 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11416275

New failing tests:
editing/deleting/delete-ligature-001.html
editing/pasteboard/copy-in-password-field.html
editing/deleting/delete-all-text-in-text-field-assertion.html

[Hayato Ito]

Created attachment 125304 [details]
rebase some layout tests

[Hayato Ito]

Comment on attachment 125304 [details]
rebase some layout tests

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

> LayoutTests/ChangeLog:12
> +        * fast/dom/shadow/nodetype-expected.txt:

I rebased the following 3 layout tests.

10        * editing/deleting/delete-ligature-001-expected.txt:
11        * editing/pasteboard/copy-in-password-field-expected.txt:
12        * fast/dom/shadow/nodetype-expected.txt:

I am not sure that rebasing is correct way to fix.
Its difficult to judge for me, but I'd like to avoid inserting, 

'node->nodeType() == DOCUMENT_FRAGMENT_NODE && node->isShadowRoot()'

to every places where 'node->nodeType() == DOCUMENT_FRAGMENT_NODE' appears in WebKit's codebase.
I expect its be time to rebase.
I am adding rniwa and ggaren as reviewers since I expect they know much better about this rebasing make sense or not.

[Hayato Ito]

[Wrong in the previous post]
> 'node->nodeType() == DOCUMENT_FRAGMENT_NODE && node->isShadowRoot()'

[Correct]
> 'node->nodeType() == DOCUMENT_FRAGMENT_NODE && !node->isShadowRoot()'

[Hayato Ito]

Created attachment 125307 [details]
cleanup. remove unnecessary (hopefully) isShadowRoot() check logic as long as regressions does not happen.

[Dimitri Glazkov (Google)]

Comment on attachment 125307 [details]
cleanup. remove unnecessary (hopefully) isShadowRoot() check logic as long as regressions does not happen.

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

Ok. But why is the mac EWS sad? Can you fix before landing?

> Source/WebCore/bindings/js/JSNodeCustom.cpp:259
> +            // Once we have such APIs, we should call toV8(static_cast<ShadowRoot*>..).

not sure why you're mentioning V8 in JSC bindings. Typo or mystery? :)

[Ryosuke Niwa]

Comment on attachment 125307 [details]
cleanup. remove unnecessary (hopefully) isShadowRoot() check logic as long as regressions does not happen.

You need to update http://trac.webkit.org/browser/trunk/Source/WebCore/editing/FrameSelection.cpp#L428

[Ryosuke Niwa]

In general, you have to examine every place that compares the node type to Node::DOCUMENT_FRAGMENT_NODE.

[Hayato Ito]

Created attachment 125570 [details]
regression has gone.

[Ryosuke Niwa]

Comment on attachment 125570 [details]
regression has gone.

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

> Source/WebCore/editing/FrameSelection.cpp:428
> +static inline bool isHighestAncestorDocumentFragment(Node* node) 

I would have named this function nodeIsDetachedFromDocument or negated the condition inside and called it nodeIsInDocument.

[Hayato Ito]

Created attachment 125575 [details]
ready for review. let me watch whether mac-build error is reproducible or not

[Hayato Ito]

Thank you for the reviews.

(In reply to comment #21)
> (From update of attachment 125307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125307&action=review
> 
> Ok. But why is the mac EWS sad? Can you fix before landing?
> 
> > Source/WebCore/bindings/js/JSNodeCustom.cpp:259
> > +            // Once we have such APIs, we should call toV8(static_cast<ShadowRoot*>..).
> 
> not sure why you're mentioning V8 in JSC bindings. Typo or mystery? :)

Done. It seems I forgot to update the comment after coping it from V8's.

(In reply to comment #22)
> (From update of attachment 125307 [details])
> You need to update http://trac.webkit.org/browser/trunk/Source/WebCore/editing/FrameSelection.cpp#L428

You are my hero. All regression of LayoutTest has gone. Thank you!

(In reply to comment #25)
> (From update of attachment 125570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125570&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:428
> > +static inline bool isHighestAncestorDocumentFragment(Node* node) 
> 
> I would have named this function nodeIsDetachedFromDocument or negated the condition inside and called it nodeIsInDocument.

Sounds good name. Done.

[Hayato Ito]

Ready for review. Could you take another look?

[Ryosuke Niwa]

Comment on attachment 125575 [details]
ready for review. let me watch whether mac-build error is reproducible or not

How about line 1000 in Range.cpp:
    if (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) {

line 1054:
            lastChild = (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) ? newNode->lastChild() : newNode;

Or collectTargetNodes(Node* node, NodeVector& nodes), or line 294:
    bool isFragment = newChild->nodeType() == DOCUMENT_FRAGMENT_NODE;
in ContainerNode.cpp?

There are so many other places where we compare nodeType to DOCUMENT_FRAGMENT_NODE. Why is it okay for us to start including shadow root in all of those cases? I'd like to see some explanation on why that's the case, if appropriate, for each occurrence of such comparison.

[Hayato Ito]

Let me explain my approach of this change. (I don't intend to insist my approach is correct one. Let discuss what is best one.)

1. Treat ShadowRoot and DocumentFragment in the same fashion as long as layout tests do not break.
2. Insert isShadowRoot() check where a layout test is broken unless it. I guess we don't have to add such isShadowRoot() check in most places, but I don't want to rebase any layout tests in this patch. That should be done in follow-up patches.

In general, I chose A over B as long as layout test do not break.
A. 'node->nodeType() == DOCUMENT_TYPE_NODE
B. 'node->nodeType() == DOCUMENT_TYPE_NODE && !node->isShadowRoot()'

My approach is totally assuming existing layout tests will catch a regression, hopefully.
If there is no difference in layout test's result whether we have A or B at some place, we should add a layout test for that to detect the different behavior. But adding layout tests is beyond this patch and should be done in other patches.

Yours is that I tried at first. I tried to replace all B with A at first. But I thought most of them are unnecessary and I got to feel I should rely on existing layout tests for now.

So either is okay for me. Changing all B with A is an easy task and safest, but I am afraid that will leave unnecessary cluttering of code.

Any advices are welcome.






(In reply to comment #29)
> (From update of attachment 125575 [details])
> How about line 1000 in Range.cpp:
>     if (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) {
> 
> line 1054:
>             lastChild = (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) ? newNode->lastChild() : newNode;
> 
> Or collectTargetNodes(Node* node, NodeVector& nodes), or line 294:
>     bool isFragment = newChild->nodeType() == DOCUMENT_FRAGMENT_NODE;
> in ContainerNode.cpp?
> 
> There are so many other places where we compare nodeType to DOCUMENT_FRAGMENT_NODE. Why is it okay for us to start including shadow root in all of those cases? I'd like to see some explanation on why that's the case, if appropriate, for each occurrence of such comparison.

[Hayato Ito]

<correction> A and B should be swapped in the my explanation.

(In reply to comment #30)
> Let me explain my approach of this change. (I don't intend to insist my approach is correct one. Let discuss what is best one.)
> 
> 1. Treat ShadowRoot and DocumentFragment in the same fashion as long as layout tests do not break.
> 2. Insert isShadowRoot() check where a layout test is broken unless it. I guess we don't have to add such isShadowRoot() check in most places, but I don't want to rebase any layout tests in this patch. That should be done in follow-up patches.
> 
> In general, I chose A over B as long as layout test do not break.
> A. 'node->nodeType() == DOCUMENT_TYPE_NODE
> B. 'node->nodeType() == DOCUMENT_TYPE_NODE && !node->isShadowRoot()'
> 
> My approach is totally assuming existing layout tests will catch a regression, hopefully.
> If there is no difference in layout test's result whether we have A or B at some place, we should add a layout test for that to detect the different behavior. But adding layout tests is beyond this patch and should be done in other patches.
> 
> Yours is that I tried at first. I tried to replace all B with A at first. But I thought most of them are unnecessary and I got to feel I should rely on existing layout tests for now.
> 
> So either is okay for me. Changing all B with A is an easy task and safest, but I am afraid that will leave unnecessary cluttering of code.
> 
> Any advices are welcome.
> 
> 
> 
> 
> 
> 
> (In reply to comment #29)
> > (From update of attachment 125575 [details] [details])
> > How about line 1000 in Range.cpp:
> >     if (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) {
> > 
> > line 1054:
> >             lastChild = (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) ? newNode->lastChild() : newNode;
> > 
> > Or collectTargetNodes(Node* node, NodeVector& nodes), or line 294:
> >     bool isFragment = newChild->nodeType() == DOCUMENT_FRAGMENT_NODE;
> > in ContainerNode.cpp?
> > 
> > There are so many other places where we compare nodeType to DOCUMENT_FRAGMENT_NODE. Why is it okay for us to start including shadow root in all of those cases? I'd like to see some explanation on why that's the case, if appropriate, for each occurrence of such comparison.

[Ryosuke Niwa]

(In reply to comment #30)
> Let me explain my approach of this change. (I don't intend to insist my approach is correct one. Let discuss what is best one.)
> 
> 1. Treat ShadowRoot and DocumentFragment in the same fashion as long as layout tests do not break.

Unfortunately, having passed layout test isn't a good indication of the change being correct especially in editing/DOM code. I can make hundreds of incorrect changes without breaking existing layout tests.

> 2. Insert isShadowRoot() check where a layout test is broken unless it. I guess we don't have to add such isShadowRoot() check in most places, but I don't want to rebase any layout tests in this patch. That should be done in follow-up patches.
> 
> In general, I chose A over B as long as layout test do not break.
> A. 'node->nodeType() == DOCUMENT_TYPE_NODE
> B. 'node->nodeType() == DOCUMENT_TYPE_NODE && !node->isShadowRoot()'
> 
> My approach is totally assuming existing layout tests will catch a regression, hopefully.

I don't think we can make this assumption. We need to examine every and each occurrence of DOCUMENT_TYPE_NODE and carefully decide whether we need to do A or B.

> If there is no difference in layout test's result whether we have A or B at some place, we should add a layout test for that to detect the different behavior. But adding layout tests is beyond this patch and should be done in other patches.

I tend to agree with you. But we should do our best to prevent regressions by inspecting every and each occurrence of DOCUMENT_TYPE_NODE in codebase like we did for parentNode when we switched text form controls to new shadow dom model. I'm with the answer that all such conditions are tested by existing layout tests but when they're not, we need to make educated decisions. For example, the code in FrameSelection I asked you to modify may have introduced a crash if it wasn't properly updated.

I'm more than happy to help you on such code inspection since I've helped other contributors creating similar patches in the past. However, I don't think we should land the patch as is without it.

[Ryosuke Niwa]

Luckily, this process won't be as bad as parentNode one since there are only 49 occurrences of DOCUMENT_FRAGMENT_NODE in WebCore.

[Hayato Ito]

Thank you for the comment.

I tend to agree with you. I am thinking that we should have most safest way to prevent any regressions.

To avoid having a lot of responsibility in one patch, I'd like to choose replacing all A with B in this patch.
Then, in follow-up patches, we should try to remove unnecessary isShadowRootCheck one-by-one.

Let me replace all A with B in the next patch. Thank you.


(In reply to comment #32)
> (In reply to comment #30)
> > Let me explain my approach of this change. (I don't intend to insist my approach is correct one. Let discuss what is best one.)
> > 
> > 1. Treat ShadowRoot and DocumentFragment in the same fashion as long as layout tests do not break.
> 
> Unfortunately, having passed layout test isn't a good indication of the change being correct especially in editing/DOM code. I can make hundreds of incorrect changes without breaking existing layout tests.
> 
> > 2. Insert isShadowRoot() check where a layout test is broken unless it. I guess we don't have to add such isShadowRoot() check in most places, but I don't want to rebase any layout tests in this patch. That should be done in follow-up patches.
> > 
> > In general, I chose A over B as long as layout test do not break.
> > A. 'node->nodeType() == DOCUMENT_TYPE_NODE
> > B. 'node->nodeType() == DOCUMENT_TYPE_NODE && !node->isShadowRoot()'
> > 
> > My approach is totally assuming existing layout tests will catch a regression, hopefully.
> 
> I don't think we can make this assumption. We need to examine every and each occurrence of DOCUMENT_TYPE_NODE and carefully decide whether we need to do A or B.
> 
> > If there is no difference in layout test's result whether we have A or B at some place, we should add a layout test for that to detect the different behavior. But adding layout tests is beyond this patch and should be done in other patches.
> 
> I tend to agree with you. But we should do our best to prevent regressions by inspecting every and each occurrence of DOCUMENT_TYPE_NODE in codebase like we did for parentNode when we switched text form controls to new shadow dom model. I'm with the answer that all such conditions are tested by existing layout tests but when they're not, we need to make educated decisions. For example, the code in FrameSelection I asked you to modify may have introduced a crash if it wasn't properly updated.
> 
> I'm more than happy to help you on such code inspection since I've helped other contributors creating similar patches in the past. However, I don't think we should land the patch as is without it.

[Dimitri Glazkov (Google)]

This is great, guys.  One thing to consider is to replace these checks with their intent-based equivalents (or create these equivalents).

For example, that check Niwa-san pointed is an optimization which is really asking whether the node is part of a subtree that may never have a selection. In other words, instead of having the plain comparisons, add member functions that also indicate intent or _why_ the test is performed.

The benefit here is both code readability for future engineers and the fact that the "whys" will also fall along the lines of whether to include the check for the shadow root.

[Ryosuke Niwa]

(In reply to comment #35)
> For example, that check Niwa-san pointed is an optimization which is really asking whether the node is part of a subtree that may never have a selection. In other words, instead of having the plain comparisons, add member functions that also indicate intent or _why_ the test is performed.
>
> The benefit here is both code readability for future engineers and the fact that the "whys" will also fall along the lines of whether to include the check for the shadow root.

Yup, that'll be an excellent way to approach patches like this.

[Hayato Ito]

Created attachment 125787 [details]
no change in behavior

[Hayato Ito]

I've updated the patch so that it does not cause any change in behavior where Node.DOCUMENT_FRAGMENT_NODE is used, considering also the context around the point. I expect I covered every places.

(In reply to comment #36)
> (In reply to comment #35)
> > For example, that check Niwa-san pointed is an optimization which is really asking whether the node is part of a subtree that may never have a selection. In other words, instead of having the plain comparisons, add member functions that also indicate intent or _why_ the test is performed.
> >
> > The benefit here is both code readability for future engineers and the fact that the "whys" will also fall along the lines of whether to include the check for the shadow root.
> 
> Yup, that'll be an excellent way to approach patches like this.

That's good. But it might be difficult for me to think a good name of member_function for each points.
I think 'node->nodeType() == Node.DOCUMENT_FRAGMENT_NODE && !node->isShadowRoot()' is very intuitive and is not so bad for future engineers in most cases. I expect that such a long condition reminds us of 'Do we need this isShadowRoot() check? What should we do if it is ShadowRoot?' in the future.

The one good news is that we don't have so many places where we must use 'isShadowRoot()' check as I thought at first.

[Roland Steiner]

Completely ignorant question of mine: Why do we have to test nodeType() everywhere in the first place? That looks like a JavaScript-ism to me. Why not simply use a function isDocumentFragment() similar to isElementNode() and isShadowRoot()?

[Darin Adler]

(In reply to comment #39)
> Why not simply use a function isDocumentFragment() similar to isElementNode() and isShadowRoot()?

We should. However it’s a separate question whether to optimize that function rather than using a virtual function. That’s something we do for isElementNode, for example.

[Darin Adler]

However I don’t think that having a name isDocumentFragment would have any effect on this patch.

[Hayato Ito]

It seemed that I missed your comment. My bad.
I think we can do, assuming that there is no big penalty of performance. But I'd like to confirm whether using virtual function here is encouraged or not. If it is okay, I am willing to introduce and use isDocumentFragment() in a follow-up patch.

(In reply to comment #39)
> Completely ignorant question of mine: Why do we have to test nodeType() everywhere in the first place? That looks like a JavaScript-ism to me. Why not simply use a function isDocumentFragment() similar to isElementNode() and isShadowRoot()?

[Ryosuke Niwa]

Comment on attachment 125787 [details]
no change in behavior

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

> Source/WebCore/dom/ContainerNode.cpp:72
> +    if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE || node->isShadowRoot()) {

Can we introduce Node::isDetachedDocumentFragment() and replace all these conditions by that first? Then, this patch will be a 5-liner.

[Ryosuke Niwa]

I feel bad to keep pushing your patch back but we've had a lot of crashes and security vulnerabilities when we switched text form controls to use new shadow DOM model, and I want to make sure we don't repeat the same here.

[Hayato Ito]

Thank you for the review.

(In reply to comment #43)
> (From update of attachment 125787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review
> 
> > Source/WebCore/dom/ContainerNode.cpp:72
> > +    if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE || node->isShadowRoot()) {
> 
> Can we introduce Node::isDetachedDocumentFragment() and replace all these conditions by that first? Then, this patch will be a 5-liner.

I am afraid it's not good name since ShadowRoot can be also *detached* from Document:
    var shadowHost = document.createElement('div');
    var shadowRoot = new WebKitShadowRoot(shadowHost);
In this case, unless shadowHost is attached to Document, shadowRoot is also detached from Document.
I am willing to replace all these conditions once we have a good name.

(In reply to comment #44)
> I feel bad to keep pushing your patch back but we've had a lot of crashes and security vulnerabilities when we switched text form controls to use new shadow DOM model, and I want to make sure we don't repeat the same here.

No problem. You don't have to feel bad since that's a signal of good reviewers.

[Ryosuke Niwa]

(In reply to comment #45)
> Thank you for the review.
> 
> (In reply to comment #43)
> > (From update of attachment 125787 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review
> > 
> > > Source/WebCore/dom/ContainerNode.cpp:72
> > > +    if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE || node->isShadowRoot()) {
> > 
> > Can we introduce Node::isDetachedDocumentFragment() and replace all these conditions by that first? Then, this patch will be a 5-liner.
> 
> I am afraid it's not good name since ShadowRoot can be also *detached* from Document:
>     var shadowHost = document.createElement('div');
>     var shadowRoot = new WebKitShadowRoot(shadowHost);
> In this case, unless shadowHost is attached to Document, shadowRoot is also detached from Document.
> I am willing to replace all these conditions once we have a good name.

That's a good point. In that case, our change to FrameSelection is incorrect. It breaks the optimization for the case where the detached fragment was a shadow root. So resolving this patch is even tricker than I initially thought.

In fact, in the case of the change in FrameSelection, isDetachedDocumentFragment is exactly what it's asking.

[Hayato Ito]

(In reply to comment #46)
> (In reply to comment #45)
> > Thank you for the review.
> > 
> > (In reply to comment #43)
> > > (From update of attachment 125787 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=125787&action=review
> > > 
> > > > Source/WebCore/dom/ContainerNode.cpp:72
> > > > +    if (node->nodeType() != Node::DOCUMENT_FRAGMENT_NODE || node->isShadowRoot()) {
> > > 
> > > Can we introduce Node::isDetachedDocumentFragment() and replace all these conditions by that first? Then, this patch will be a 5-liner.
> > 
> > I am afraid it's not good name since ShadowRoot can be also *detached* from Document:
> >     var shadowHost = document.createElement('div');
> >     var shadowRoot = new WebKitShadowRoot(shadowHost);
> > In this case, unless shadowHost is attached to Document, shadowRoot is also detached from Document.
> > I am willing to replace all these conditions once we have a good name.
> 
> That's a good point. In that case, our change to FrameSelection is incorrect. It breaks the optimization for the case where the detached fragment was a shadow root. So resolving this patch is even tricker than I initially thought.
> 
> In fact, in the case of the change in FrameSelection, isDetachedDocumentFragment is exactly what it's asking.

I see. But that should be fixed in another patch, right? It is not good to include any bug fix in this patch.
Our naming of 'nodeIsDetachedFromDocument(Node* node)' in FrameSelection.cpp is not correct, but it is not the change in behavior.

[Hayato Ito]

Hi reviewers, could you take a look?
I think this patch is not harmful in theory. No change in behavior. No blocking issue.

Follow-up patches could fix some minor points which were found in the review.

[Ryosuke Niwa]

Comment on attachment 125787 [details]
no change in behavior

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1143
> +        if (!node->isShadowRoot())
> +            break;
> +        nodeName = node->nodeName();
> +        localName = node->localName();

Where did this code come from?

[Hayato Ito]

Comment on attachment 125787 [details]
no change in behavior

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

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1143
>> +        localName = node->localName();
> 
> Where did this code come from?

Take a look at default case of this switch/case statements.

[Ryosuke Niwa]

Comment on attachment 125787 [details]
no change in behavior

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

>>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1143
>>> +        localName = node->localName();
>> 
>> Where did this code come from?
> 
> Take a look at default case of this switch/case statements.

I think it's better to move this case below Node::ELEMENT_NODE and let it fall through when the node is a shadow root rather than duplicating code here.

[Hayato Ito]

Comment on attachment 125787 [details]
no change in behavior

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

>>>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1143
>>>> +        localName = node->localName();
>>> 
>>> Where did this code come from?
>> 
>> Take a look at default case of this switch/case statements.
> 
> I think it's better to move this case below Node::ELEMENT_NODE and let it fall through when the node is a shadow root rather than duplicating code here.

Moving this case below Node::ELEMENT_NODE causes unnecessary isShadowRoot() calls for Node::DOCUMENT_NODE and NODE::ELEMENT_NODE, doesn't that?

case Node::DOCUMENT_NODE:
case Node::ELEMENT_NODE:
case NODE::DOCUMENT_FRAGMENT_NODE:
    if (!node->isShadowRoot())
        break;
    // Fall through
default:
    nodename = node->nodeName();
    localName = node->localName();
    break;


I guess you meant this?

case NODE::DOCUMENT_FRAGMENT_NODE:
    if (!node->isShadowRoot())
        break;
    // Fall through
case Node::DOCUMENT_NODE:
case Node::ELEMENT_NODE:
default:
    nodename = node->nodeName();
    localName = node->localName();
    break;

[Hayato Ito]

Created attachment 126721 [details]
fall through

[Hayato Ito]

After I get r+, I'll rebase and check all code path again for ToT.

[Ryosuke Niwa]

Comment on attachment 126721 [details]
fall through

Looks okay to me. Maybe Dimitri should take another look as well?

[Dimitri Glazkov (Google)]

Comment on attachment 126721 [details]
fall through

Looks good to me.

[WebKit Review Bot]

Comment on attachment 126721 [details]
fall through

Clearing flags on attachment: 126721

Committed r107661: <http://trac.webkit.org/changeset/107661>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

This is crashing the Lion bots. see eg. http://build.webkit.org/results/Lion%20Intel%20Debug%20(Tests)/r107672%20(3536)/editing/inserting/5607069-2-crash-log.txt

[Csaba Osztrogonác]

(In reply to comment #59)
> This is crashing the Lion bots. see eg. http://build.webkit.org/results/Lion%20Intel%20Debug%20(Tests)/r107672%20(3536)/editing/inserting/5607069-2-crash-log.txt

and Qt too - http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r107661%20%2821103%29/results.html

[Hayato Ito]

Thank you for letting me know it. Let me roll out.

(In reply to comment #60)
> (In reply to comment #59)
> > This is crashing the Lion bots. see eg. http://build.webkit.org/results/Lion%20Intel%20Debug%20(Tests)/r107672%20(3536)/editing/inserting/5607069-2-crash-log.txt
> 
> and Qt too - http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r107661%20%2821103%29/results.html

[Hayato Ito]

r107661 was rolled out in http://trac.webkit.org/changeset/107691.

I am sure that the root cause is ASSERT in JSNodeCustom.

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

> Source/WebCore/bindings/js/JSNodeCustom.cpp:257
> +            // In case of ShadowRoot, a cached ShadowRoot binding is always used and this


FYI. morrita-san, our assumption was apparently wrong.
Let me update the patch so that it does not include this assertion.

[Hayato Ito]

Created attachment 126945 [details]
Patch for landing

[Hayato Ito]

I've updated the patch, removing ASSERT which caused the crashes.
I've tested it on Mac debug build, and confirmed that crash has gone. Let me land the patch using land-safely.

[WebKit Review Bot]

Comment on attachment 126945 [details]
Patch for landing

Clearing flags on attachment: 126945

Committed r107700: <http://trac.webkit.org/changeset/107700>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.