Bug 77772 – implement display: -webkit-inline-flex

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tony Chang]

Currently this behaves the same as -webkit-flexbox, but should behave like inline-block.

[Ojan Vafai]

Created attachment 127043 [details]
Patch

[Tony Chang]

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?

[Ojan Vafai]

Created attachment 127082 [details]
Patch

[Dave Hyatt]

Comment on attachment 127082 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 127082 [details]
Patch

Clearing flags on attachment: 127082

Committed r109014: <http://trac.webkit.org/changeset/109014>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

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

[Tony Chang]

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.

[Tony Chang]

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

[Ojan Vafai]

Created attachment 156966 [details]
crash reduction 1

[Ojan Vafai]

Created attachment 156967 [details]
crash reduction 2

[Ojan Vafai]

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 "؀"

[Tony Chang]

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.

[Tony Chang]

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.

[Tony Chang]

Created attachment 157635 [details]
Patch

[Build Bot]

Comment on attachment 157635 [details]
Patch

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

[Ojan Vafai]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Tony Chang]

Created attachment 157656 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 157656 [details]
Patch for landing

Clearing flags on attachment: 157656

Committed r125262: <http://trac.webkit.org/changeset/125262>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

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

[Ojan Vafai]

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.

[Ryosuke Niwa]

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

[Xan Lopez]

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.

[Ojan Vafai]

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

[Ojan Vafai]

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

[Ojan Vafai]

http://trac.webkit.org/changeset/125301