Bug 129147 – Fix crash in Youtube site when loading is finished.

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Joonghun Park]

In EFL MiniBrowser, Youtube site crashes when loading is finished.

[Joonghun Park]

Reproduce paths are as below:

common precondition: 
In Tools/MiniBrowser/efl/main.c, comment out a statement below.
    //ewk_settings_prefferred_minimum_contents_width_set(settings, 0);

1. execute EFL MiniBrowser with options like below for using fixed layout
    ./MiniBrowser -s 720x1280 -r 1.0 -T 1 -L 1 http://www.youtube.com

2. In WebChromeClient::dispatchViewportPropertiesDidChange(),
    revise if(m_page->useFixedLayout()) - > if(m_page->useFixedLayout()) for the test 
    is the equivalence because the revision makes it as if it uses fixed layout and it    leads to the same code flow as 1.

[Joonghun Park]

Created attachment 224862 [details]
Patch

[Joonghun Park]

Created attachment 224863 [details]
Patch

[Joonghun Park]

Created attachment 224864 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 224864 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1361
> +#if PLATFORM(EFL)
> +        view->scheduleRelayout();
> +#else
>          view->forceLayout();
> +#endif

This doesn't look like it can be right. Why is Efl different?

[Simon Fraser (smfr)]

Comment on attachment 224864 [details]
Patch

Patch is way too mysterious.

[Joonghun Park]

As far as I know, currently Only EFL browser uses fixed layout, and other browser is not.
So when using fixed layout, in WebChromeClient::dispatchViewportPropertiesDidChange()  
the code flow leads to the m_page->sendViewportAttributesChanged();
while other browsers not using fixed layout leads to return; 
becuase of the statement if (!m_page->useFixedLayout()) return;

That's why I made this patch, but if it is not reasonable, I will withdraw it.

[Simon Fraser (smfr)]

If that's true then:
1. Your channgelog should have explained that
2. Your patch should do if (usingFixedLayout)... rather than a #if PLATFORM

BTW, iOS uses fixed layout and doesn't crash.

[Joonghun Park]

The crash's cause is as below:
1. In ViewportStyleResolver::resolve(), m_document->updateViewportArguments() is called and in there, forcelayout() is called sequentially, and it leads to the deletion of the ViewportStyleResolver itself.
2. This time, the remained statement in ViewportStyleResolver::resolve(),
i.e. m_propertySet = 0;
is called. It means the access to already deleted ViewportStyleResolver instance's member variable.

It means that with some conditions, the log is displayed as below
When I put the logs in ViewportStyleResolver::resolve()'s start and end part, 
respectively. I would be glad if I can get to know what the condition is exactly.

ViewportStyleResolver::resolve(1) start 
ViewportStyleResolver::resolve(2) start
ViewportStyleResolver::resolve(2) end
ViewportStyleResolver::resolve(1) end

The above sequence cause the deletion of the ViewportStyleResolver even it's member function's execution is not completed yet.


FYI, When I executed EFL MiniBrowser with -r(resolution) 1.0 and iteration try, I couldn't see the webprocess crash sometimes, but more than 2.0 almost always(-r>=2.0). If it's not occured in iOS, then it will be needed to know the difference of efl and iOS in code flow from the statement, 
m_document->updateViewportArguments() in ViewportStyleResolver::resolve().

In the next changeLog and patch, I'll reflect the comment Mr.Simon Fraser have left.


BTW, I have a curiosity. In the first patch, I just only used scheduleRelayout() not with #if PLATFORM(...), and I faced an build error in mac and mac-wk2. Why is it?

[alan]

(In reply to comment #8)
> If that's true then:
> 1. Your channgelog should have explained that
> 2. Your patch should do if (usingFixedLayout)... rather than a #if PLATFORM
> 
> BTW, iOS uses fixed layout and doesn't crash.
The difference is that iOS takes a different codepath at WebChromeClient::dispatchViewportPropertiesDidChange. 
By taking a quick look at the code, it seems EFL ends up segfaulting due to calling layout recursively. setFixedLayoutSize() -> forceLayout() -> layout() -> styleResolverChanged() -> updateActiveStyleSheets() -> appendAuthorStyleSheets() -> viewportStyleResolver()->resolve() -> m_document->updateViewportArguments() -> dispatchViewportPropertiesDidChange() -> sendViewportAttributesChanged() -> setFixedLayoutSize()

Provided we are inside this loop, calling scheduleRelayout() at this point is a noop as scheduling is disabled for the FrameView. (as a side node, bug 52309 introduced this API and I am not sure if the caller expects sync layout, if it does, i'd branch out and make a new function with this async behavior.)  
 

> BTW, I have a curiosity. In the first patch, I just only used scheduleRelayout() not with #if PLATFORM(...), and I faced an build error in mac and mac-wk2. Why is it?
scheduleRelayout needs to be exported out if you intend to use it in WebKit2. (WebCore.exp.in)