Bug 77772

Summary: implement display: -webkit-inline-flex
Product: WebKit Reporter: Tony Chang <tony>
Component: Layout and RenderingAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eoconnor, eric, hyatt, ojan, rniwa, tony, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 81551, 93718    
Bug Blocks: 62048    
Attachments:
Description Flags
Patch
none
Patch
none
crash reduction 1
none
crash reduction 2
none
Patch
none
Archive of layout-test-results from gce-cr-linux-06
none
Patch for landing none

Description Tony Chang 2012-02-03 13:34:50 PST
Currently this behaves the same as -webkit-flexbox, but should behave like inline-block.
Comment 1 Ojan Vafai 2012-02-14 14:22:56 PST
Created attachment 127043 [details]
Patch
Comment 2 Tony Chang 2012-02-14 14:40:16 PST
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?
Comment 3 Ojan Vafai 2012-02-14 17:07:21 PST
Created attachment 127082 [details]
Patch
Comment 4 Dave Hyatt 2012-02-27 11:59:32 PST
Comment on attachment 127082 [details]
Patch

r=me
Comment 5 WebKit Review Bot 2012-02-27 12:33:44 PST
Comment on attachment 127082 [details]
Patch

Clearing flags on attachment: 127082

Committed r109014: <http://trac.webkit.org/changeset/109014>
Comment 6 WebKit Review Bot 2012-02-27 12:33:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Ojan Vafai 2012-03-19 13:49:26 PDT
Rolled out due to causing crashes. Will commit later with fixes for the crashes.
Comment 8 Tony Chang 2012-08-07 00:25:41 PDT
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.
Comment 9 Tony Chang 2012-08-07 00:41:38 PDT
The debug assert seems real. I don't get it when I switch the test to use -webkit-inline-box.  I'll investigate tomorrow.
Comment 10 Ojan Vafai 2012-08-07 11:48:00 PDT
Created attachment 156966 [details]
crash reduction 1
Comment 11 Ojan Vafai 2012-08-07 11:48:18 PDT
Created attachment 156967 [details]
crash reduction 2
Comment 12 Ojan Vafai 2012-08-07 11:54:45 PDT
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 "؀"
Comment 13 Tony Chang 2012-08-08 00:14:30 PDT
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.
Comment 14 Tony Chang 2012-08-09 00:42:02 PDT
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.
Comment 15 Tony Chang 2012-08-09 21:54:22 PDT
Created attachment 157635 [details]
Patch
Comment 16 Build Bot 2012-08-09 22:31:19 PDT
Comment on attachment 157635 [details]
Patch

Attachment 157635 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13471421
Comment 17 Ojan Vafai 2012-08-09 23:54:20 PDT
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 18 WebKit Review Bot 2012-08-10 00:02:31 PDT
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
Comment 19 WebKit Review Bot 2012-08-10 00:02:36 PDT
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
Comment 20 Tony Chang 2012-08-10 00:42:47 PDT
Created attachment 157656 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-08-10 02:06:36 PDT
Comment on attachment 157656 [details]
Patch for landing

Clearing flags on attachment: 157656

Committed r125262: <http://trac.webkit.org/changeset/125262>
Comment 22 WebKit Review Bot 2012-08-10 02:06:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Zan Dobersek 2012-08-10 10:25:57 PDT
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).
Comment 24 Ojan Vafai 2012-08-10 10:28:36 PDT
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.
Comment 25 Ryosuke Niwa 2012-08-10 10:50:14 PDT
(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 :(
Comment 26 Xan Lopez 2012-08-10 11:12:04 PDT
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.
Comment 27 Ojan Vafai 2012-08-10 11:15:52 PDT
(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.
Comment 28 Ojan Vafai 2012-08-10 11:16:18 PDT
(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.
Comment 29 Ojan Vafai 2012-08-10 11:21:33 PDT
http://trac.webkit.org/changeset/125301