[EFL][DRT] ewk_view_paint_contents may trigger assertion failure

[Dominik Röttsches (drott)]

We're running into an assertion failure in
WebCore::FrameView::paintContents
ASSERTION FAILED: !needsLayout()

when running media/media-fragments/ tests.

ewk_view_paint resolves that by calling

    if (view->needsLayout())
        view->forceLayout();

So we should probably do that in ewk_view_paint_contents, too.

[Dominik Röttsches (drott)]

Created attachment 139185 [details]
Don't go into painting without layout

[Gyuyoung Kim]

Comment on attachment 139185 [details]
Don't go into painting without layout

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

> Source/WebKit/efl/ewk/ewk_view.cpp:2740
> +        view->forceLayout();

I'm wondering whether we should call forceLayout(). As you know, there is view->layout().

[Dominik Röttsches (drott)]

They are identical - forceLayout calls layout internally - and I don't see any deprecation warning in FrameView.h. I am using forceLayout for consistency with ewk_view_paint: forceLayout is used there. So, looks okay to you?

[Ryuan Choi]

(In reply to comment #3)
> They are identical - forceLayout calls layout internally - and I don't see any deprecation warning in FrameView.h. I am using forceLayout for consistency with ewk_view_paint: forceLayout is used there. So, looks okay to you?

I think that we should investigate this more.

If contents is dynamic, each tile may render different layout for ewk_view_pre_render.
ewk_view_pre_render make several tiles render on idle time.

Add jungjik in CCs.

[Gyuyoung Kim]

I think we should avoid unneeded layout operation. Because, layout cost is expensive.

[Dominik Röttsches (drott)]

Sure, but it's not unneeded here - since without a layout we run into the assertion failure, so clearly something is wrong. 
Jungjik, Ryuan - which direction do you think we should look to investigate this, any pointers? Which component should have done the layout before we actually get to ewk_view_paint_contents and run into the assertion failure?

[Dominik Röttsches (drott)]

The media fragment tests are constructed in a way that they quickly iterate through a number of fragment suffixes that they append to the media URL. After each test, they add elements to the DOM to report the result of the test case using .innerHTML which goes through the parser and ends up adding child elements to the DOM.

We hit the assertion failure because of a race condition. Idle-repainting of EFL fights with the scheduleRelayout() timer that is triggered by the addChild insertion.

HTMLConstructionSite::executeTask gets to
 task.parent->parserAddChild(task.child.get());

that triggers as a child note insertion notification in:
ChildNodeInsertionNotifier::notifyInsertedIntoDocument

which in turn triggers a
        view->scheduleRelayout();

in HTMLBodyElement::didNotifyDescendantInseretions.
which has the following interesting fixme:
    // FIXME: This call to scheduleRelayout should not be needed here.
    // But without it we hang during WebKit tests; need to fix that and remove this.

Now, if the EFL idle tiling repainting timer is triggered first, we arrive in ewk_view_paint_contents without having done a relayout. 

So, I would conclude, it's safe to do a layout here if needed to avoid this assertion failure.
  if (view->needsLayout())
      view->forceLayout();

Since a relayout would be done anyway if the scheduleRelayout timer would fire first before we get to idle.

[Dominik Röttsches (drott)]

Created attachment 141154 [details]
Updated, rebased patch.

Updated changelog to include reason for why assertion is hit.

[Gustavo Noronha (kov)]

Comment on attachment 141154 [details]
Updated, rebased patch.

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

> Source/WebKit/efl/ewk/ewk_view.cpp:2810
> +    if (view->needsLayout())
> +        view->forceLayout();

To me this looks like using too strong a force. Why don't you call view->updateLayoutAndStyleIfNeededRecursive() unconditionally instead?

[Dominik Röttsches (drott)]

Created attachment 141165 [details]
Updated, review comment addressed.

Using less force, Luke. Thanks, kov!

[Dominik Röttsches (drott)]

Comment on attachment 141165 [details]
Updated, review comment addressed.

Clearing flags, forgot something.

[Dominik Röttsches (drott)]

Created attachment 141170 [details]
Updated again.

[Dominik Röttsches (drott)]

Thought about it again - updateLayoutAndStyleIfNeededRecursive() does nothing else than if(needsLayout()) layout() as well. We don't actually need to update style, I believe. We just need to protect against the race condition where the idle timer tries to repaint first before layoutTimerFired is called, so this conditional layout() call should be enough here. To make it sound less forceful, I call the layout() function, which is equivalent.

[Dominik Röttsches (drott)]

Created attachment 141174 [details]
Updated patch

Following reviewer's advice here and finally using updateLayoutAndStyleIfNeededRecursive() - thanks.

[WebKit Review Bot]

Comment on attachment 141174 [details]
Updated patch

Clearing flags on attachment: 141174

Committed r116656: <http://trac.webkit.org/changeset/116656>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.