Bug 78878 - Adding a ShadowRoot to image-backed element causes a crash
Summary: Adding a ShadowRoot to image-backed element causes a 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: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-16 22:38 PST by Hajime Morrita
Modified: 2012-02-24 01:59 PST (History)
4 users (show)

See Also:


Attachments
Patch (15.93 KB, patch)
2012-02-17 03:10 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (45.56 KB, patch)
2012-02-20 01:56 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (46.44 KB, patch)
2012-02-20 16:36 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (47.20 KB, patch)
2012-02-20 19:31 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (46.44 KB, patch)
2012-02-20 19:34 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (19.19 MB, application/zip)
2012-02-20 21:12 PST, WebKit Review Bot
no flags Details
Patch for landing (46.42 KB, patch)
2012-02-21 01:52 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (47.24 KB, patch)
2012-02-21 19:59 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (51.04 KB, patch)
2012-02-22 16:51 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (55.04 KB, patch)
2012-02-23 15:45 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Fixing windows build break (55.03 KB, patch)
2012-02-23 17:32 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (56.79 KB, patch)
2012-02-23 20:25 PST, Hajime Morrita
morrita: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-02-16 22:38:15 PST
From: http://code.google.com/p/chromium/issues/detail?id=113167
Comment 1 Hajime Morrita 2012-02-16 23:16:32 PST
I changed the summary line. since the reduced case unveiled that <iframe> is innocent. 
----
<script>
function boom() {
  var divWithImage = document.createElement('div');
  divWithImage.setAttribute("style", "content: url(data:text/plain,aaa);");
  document.documentElement.appendChild(divWithImage);
  var baseShadow = new WebKitShadowRoot(divWithImage); 
  baseShadow.appendChild(document.createElement('div'));
}

window.onload = boom;
</script>
Comment 2 Hajime Morrita 2012-02-17 03:10:05 PST
Created attachment 127566 [details]
Patch
Comment 3 Hajime Morrita 2012-02-17 03:14:23 PST
Hi Dimitri, could you take a look at this?

This bug is pretty interesting because the crash spots the replaced element ambiguity.
In ideal world, any replaced element is capable to have shadow children.
But current implementation isn't. NodeRenderingContext tries to attach child to such children
which caused the crash.
Comment 4 Dimitri Glazkov (Google) 2012-02-17 10:32:44 PST
Comment on attachment 127566 [details]
Patch

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

This is interesting. This specific bit of code uses CSS content property, which means that not rendering children is a purely CSS concept, and not something we could ever rationalize in terms of the shadow DOM. These two parallel worlds (CSS replaced content and shadow DOM) will continue to need to coexist.

In this sense, the meaning of canHaveChildren changes to "will the renderers ever be created for my children" and "will I have children renderers attached to my renderer as children". I think we should try to stick to these concepts and decouple them from the shadow DOM itself. Does it make sense?

> Source/WebCore/dom/NodeRenderingContext.cpp:279
> +        if (RenderObject::ParentableForAll != parentRenderer->parentability())

It's weird, but you never check for ParentableForShadow. If there are only two states, why have more than two exposed?

> Source/WebCore/dom/NodeRenderingContext.cpp:284
> +        ASSERT(m_phase == AttachingShadowChild

Now that I look at it, "ShadowChild" doesn't make sense anymore. I think we should align this with the spec wording, too.

> Source/WebCore/rendering/RenderButton.h:59
> +    virtual Parentability parentability() const OVERRIDE;

parentability is not a real word :)
Comment 5 Hajime Morrita 2012-02-20 01:56:25 PST
Created attachment 127784 [details]
Patch
Comment 6 Hajime Morrita 2012-02-20 01:58:51 PST
(In reply to comment #4)
> 
> This is interesting. This specific bit of code uses CSS content property, which means that not rendering children is a purely CSS concept, and not something we could ever rationalize in terms of the shadow DOM. These two parallel worlds (CSS replaced content and shadow DOM) will continue to need to coexist.

You are definitely right. Definitely.
I retook the patch in this direction and found it more sense.
Could you take another look?
Comment 7 WebKit Review Bot 2012-02-20 04:36:05 PST
Comment on attachment 127784 [details]
Patch

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

New failing tests:
dom/svg/level3/xpath/Comment_Nodes.svg
http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-005.htm
compositing/images/direct-svg-image.html
css3/images/cross-fade-overflow-position.html
css2.1/20110323/replaced-intrinsic-002.htm
dom/svg/level3/xpath/Attribute_Nodes_xmlns.svg
http/tests/inspector/inspect-element.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
dom/svg/level3/xpath/Conformance_isSupported_3.svg
dom/svg/level3/xpath/Conformance_isSupported_empty.svg
css2.1/20110323/background-intrinsic-002.htm
dom/svg/level3/xpath/Conformance_ID.svg
css2.1/20110323/abspos-containing-block-initial-004d.htm
css2.1/20110323/background-intrinsic-007.htm
dom/svg/level3/xpath/Conformance_hasFeature_3.svg
dom/svg/level3/xpath/Conformance_hasFeature_empty.svg
css2.1/20110323/background-intrinsic-009.htm
css2.1/20110323/background-intrinsic-001.htm
accessibility/aria-describedby-on-input.html
css3/zoom-coords.xhtml
css2.1/20110323/replaced-intrinsic-001.htm
dom/svg/level3/xpath/Attribute_Nodes.svg
css2.1/20110323/background-intrinsic-008.htm
css3/images/cross-fade-invalidation.html
dom/svg/level3/xpath/Conformance_hasFeature_null.svg
dom/svg/level3/xpath/Conformance_Expressions.svg
css2.1/20110323/background-intrinsic-003.htm
Comment 8 Dimitri Glazkov (Google) 2012-02-20 09:06:52 PST
Comment on attachment 127784 [details]
Patch

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

This patch looks awesome! There are some failures on bots, which means it's not yet ready, but I really like the direction.

> Source/WebCore/ChangeLog:19
> +        This change re-align the responsibility:

"re-aligns"

> Source/WebCore/ChangeLog:23
> +          return true regardless the HTML semantics allows its node to do that.

"regardless of HTML semantics."

> Source/WebCore/ChangeLog:34
> +        By this change, some decision points are moved from a renderer to an

"With this change"
Comment 9 Hajime Morrita 2012-02-20 16:36:47 PST
Created attachment 127869 [details]
Patch
Comment 10 Hajime Morrita 2012-02-20 16:41:06 PST
(In reply to comment #8)
> (From update of attachment 127784 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127784&action=review
> 
> This patch looks awesome! There are some failures on bots, which means it's not yet ready, but I really like the direction.
Thanks for taking a look! I updated the patch to fix the build and bad grammars.
Comment 11 Hajime Morrita 2012-02-20 19:31:49 PST
Created attachment 127897 [details]
Patch
Comment 12 Hajime Morrita 2012-02-20 19:34:01 PST
Created attachment 127898 [details]
Patch
Comment 13 Dimitri Glazkov (Google) 2012-02-20 20:56:49 PST
Comment on attachment 127898 [details]
Patch

r=me, provided build is fixed.
Comment 14 WebKit Review Bot 2012-02-20 21:12:30 PST
Comment on attachment 127898 [details]
Patch

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

New failing tests:
dom/svg/level3/xpath/Comment_Nodes.svg
http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-005.htm
compositing/images/direct-svg-image.html
css3/images/cross-fade-overflow-position.html
css2.1/20110323/replaced-intrinsic-002.htm
dom/svg/level3/xpath/Attribute_Nodes_xmlns.svg
http/tests/inspector/inspect-element.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
dom/svg/level3/xpath/Conformance_isSupported_3.svg
dom/svg/level3/xpath/Conformance_isSupported_empty.svg
css2.1/20110323/background-intrinsic-002.htm
dom/svg/level3/xpath/Conformance_ID.svg
css2.1/20110323/abspos-containing-block-initial-004d.htm
css2.1/20110323/background-intrinsic-007.htm
dom/svg/level3/xpath/Conformance_hasFeature_3.svg
dom/svg/level3/xpath/Conformance_hasFeature_empty.svg
css2.1/20110323/background-intrinsic-009.htm
css2.1/20110323/background-intrinsic-001.htm
accessibility/aria-describedby-on-input.html
css3/zoom-coords.xhtml
css2.1/20110323/replaced-intrinsic-001.htm
dom/svg/level3/xpath/Attribute_Nodes.svg
css2.1/20110323/background-intrinsic-008.htm
css3/images/cross-fade-invalidation.html
dom/svg/level3/xpath/Conformance_hasFeature_null.svg
dom/svg/level3/xpath/Conformance_Expressions.svg
css2.1/20110323/background-intrinsic-003.htm
Comment 15 WebKit Review Bot 2012-02-20 21:12:45 PST
Created attachment 127912 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 16 Hajime Morrita 2012-02-21 01:52:31 PST
Created attachment 127940 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-02-21 04:55:06 PST
Comment on attachment 127940 [details]
Patch for landing

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

New failing tests:
dom/svg/level3/xpath/Comment_Nodes.svg
http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-005.htm
compositing/images/direct-svg-image.html
css3/images/cross-fade-overflow-position.html
css2.1/20110323/replaced-intrinsic-002.htm
dom/svg/level3/xpath/Attribute_Nodes_xmlns.svg
http/tests/inspector/inspect-element.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
dom/svg/level3/xpath/Conformance_isSupported_3.svg
dom/svg/level3/xpath/Conformance_isSupported_empty.svg
css2.1/20110323/background-intrinsic-002.htm
dom/svg/level3/xpath/Conformance_ID.svg
css2.1/20110323/abspos-containing-block-initial-004d.htm
css2.1/20110323/background-intrinsic-007.htm
dom/svg/level3/xpath/Conformance_hasFeature_3.svg
dom/svg/level3/xpath/Conformance_hasFeature_empty.svg
css2.1/20110323/background-intrinsic-009.htm
css2.1/20110323/background-intrinsic-001.htm
accessibility/aria-describedby-on-input.html
css3/zoom-coords.xhtml
css2.1/20110323/replaced-intrinsic-001.htm
dom/svg/level3/xpath/Attribute_Nodes.svg
css2.1/20110323/background-intrinsic-008.htm
css3/images/cross-fade-invalidation.html
dom/svg/level3/xpath/Conformance_hasFeature_null.svg
dom/svg/level3/xpath/Conformance_Expressions.svg
css2.1/20110323/background-intrinsic-003.htm
Comment 18 Dimitri Glazkov (Google) 2012-02-21 08:24:33 PST
(In reply to comment #17)
> (From update of attachment 127940 [details])
> Attachment 127940 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11446005
> 
> New failing tests:
> dom/svg/level3/xpath/Comment_Nodes.svg
> http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
> css2.1/20110323/background-intrinsic-004.htm
> css2.1/20110323/background-intrinsic-006.htm
> css2.1/20110323/background-intrinsic-005.htm
> compositing/images/direct-svg-image.html
> css3/images/cross-fade-overflow-position.html
> css2.1/20110323/replaced-intrinsic-002.htm
> dom/svg/level3/xpath/Attribute_Nodes_xmlns.svg
> http/tests/inspector/inspect-element.html
> css2.1/20110323/abspos-containing-block-initial-004b.htm
> dom/svg/level3/xpath/Conformance_isSupported_3.svg
> dom/svg/level3/xpath/Conformance_isSupported_empty.svg
> css2.1/20110323/background-intrinsic-002.htm
> dom/svg/level3/xpath/Conformance_ID.svg
> css2.1/20110323/abspos-containing-block-initial-004d.htm
> css2.1/20110323/background-intrinsic-007.htm
> dom/svg/level3/xpath/Conformance_hasFeature_3.svg
> dom/svg/level3/xpath/Conformance_hasFeature_empty.svg
> css2.1/20110323/background-intrinsic-009.htm
> css2.1/20110323/background-intrinsic-001.htm
> accessibility/aria-describedby-on-input.html
> css3/zoom-coords.xhtml
> css2.1/20110323/replaced-intrinsic-001.htm
> dom/svg/level3/xpath/Attribute_Nodes.svg
> css2.1/20110323/background-intrinsic-008.htm
> css3/images/cross-fade-invalidation.html
> dom/svg/level3/xpath/Conformance_hasFeature_null.svg
> dom/svg/level3/xpath/Conformance_Expressions.svg
> css2.1/20110323/background-intrinsic-003.htm

I think SVG hates you :)
Comment 19 Hajime Morrita 2012-02-21 16:06:06 PST
(In reply to comment #18)
> 
> I think SVG hates you :)
Hmm I hope there were just existing redness. But that wasn't true. Looking.
Comment 20 Hajime Morrita 2012-02-21 19:59:36 PST
Created attachment 128122 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-02-21 21:54:45 PST
Comment on attachment 128122 [details]
Patch for landing

Rejecting attachment 128122 [details] from commit-queue.

New failing tests:
media/audio-garbage-collect.html
fullscreen/video-controls-timeline.html
media/audio-repaint.html
media/audio-controls-rendering.html
media/track/track-cue-rendering.html
media/video-empty-source.html
http/tests/inspector/inspect-element.html
http/tests/media/video-referer.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
fast/frames/lots-of-objects.html
media/video-controls-zoomed.html
media/controls-without-preload.html
css2.1/20110323/abspos-containing-block-initial-004d.htm
media/video-controls-visible-audio-only.html
media/audio-delete-while-slider-thumb-clicked.html
media/controls-after-reload.html
media/media-document-audio-repaint.html
fast/loader/subresource-willSendRequest-null.html
media/controls-styling.html
media/video-controls-rendering.html
media/media-controls-clone.html
fast/layers/video-layer.html
accessibility/aria-describedby-on-input.html
media/video-controls-transformed.html
fullscreen/full-screen-stacking-context.html
media/controls-strict.html
media/controls-drag-timebar.html
media/video-display-toggle.html
inspector/protocol/console-agent.html
media/controls-layout-direction.html
Full output: http://queues.webkit.org/results/11520374
Comment 22 Hajime Morrita 2012-02-22 02:00:05 PST
These failure looks just freakiness. They run well on my machine.
I'll land this manually tomorrow.
Comment 23 Hajime Morrita 2012-02-22 16:51:22 PST
Created attachment 128333 [details]
Patch for landing
Comment 24 Hajime Morrita 2012-02-22 16:52:00 PST
Well, I was wrong again. asking cq+...
Comment 25 WebKit Review Bot 2012-02-22 20:01:31 PST
Comment on attachment 128333 [details]
Patch for landing

Clearing flags on attachment: 128333

Committed r108602: <http://trac.webkit.org/changeset/108602>
Comment 26 WebKit Review Bot 2012-02-22 20:02:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Yuta Kitamura 2012-02-22 21:46:19 PST
Reverted r108602 for reason:

Caused a couple of layout test failures on Chromium bots.

Committed r108607: <http://trac.webkit.org/changeset/108607>
Comment 29 Hajime Morrita 2012-02-22 22:05:04 PST
(In reply to comment #27)
> Reverted r108602 for reason:
> 
> Caused a couple of layout test failures on Chromium bots.
> 
> Committed r108607: <http://trac.webkit.org/changeset/108607>
Huuu. Confirmed.. canHaveChildren() is used at unexpected places.
Comment 30 Hajime Morrita 2012-02-23 15:45:29 PST
Created attachment 128574 [details]
Patch
Comment 31 Hajime Morrita 2012-02-23 16:04:27 PST
Hi Dimitri, could you take another look at this?
I touched RenderBlock::updateFirstLetter() a bit.
This should be harmless. But I'd like to have someone's glance before land.

Thanks,
Comment 32 Hajime Morrita 2012-02-23 17:32:50 PST
Created attachment 128615 [details]
Fixing windows build break
Comment 33 Dimitri Glazkov (Google) 2012-02-23 19:17:47 PST
Comment on attachment 128615 [details]
Fixing windows build break

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

> Source/WebCore/rendering/RenderBlock.cpp:5831
> +            && !(firstLetterBlock->isDeprecatedFlexibleBox()
> +                 && static_cast<RenderDeprecatedFlexibleBox*>(firstLetterBlock)->buttonText());

whoa, this is so specific and hard to understand. Perhaps a static inline helper with a descriptive name should go here?
Comment 34 Hajime Morrita 2012-02-23 20:25:46 PST
Created attachment 128640 [details]
Patch for landing
Comment 35 Hajime Morrita 2012-02-23 20:34:14 PST
(In reply to comment #33)
> 
> whoa, this is so specific and hard to understand. Perhaps a static inline helper with a descriptive name should go here?
Done. I feel that we should extract whole first-letter stuff into a separate class.
It's complicated enough to have its own class...
Comment 36 Hajime Morrita 2012-02-24 01:59:09 PST
Committed r108758: <http://trac.webkit.org/changeset/108758>