From: http://code.google.com/p/chromium/issues/detail?id=113167
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>
Created attachment 127566 [details] Patch
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 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 :)
Created attachment 127784 [details] Patch
(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 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 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"
Created attachment 127869 [details] Patch
(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.
Created attachment 127897 [details] Patch
Created attachment 127898 [details] Patch
Comment on attachment 127898 [details] Patch r=me, provided build is fixed.
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
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
Created attachment 127940 [details] Patch for landing
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
(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 :)
(In reply to comment #18) > > I think SVG hates you :) Hmm I hope there were just existing redness. But that wasn't true. Looking.
Created attachment 128122 [details] Patch for landing
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
These failure looks just freakiness. They run well on my machine. I'll land this manually tomorrow.
Created attachment 128333 [details] Patch for landing
Well, I was wrong again. asking cq+...
Comment on attachment 128333 [details] Patch for landing Clearing flags on attachment: 128333 Committed r108602: <http://trac.webkit.org/changeset/108602>
All reviewed patches have been landed. Closing bug.
Reverted r108602 for reason: Caused a couple of layout test failures on Chromium bots. Committed r108607: <http://trac.webkit.org/changeset/108607>
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
(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.
Created attachment 128574 [details] Patch
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,
Created attachment 128615 [details] Fixing windows build break
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?
Created attachment 128640 [details] Patch for landing
(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...
Committed r108758: <http://trac.webkit.org/changeset/108758>