Bug 59304 – Crash when adding a text node to a shadow root

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dominic Cooney]

Adding a text node to a shadow root crashes; it should instead present the text in the shadow DOM.

[Dominic Cooney]

Created attachment 90885 [details]
Patch

[WebKit Review Bot]

Attachment 90885 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8507105
Unexpected failures:
fast/dom/shadow/gc-shadow.html
fast/dom/shadow/append-child-text.html

[Dominic Cooney]

I’m setting up a Linux build environment to investigate the cr-linux EWS failures. I don’t know why fast/dom/shadow/append-child-text.html is an unexpected failure… the linked log output indicates that it passed :/

[Dominic Cooney]

s/append-child-text/gc-shadow/

[Adam Barth]

(In reply to comment #4)
> s/append-child-text/gc-shadow/

Hum...  Interesting.  Running tests on the EWS is still very experimental.  I wouldn't be surprised of there were some bugs.

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> I’m setting up a Linux build environment to investigate the cr-linux EWS failures. I don’t know why fast/dom/shadow/append-child-text.html is an unexpected failure… the linked log output indicates that it passed :/

I don't think you need a Linux env to test this. There's nothing Linuxey about this code.

[Dimitri Glazkov (Google)]

Comment on attachment 90885 [details]
Patch

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

> LayoutTests/fast/dom/shadow/append-child-text-expected.txt:13
> +          text run at (0,0) width 39: "PASS"

Can this test be dumpAsText?

[Dominic Cooney]

Comment on attachment 90885 [details]
Patch

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

>> LayoutTests/fast/dom/shadow/append-child-text-expected.txt:13
>> +          text run at (0,0) width 39: "PASS"
> 
> Can this test be dumpAsText?

No, because shadow DOM is not dumped as text. That it doesn’t crash is observable, but that the text appears is not observable with a text result. WDYT?

[Dominic Cooney]

cr-linux’s complaint is probably the lack of pixel test results.

[Dimitri Glazkov (Google)]

(In reply to comment #9)
> cr-linux’s complaint is probably the lack of pixel test results.

Yes, and it will need its own expectations due to text metric differences.

Let's make this a dumpAsText.

Perhaps event file a bug to teach the dumper about shadow DOM.

[Dominic Cooney]

Created attachment 90886 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 90886 [details]
Patch

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

the only nit remaining:

> LayoutTests/fast/dom/shadow/append-child-text.html:4
> +<script src="../../js/resources/js-test-pre.js"></script>

don't really need the harness for this test.

[Dominic Cooney]

Created attachment 90887 [details]
Patch

[Dominic Cooney]

For the fossil record: filed bug 59305 for adding text in shadow DOM to layout test text output.

[WebKit Review Bot]

Attachment 90887 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8504488
Unexpected failures:
fast/dom/shadow/gc-shadow.html

[WebKit Commit Bot]

Comment on attachment 90887 [details]
Patch

Clearing flags on attachment: 90887

Committed r84759: <http://trac.webkit.org/changeset/84759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> Attachment 90885 [details] did not pass chromium-ews:
> Output: http://queues.webkit.org/results/8507105
> Unexpected failures:
> fast/dom/shadow/gc-shadow.html
> fast/dom/shadow/append-child-text.html

EWS, you were right. gc-shadow is now crashing on canaries. Dominic, Adam, can you rollout on #webkit?