Bug 77514

Summary: ShadowRoot's nodeType should be DOCUMENT_FRAGMENT_NODE.
Product: WebKit Reporter: Hayato Ito <hayato>
Component: DOMAssignee: Hayato Ito <hayato>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, dglazkov, dominicc, enrica, ggaren, gns, haraken, japhet, morrita, ossy, rniwa, rolandsteiner, shinyak, webkit.review.bot, xan.lopez, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 77511, 78591    
Bug Blocks: 63601    
Attachments:
Description Flags
get rid of NodeType.SHADOW_ROOT_NODE
none
fix JSC.
none
Add ASSERT
none
Fix mac build hopefully
none
rebase some layout tests
none
cleanup. remove unnecessary (hopefully) isShadowRoot() check logic as long as regressions does not happen.
none
regression has gone.
none
ready for review. let me watch whether mac-build error is reproducible or not
none
no change in behavior
none
fall through
none
Patch for landing none

Description Hayato Ito 2012-01-31 23:24:18 PST
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".
Comment 1 Hayato Ito 2012-02-01 22:09:52 PST
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.
Comment 2 Hayato Ito 2012-02-02 21:22:20 PST
I'ver renamed title of bug, focusing on only ShadowRoot.nodeType.
Comment 3 Hayato Ito 2012-02-02 21:37:48 PST
Created attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE
Comment 4 Hajime Morrita 2012-02-02 21:47:41 PST
Comment on attachment 125257 [details]
get rid of NodeType.SHADOW_ROOT_NODE

Let's land this once cr-linux gets gtreen.
Comment 5 Early Warning System Bot 2012-02-02 21:59:55 PST
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
Comment 6 Hayato Ito 2012-02-02 22:28:51 PST
It seems we need to update JSNodeCustom too. Let me fix it.
Comment 7 WebKit Review Bot 2012-02-02 22:32:40 PST
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
Comment 8 Gustavo Noronha (kov) 2012-02-02 22:33:01 PST
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
Comment 9 Gyuyoung Kim 2012-02-02 22:36:59 PST
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
Comment 10 Hayato Ito 2012-02-02 22:45:39 PST
Created attachment 125264 [details]
fix JSC.
Comment 11 Hayato Ito 2012-02-02 22:48:42 PST
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.
Comment 12 Hayato Ito 2012-02-03 00:40:01 PST
Created attachment 125276 [details]
Add ASSERT
Comment 13 Hayato Ito 2012-02-03 00:42:42 PST
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.
Comment 14 Hayato Ito 2012-02-03 00:44:53 PST
It seems I wrongly cleared the r+ flag on the previous patch. So let me set r? in the latest patch.
Comment 15 Hayato Ito 2012-02-03 01:02:07 PST
Created attachment 125282 [details]
Fix mac build hopefully
Comment 16 WebKit Review Bot 2012-02-03 02:28:33 PST
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
Comment 17 Hayato Ito 2012-02-03 03:50:43 PST
Created attachment 125304 [details]
rebase some layout tests
Comment 18 Hayato Ito 2012-02-03 03:59:21 PST
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.
Comment 19 Hayato Ito 2012-02-03 04:01:10 PST
[Wrong in the previous post]
> 'node->nodeType() == DOCUMENT_FRAGMENT_NODE && node->isShadowRoot()'

[Correct]
> 'node->nodeType() == DOCUMENT_FRAGMENT_NODE && !node->isShadowRoot()'
Comment 20 Hayato Ito 2012-02-03 04:39:11 PST
Created attachment 125307 [details]
cleanup. remove unnecessary (hopefully) isShadowRoot() check logic as long as regressions does not happen.
Comment 21 Dimitri Glazkov (Google) 2012-02-03 09:29:09 PST
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? :)
Comment 22 Ryosuke Niwa 2012-02-03 10:29:39 PST
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
Comment 23 Ryosuke Niwa 2012-02-03 10:32:35 PST
In general, you have to examine every place that compares the node type to Node::DOCUMENT_FRAGMENT_NODE.
Comment 24 Hayato Ito 2012-02-05 22:08:38 PST
Created attachment 125570 [details]
regression has gone.
Comment 25 Ryosuke Niwa 2012-02-05 22:17:50 PST
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.
Comment 26 Hayato Ito 2012-02-05 22:26:40 PST
Created attachment 125575 [details]
ready for review. let me watch whether mac-build error is reproducible or not
Comment 27 Hayato Ito 2012-02-05 22:28:39 PST
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.
Comment 28 Hayato Ito 2012-02-06 18:45:46 PST
Ready for review. Could you take another look?
Comment 29 Ryosuke Niwa 2012-02-06 19:44:20 PST
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.
Comment 30 Hayato Ito 2012-02-06 20:18:24 PST
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.
Comment 31 Hayato Ito 2012-02-06 20:30:56 PST
<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.
Comment 32 Ryosuke Niwa 2012-02-06 20:39:57 PST
(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.
Comment 33 Ryosuke Niwa 2012-02-06 20:47:26 PST
Luckily, this process won't be as bad as parentNode one since there are only 49 occurrences of DOCUMENT_FRAGMENT_NODE in WebCore.
Comment 34 Hayato Ito 2012-02-06 20:50:38 PST
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.
Comment 35 Dimitri Glazkov (Google) 2012-02-06 20:54:45 PST
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.
Comment 36 Ryosuke Niwa 2012-02-06 20:58:12 PST
(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.
Comment 37 Hayato Ito 2012-02-07 01:47:49 PST
Created attachment 125787 [details]
no change in behavior
Comment 38 Hayato Ito 2012-02-07 01:53:12 PST
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.
Comment 39 Roland Steiner 2012-02-07 16:38:28 PST
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()?
Comment 40 Darin Adler 2012-02-07 17:49:14 PST
(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.
Comment 41 Darin Adler 2012-02-07 17:50:34 PST
However I don’t think that having a name isDocumentFragment would have any effect on this patch.
Comment 42 Hayato Ito 2012-02-07 18:06:48 PST
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()?
Comment 43 Ryosuke Niwa 2012-02-08 15:24:45 PST
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.
Comment 44 Ryosuke Niwa 2012-02-08 15:26:32 PST
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.
Comment 45 Hayato Ito 2012-02-08 18:19:01 PST
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.
Comment 46 Ryosuke Niwa 2012-02-08 18:22:47 PST
(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.
Comment 47 Hayato Ito 2012-02-08 18:44:16 PST
(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.
Comment 48 Hayato Ito 2012-02-12 17:58:50 PST
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.
Comment 49 Ryosuke Niwa 2012-02-12 18:49:51 PST
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?
Comment 50 Hayato Ito 2012-02-12 20:13:27 PST
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.
Comment 51 Ryosuke Niwa 2012-02-12 20:25:41 PST
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.
Comment 52 Hayato Ito 2012-02-12 20:42:32 PST
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;
Comment 53 Hayato Ito 2012-02-12 23:10:20 PST
Created attachment 126721 [details]
fall through
Comment 54 Hayato Ito 2012-02-12 23:27:33 PST
After I get r+, I'll rebase and check all code path again for ToT.
Comment 55 Ryosuke Niwa 2012-02-13 12:34:51 PST
Comment on attachment 126721 [details]
fall through

Looks okay to me. Maybe Dimitri should take another look as well?
Comment 56 Dimitri Glazkov (Google) 2012-02-13 12:38:31 PST
Comment on attachment 126721 [details]
fall through

Looks good to me.
Comment 57 WebKit Review Bot 2012-02-13 19:15:36 PST
Comment on attachment 126721 [details]
fall through

Clearing flags on attachment: 126721

Committed r107661: <http://trac.webkit.org/changeset/107661>
Comment 58 WebKit Review Bot 2012-02-13 19:15:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 61 Hayato Ito 2012-02-14 01:25:06 PST
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
Comment 62 Hayato Ito 2012-02-14 02:02:22 PST
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.
Comment 63 Hayato Ito 2012-02-14 02:47:57 PST
Created attachment 126945 [details]
Patch for landing
Comment 64 Hayato Ito 2012-02-14 02:54:36 PST
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.
Comment 65 WebKit Review Bot 2012-02-14 03:32:29 PST
Comment on attachment 126945 [details]
Patch for landing

Clearing flags on attachment: 126945

Committed r107700: <http://trac.webkit.org/changeset/107700>
Comment 66 WebKit Review Bot 2012-02-14 03:32:37 PST
All reviewed patches have been landed.  Closing bug.