Bug 59304 - Crash when adding a text node to a shadow root
Summary: Crash when adding a text node to a shadow root
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 59306
Blocks: 52962
  Show dependency treegraph
 
Reported: 2011-04-24 12:15 PDT by Dominic Cooney
Modified: 2011-04-24 16:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.42 KB, patch)
2011-04-24 12:32 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2011-04-24 13:44 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (5.03 KB, patch)
2011-04-24 13:55 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2011-04-24 12:15:50 PDT
Adding a text node to a shadow root crashes; it should instead present the text in the shadow DOM.
Comment 1 Dominic Cooney 2011-04-24 12:32:44 PDT
Created attachment 90885 [details]
Patch
Comment 2 WebKit Review Bot 2011-04-24 12:53:02 PDT
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
Comment 3 Dominic Cooney 2011-04-24 13:07:28 PDT
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 :/
Comment 4 Dominic Cooney 2011-04-24 13:08:08 PDT
s/append-child-text/gc-shadow/
Comment 5 Adam Barth 2011-04-24 13:20:41 PDT
(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.
Comment 6 Dimitri Glazkov (Google) 2011-04-24 13:24:05 PDT
(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.
Comment 7 Dimitri Glazkov (Google) 2011-04-24 13:30:53 PDT
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?
Comment 8 Dominic Cooney 2011-04-24 13:32:26 PDT
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?
Comment 9 Dominic Cooney 2011-04-24 13:33:35 PDT
cr-linux’s complaint is probably the lack of pixel test results.
Comment 10 Dimitri Glazkov (Google) 2011-04-24 13:34:43 PDT
(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.
Comment 11 Dominic Cooney 2011-04-24 13:44:31 PDT
Created attachment 90886 [details]
Patch
Comment 12 Dimitri Glazkov (Google) 2011-04-24 13:45:49 PDT
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.
Comment 13 Dominic Cooney 2011-04-24 13:55:51 PDT
Created attachment 90887 [details]
Patch
Comment 14 Dominic Cooney 2011-04-24 14:00:43 PDT
For the fossil record: filed bug 59305 for adding text in shadow DOM to layout test text output.
Comment 15 WebKit Review Bot 2011-04-24 14:15:10 PDT
Attachment 90887 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8504488
Unexpected failures:
fast/dom/shadow/gc-shadow.html
Comment 16 WebKit Commit Bot 2011-04-24 14:20:53 PDT
Comment on attachment 90887 [details]
Patch

Clearing flags on attachment: 90887

Committed r84759: <http://trac.webkit.org/changeset/84759>
Comment 17 WebKit Commit Bot 2011-04-24 14:20:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Dimitri Glazkov (Google) 2011-04-24 16:02:28 PDT
(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?