Bug 79620 - Appending ShadowRoot into an element should not cause crash.
Summary: Appending ShadowRoot into an element should not cause crash.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-26 21:08 PST by Shinya Kawanaka
Modified: 2012-03-01 10:06 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.62 KB, patch)
2012-02-26 22:17 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (4.06 KB, patch)
2012-02-29 20:55 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch for landing (8.16 KB, patch)
2012-02-29 21:20 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-02-26 21:08:12 PST
This will cause an assertion failure.

var div1 = document.createElement('div');
var root = new WebKithShadowRoot(div1);

var div2 = document.createElement('div');
div2.appendChild(root);
Comment 1 Hayato Ito 2012-02-26 21:19:10 PST
I think we have two options in this case:

A). Throws DOM Exception
B). ShadowRoot acts like as DOCUMENT_FRAGMENT so every children of shadowRoot are appended.

It seems the spec says B implicitly because ShadowRoot's nodeType is DOCUMENT_FRAGMENT_NODE.
I thinks A is safer option, but we should file a bug for the spec if we choose A.
Comment 2 Shinya Kawanaka 2012-02-26 22:13:31 PST
I've filed a bug here.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=16130
Comment 3 Shinya Kawanaka 2012-02-26 22:17:17 PST
Created attachment 128953 [details]
Patch
Comment 4 Shinya Kawanaka 2012-02-26 22:18:45 PST
(In reply to comment #1)
> I think we have two options in this case:
> 
> A). Throws DOM Exception
> B). ShadowRoot acts like as DOCUMENT_FRAGMENT so every children of shadowRoot are appended.
> 
> It seems the spec says B implicitly because ShadowRoot's nodeType is DOCUMENT_FRAGMENT_NODE.
> I thinks A is safer option, but we should file a bug for the spec if we choose A.

I wrote a patch to reject appending shadow root.
If we should choose (B) option, I'll change the patch.
Comment 5 Dimitri Glazkov (Google) 2012-02-27 10:18:23 PST
Comment on attachment 128953 [details]
Patch

It should act as a document fragment.
Comment 6 Shinya Kawanaka 2012-02-29 17:37:01 PST
(In reply to comment #5)
> (From update of attachment 128953 [details])
> It should act as a document fragment.

OK. Thnaks, Dimitri. I'll update the patch.
Comment 7 Shinya Kawanaka 2012-02-29 20:55:27 PST
Created attachment 129624 [details]
Patch
Comment 8 Dimitri Glazkov (Google) 2012-02-29 20:58:25 PST
Comment on attachment 129624 [details]
Patch

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

> LayoutTests/fast/dom/shadow/shadow-root-append.html:25
> +    shouldBeNull('root.firstChild');

Should you be checking to see if the container has chlidren now?
Comment 9 James Robinson 2012-02-29 21:04:29 PST
Woah. the watchlist thought this patch touched Source/WebCore/platform/graphics/chromium/*. very weird!
Comment 10 Shinya Kawanaka 2012-02-29 21:10:07 PST
(In reply to comment #8)
> (From update of attachment 129624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129624&action=review
> 
> > LayoutTests/fast/dom/shadow/shadow-root-append.html:25
> > +    shouldBeNull('root.firstChild');
> 
> Should you be checking to see if the container has chlidren now?

Sure.
Comment 11 Shinya Kawanaka 2012-02-29 21:15:01 PST
Ah, I found that I missed adding Skipped list. I'll land this patch with Skipped list.
Comment 12 Shinya Kawanaka 2012-02-29 21:20:33 PST
Created attachment 129625 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-03-01 00:54:05 PST
Comment on attachment 129625 [details]
Patch for landing

Rejecting attachment 129625 [details] from commit-queue.

shinyak@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 14 WebKit Review Bot 2012-03-01 10:06:04 PST
Comment on attachment 129625 [details]
Patch for landing

Clearing flags on attachment: 129625

Committed r109359: <http://trac.webkit.org/changeset/109359>
Comment 15 WebKit Review Bot 2012-03-01 10:06:10 PST
All reviewed patches have been landed.  Closing bug.