In EFL MiniBrowser, Youtube site crashes when loading is finished.
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.
Created attachment 224862 [details] Patch
Created attachment 224863 [details] Patch
Created attachment 224864 [details] Patch
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?
Comment on attachment 224864 [details] Patch Patch is way too mysterious.
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.
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 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?
(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)