Bug 78878 – Adding a ShadowRoot to image-backed element causes a crash

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

From: http://code.google.com/p/chromium/issues/detail?id=113167

[Hajime Morrita]

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>

[Hajime Morrita]

Created attachment 127566 [details]
Patch

[Hajime Morrita]

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.

[Dimitri Glazkov (Google)]

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 :)

[Hajime Morrita]

Created attachment 127784 [details]
Patch

[Hajime Morrita]

(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?

[WebKit Review Bot]

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

[Dimitri Glazkov (Google)]

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"

[Hajime Morrita]

Created attachment 127869 [details]
Patch

[Hajime Morrita]

(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.

[Hajime Morrita]

Created attachment 127897 [details]
Patch

[Hajime Morrita]

Created attachment 127898 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 127898 [details]
Patch

r=me, provided build is fixed.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 127940 [details]
Patch for landing

[WebKit Review Bot]

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

[Dimitri Glazkov (Google)]

(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 :)

[Hajime Morrita]

(In reply to comment #18)
> 
> I think SVG hates you :)
Hmm I hope there were just existing redness. But that wasn't true. Looking.

[Hajime Morrita]

Created attachment 128122 [details]
Patch for landing

[WebKit Review Bot]

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

[Hajime Morrita]

These failure looks just freakiness. They run well on my machine.
I'll land this manually tomorrow.

[Hajime Morrita]

Created attachment 128333 [details]
Patch for landing

[Hajime Morrita]

Well, I was wrong again. asking cq+...

[WebKit Review Bot]

Comment on attachment 128333 [details]
Patch for landing

Clearing flags on attachment: 128333

Committed r108602: <http://trac.webkit.org/changeset/108602>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

Reverted r108602 for reason:

Caused a couple of layout test failures on Chromium bots.

Committed r108607: <http://trac.webkit.org/changeset/108607>

[Yuta Kitamura]

I reverted the change due to layout test breakage:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Finput-first-letter.html%20fast%2Fforms%2Finput-hit-test-border.html

[Hajime Morrita]

(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.

[Hajime Morrita]

Created attachment 128574 [details]
Patch

[Hajime Morrita]

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,

[Hajime Morrita]

Created attachment 128615 [details]
Fixing windows build break

[Dimitri Glazkov (Google)]

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?

[Hajime Morrita]

Created attachment 128640 [details]
Patch for landing

[Hajime Morrita]

(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...

[Hajime Morrita]

Committed r108758: <http://trac.webkit.org/changeset/108758>