Bug 77772

Created attachment 127043 [details] Patch

Comment on attachment 127043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127043&action=review > Source/WebCore/rendering/style/RenderStyle.h:1443 > + bool isDisplayInlineType(EDisplay display) const { return display == INLINE || isDisplayReplacedType(display); } Should the methods that take EDisplay be private?

Created attachment 127082 [details] Patch

Comment on attachment 127082 [details] Patch r=me

Comment on attachment 127082 [details] Patch Clearing flags on attachment: 127082 Committed r109014: <http://trac.webkit.org/changeset/109014>

All reviewed patches have been landed. Closing bug.

Rolled out due to causing crashes. Will commit later with fixes for the crashes.

I've started looking at this. For my own reference, the following bug has a test case with a crash. http://code.google.com/p/chromium/issues/detail?id=118662 I didn't get a crash in a release build and in debug, I'm hitting an assert. I'll see if the assert is related and try an ASAN build.

The debug assert seems real. I don't get it when I switch the test to use -webkit-inline-box. I'll investigate tomorrow.

Created attachment 156966 [details] crash reduction 1

Created attachment 156967 [details] crash reduction 2

I welcome a fresh set of eyes looking at this. Attached two reductions of the crash from crbug.com/118662. They get slightly different stacks. When I looked into this before, the issue was with the generated RenderInline container inside the block inside the flexbox. For deprecated flexboxes, the format block command looks for the next line-break and does the wrong thing, but doesn't seem to corrupt the tree. For new flexboxes, with the same rendertree (except for the flex container) it somehow corrupts the render tree. Here are some of my render tree dumps from the first crash reduction right before and in the middle of the formatBlock command. You can see that we end up with a different rendertree for the new flexboxes. I could never pinpoint why. FLEXBOX: RenderView 0x7fffeb723a98 #document 0x7ffff7efe600 RenderBlock 0x7ffff7ed2ef8 HTML 0x7fffeb718e00 RenderBody 0x7ffff7ed22b8 BODY 0x7fffeb718080 RenderBlock (anonymous) 0x7fffea446558 RenderText 0x7fffeab72d08 #text 0x7fffea44d2a0 "This test passes if it doesn't crash.\n\n" RenderBlock 0x7fffea446478 DIV 0x7fffeb718180 RenderInline 0x7fffea403378 A 0x7fffea4031b0 * RenderImage 0x7fffeb708558 IMG 0x7fffeb708000 RenderFlexibleBox 0x7fffea446c58 DIV 0x7fffe6f56780 STYLE=display: -webkit-inline-flexbox RenderBlock 0x7fffea446d38 PRE 0x7fffe6f56800 RenderInline (generated) 0x7fffea3f0528 RenderText 0x7fffea44a3d8 #text 0x7fffe6f66540 "AA؀" RenderText 0x7fffea44a338 #text 0x7fffe6f66540 "AA؀" RenderView 0x7fffeb723a98 #document 0x7ffff7efe600 RenderBlock 0x7ffff7ed2ef8 HTML 0x7fffeb718e00 RenderBody 0x7ffff7ed22b8 BODY 0x7fffeb718080 RenderBlock (anonymous) 0x7fffea446558 RenderText 0x7fffeab72d08 #text 0x7fffea44d2a0 "This test passes if it doesn't crash.\n\n" RenderBlock 0x7fffea446478 DIV 0x7fffeb718180 RenderBlock 0x7fffea446a98 H1 0x7fffeaaea600 RenderInline 0x7fffe6e0aa38 A 0x7fffe6e0aab0 RenderImage 0x7fffe6e01618 IMG 0x7fffe6e01540 RenderBlock (anonymous) 0x7fffea446b78 RenderInline 0x7fffea403378 A 0x7fffea4031b0 RenderFlexibleBox 0x7fffea446c58 DIV 0x7fffe6f56780 STYLE=display: -webkit-inline-flexbox RenderBlock 0x7fffea446d38 PRE 0x7fffe6f56800 * RenderText 0x7fffe6c872e8 #text 0x7fffe6f66ee0 "AA" RenderText 0x7fffea44a338 #text 0x7fffe6f66540 "؀" DEPRECATED: RenderView 0x7fffeb723a98 #document 0x7ffff7efe600 RenderBlock 0x7ffff7ed2ef8 HTML 0x7fffeb718e00 RenderBody 0x7ffff7ed22b8 BODY 0x7fffeb718700 RenderBlock (anonymous) 0x7ffff7ed2718 RenderText 0x7fffeab71c78 #text 0x7fffeb72cd20 "This test passes if it doesn't crash.\n\n" RenderBlock 0x7fffea3e6ef8 DIV 0x7fffeb718800 RenderInline 0x7fffea4032e8 A 0x7fffea403120 * RenderImage 0x7fffeb708318 IMG 0x7fffeb708180 RenderDeprecatedFlexibleBox 0x7fffea4641d8 DIV 0x7fffea46f580 STYLE=display: -webkit-inline-box RenderBlock 0x7fffea4642b8 PRE 0x7fffea46f600 RenderInline (generated) 0x7fffea465498 RenderText 0x7fffea4463d8 #text 0x7fffea46e150 "AA؀" RenderText 0x7fffea446338 #text 0x7fffea46e150 "AA؀" RenderView 0x7fffeb723a98 #document 0x7ffff7efe600 RenderBlock 0x7ffff7ed2ef8 HTML 0x7fffeb718e00 RenderBody 0x7ffff7ed22b8 BODY 0x7fffeb718700 RenderBlock (anonymous) 0x7ffff7ed2718 RenderText 0x7fffeab71c78 #text 0x7fffeb72cd20 "This test passes if it doesn't crash.\n\n" RenderBlock 0x7fffea3e6ef8 DIV 0x7fffeb718800 RenderBlock 0x7fffea464ef8 H1 0x7fffe740c000 RenderInline 0x7fffe723fd98 A 0x7fffe7248750 RenderImage 0x7fffea3f13d8 IMG 0x7fffea3f1300 RenderBlock (anonymous) 0x7fffea464018 RenderInline 0x7fffea4032e8 A 0x7fffea403120 RenderDeprecatedFlexibleBox 0x7fffea4641d8 DIV 0x7fffea46f580 STYLE=display: -webkit-inline-box RenderBlock 0x7fffea4642b8 PRE 0x7fffea46f600 RenderInline (generated) 0x7fffe7248ac8 RenderText 0x7fffe7256e78 #text 0x7fffea3f00e0 "AA" * RenderText 0x7fffea4463d8 #text 0x7fffea3f00e0 "AA" RenderText 0x7fffea446338 #text 0x7fffea46e150 "؀"

I'm still looking at this. Using crash2.html doesn't repro anymore, but crash1.html does. crash1.html hits the following: 0x00007ffff15af91a in WTF::StringImpl::operator[] (this=0x7fffe0c592d0, i=1) at ../../third_party/WebKit/Source/WTF/wtf/text/StringImpl.h:537 537 ASSERT(i < m_length); I'll look more tomorrow.

Using binary search, I've found that removing the call to child->setChildNeedsLayout(true) in RenderFlexibleBox::layoutAndPlaceChildren causes us to not crash, but fail lots of tests. I'm not sure why forcing a layout is causing us to crash. I'll look more tomorrow.

Created attachment 157635 [details] Patch

Comment on attachment 157635 [details] Patch Attachment 157635 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13471421

Comment on attachment 157635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157635&action=review Nice find! This might also be a significant perf improvement. > LayoutTests/css3/flexbox/inline-flex.html:1 > +<!DOCTYPE html> Nit: I'd prefer you just make this a regular check-layout.js test. You can use inline-blocks instead of text and assert the size/position of the inline flexbox. We should use reftests if we really have to test paint behavior or something like that. Up to you though. Not a big deal.

Comment on attachment 157635 [details] Patch Attachment 157635 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13460839 New failing tests: http/tests/security/mixedContent/filesystem-url-in-iframe.html

Created attachment 157648 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 157656 [details] Patch for landing

Comment on attachment 157656 [details] Patch for landing Clearing flags on attachment: 157656 Committed r125262: <http://trac.webkit.org/changeset/125262>

All reviewed patches have been landed. Closing bug.

Comment on attachment 157656 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=157656&action=review > LayoutTests/css3/flexbox/inline-flex-crash2.html:8 > + layoutTestController.dumpAsText(); Is the use of window.layoutTestController instead of window.testRunner in some strange way intended? It's currently causing failures on non-Chromium ports (which, AFAIK, still exports window.layoutTestController).

Ryosuke, it appears we still expose layoutTestController from Chromium. :( (In reply to comment #23) > (From update of attachment 157656 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157656&action=review > > > LayoutTests/css3/flexbox/inline-flex-crash2.html:8 > > + layoutTestController.dumpAsText(); > > Is the use of window.layoutTestController instead of window.testRunner in some strange way intended? It's currently causing failures on non-Chromium ports (which, AFAIK, still exports window.layoutTestController). Accidental.

(In reply to comment #24) > Ryosuke, it appears we still expose layoutTestController from Chromium. :( Yeah, we had to re-expose it until the security team makes the transition to use testRunner in various fuzzers they have :(

This patch breaks my build: ../../Source/WebCore/rendering/style/RenderStyle.h:1743:79: error: ‘INLINE_FLEX’ was not declared in this scope I believe this is missing an #if ENABLE(CSS3_FLEXBOX) around the usage of INLINE_FLEX.

(In reply to comment #26) > This patch breaks my build: > > ../../Source/WebCore/rendering/style/RenderStyle.h:1743:79: error: ‘INLINE_FLEX’ was not declared in this scope > > I believe this is missing an #if ENABLE(CSS3_FLEXBOX) around the usage of INLINE_FLEX. We should remove the defines for flexbox. We had put them in because all the names were changing and we didn't want people to ship with the old names. At this point the spec is definitely stable and our implementation is as well. It should just be small bug fixes and cross-browser interoperability testing at this point.

(In reply to comment #27) > (In reply to comment #26) > > This patch breaks my build: > > > > ../../Source/WebCore/rendering/style/RenderStyle.h:1743:79: error: ‘INLINE_FLEX’ was not declared in this scope > > > > I believe this is missing an #if ENABLE(CSS3_FLEXBOX) around the usage of INLINE_FLEX. > > We should remove the defines for flexbox. We had put them in because all the names were changing and we didn't want people to ship with the old names. At this point the spec is definitely stable and our implementation is as well. It should just be small bug fixes and cross-browser interoperability testing at this point. In the meantime, I'll add in the #if to fix the build.