[CSS Regions][Mac] fast/regions/full-screen-video-from-region.html hits an assertion in RenderFlowThread::removeRenderBoxRegionInfo

[Ryosuke Niwa]

The test added by http://trac.webkit.org/changeset/138755 is hitting an assertion:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=full-screen-video-from-region.html

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r138769%20(4286)/results.html

0   com.apple.WebCore             	0x000000010f55615b WebCore::RenderFlowThread::removeRenderBoxRegionInfo(WebCore::RenderBox*) + 459 (RenderFlowThread.cpp:449)
1   com.apple.WebCore             	0x000000010f555f63 WebCore::RenderFlowThread::removeFlowChildInfo(WebCore::RenderObject*) + 67 (RenderFlowThread.cpp:90)
2   com.apple.WebCore             	0x000000010f61b0b2 WebCore::RenderObject::removeFromRenderFlowThreadRecursive(WebCore::RenderFlowThread*) + 130 (RenderObject.cpp:2496)
3   com.apple.WebCore             	0x000000010f61b093 WebCore::RenderObject::removeFromRenderFlowThreadRecursive(WebCore::RenderFlowThread*) + 99 (RenderObject.cpp:2493)
4   com.apple.WebCore             	0x000000010f61b093 WebCore::RenderObject::removeFromRenderFlowThreadRecursive(WebCore::RenderFlowThread*) + 99 (RenderObject.cpp:2493)
5   com.apple.WebCore             	0x000000010f61b093 WebCore::RenderObject::removeFromRenderFlowThreadRecursive(WebCore::RenderFlowThread*) + 99 (RenderObject.cpp:2493)
6   com.apple.WebCore             	0x000000010f61b028 WebCore::RenderObject::removeFromRenderFlowThread() + 120 (RenderObject.cpp:2488)
7   com.apple.WebCore             	0x000000010f61af52 WebCore::RenderObject::willBeRemovedFromTree() + 354 (RenderObject.cpp:2470)
8   com.apple.WebCore             	0x000000010f61f3c5 WebCore::RenderObjectChildList::removeChildNode(WebCore::RenderObject*, WebCore::RenderObject*, bool) + 389 (RenderObjectChildList.cpp:92)
9   com.apple.WebCore             	0x000000010f60be54 WebCore::RenderObject::removeChild(WebCore::RenderObject*) + 164 (RenderObject.cpp:343)
10  com.apple.WebCore             	0x000000010f4653bb WebCore::RenderBlock::removeChild(WebCore::RenderObject*) + 1019 (RenderBlock.cpp:1195)
11  com.apple.WebCore             	0x000000010f56a966 WebCore::RenderObject::remove() + 70 (RenderObject.h:936)
12  com.apple.WebCore             	0x000000010f56a259 WebCore::RenderFullScreen::wrapRenderer(WebCore::RenderObject*, WebCore::RenderObject*, WebCore::Document*) + 425 (RenderFullScreen.cpp:132)
13  com.apple.WebCore             	0x000000010e600496 WebCore::Document::webkitWillEnterFullScreenForElement(WebCore::Element*) + 646 (Document.cpp:5212)
14  com.apple.WebKit              	0x000000010dc77ca6 -[WebKitFullScreenListener webkitWillEnterFullScreen] + 118 (WebKitFullScreenListener.mm:49)
15  DumpRenderTree                	0x000000010c888477 -[UIDelegate enterFullScreenWithListener:] + 39 (UIDelegate.mm:261)
16  com.apple.Foundation          	0x00007fff86ffbdb5 __NSFireDelayedPerform + 358
17  com.apple.CoreFoundation      	0x00007fff8a050da4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
18  com.apple.CoreFoundation      	0x00007fff8a0508bd __CFRunLoopDoTimer + 557
19  com.apple.CoreFoundation      	0x00007fff8a036099 __CFRunLoopRun + 1513
20  com.apple.CoreFoundation      	0x00007fff8a0356b2 CFRunLoopRunSpecific + 290
21  com.apple.Foundation          	0x00007fff8702389e -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 268
22  DumpRenderTree                	0x000000010c84b839 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 5017 (DumpRenderTree.mm:1381)
23  DumpRenderTree                	0x000000010c84a42a runTestingServerLoop() + 282 (DumpRenderTree.mm:846)
24  DumpRenderTree                	0x000000010c849cf7 dumpRenderTree(int, char const**) + 391 (DumpRenderTree.mm:893)
25  DumpRenderTree                	0x000000010c84c029 main + 105 (DumpRenderTree.mm:931)

[Ryosuke Niwa]

Added a test expectation in http://trac.webkit.org/changeset/138772.

[Jessie Berlin]

<rdar://problem/13114899>

[Jessie Berlin]

Rather(In reply to comment #2)
> <rdar://problem/13114899>

Rather, <rdar://problem/13003424>

[Andrei Bucur]

Created attachment 185794 [details]
Patch

[Zoltan Horvath]

Comment on attachment 185794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185794&action=review

It's cool that you figured this out! Just 2 small nits below.

> Source/WebCore/ChangeLog:27
> +        Tests: No new tests. Repaired fast/regions/full-screen-video-from-region.html

Where is the fix?

> LayoutTests/platform/qt/TestExpectations:-2557
> -webkit.org/b/106095 fast/regions/full-screen-video-from-region.html

This is tracked in bug #106095 and it times out because Qt hasn't been implemented webkitRequestFullscreen() (bug #92078) yet.

[WebKit Review Bot]

Comment on attachment 185794 [details]
Patch

Attachment 185794 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16252504

New failing tests:
fast/forms/datalist/update-range-with-datalist.html
fast/layers/no-clipping-overflow-hidden-added-after-transform.html
inspector-protocol/heap-profiler/heap-snapshot-with-detached-dom-tree.html
fast/loader/text-document-wrapping.html
fast/loader/javascript-url-in-object.html
fast/layers/no-clipping-overflow-hidden-added-after-transition.html
fast/layers/no-clipping-overflow-hidden-hardware-acceleration.html

[Andrei Bucur]

(In reply to comment #5)
> (From update of attachment 185794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185794&action=review
> 
> It's cool that you figured this out! Just 2 small nits below.
> 
> > Source/WebCore/ChangeLog:27
> > +        Tests: No new tests. Repaired fast/regions/full-screen-video-from-region.html
> 
> Where is the fix?

I meant I've fixed the assertion in debug mode :).

> 
> > LayoutTests/platform/qt/TestExpectations:-2557
> > -webkit.org/b/106095 fast/regions/full-screen-video-from-region.html
> 
> This is tracked in bug #106095 and it times out because Qt hasn't been implemented webkitRequestFullscreen() (bug #92078) yet.

Oh, I thought that was added for the same reason. I'll fix it in the next version/before committing.

Thanks for the review!

[Andrei Bucur]

Created attachment 185848 [details]
Patch retry

Retry the old patch to check for bad bots.

[Andrei Bucur]

Created attachment 185979 [details]
Patch with Zoltan's feedback

[Tony Chang]

Comment on attachment 185979 [details]
Patch with Zoltan's feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=185979&action=review

> Source/WebCore/ChangeLog:11
> +        When the region chain is invalidate the information is lost so we need to return true, even for the

Grammar nit: "When the region chain is invalidated, the information is lost so ..."

> Source/WebCore/ChangeLog:18
> +        The second problem is RenderMedia not inheriting for RenderBlock. The logic of child relayout doesn't apply

Nit: for -> from

> Source/WebCore/rendering/RenderMedia.cpp:76
> +            controlsRenderer->setNeedsLayout(true, MarkOnlyThis);

Rather than calling setNeedsLayout, I would probably do something like,
bool controlsNeedLayout = controlsRenderer->needsLayout();
if (inRenderFlowThread()) {
    ...
        controlsNeedLayout = true;
}

if (newSize == oldSize && !controlsNeedLayout)
    return;

[Andrei Bucur]

Created attachment 188513 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 188513 [details]
Patch for landing

Clearing flags on attachment: 188513

Committed r142982: <http://trac.webkit.org/changeset/142982>

[Mihai Balan]

Closing as the fix landed in r142982.