Bug 148377 - [EFL] REGRESSION(r188828): All performance tests and almost all layout tests crash
Summary: [EFL] REGRESSION(r188828): All performance tests and almost all layout tests ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
: 148420 148421 (view as bug list)
Depends on:
Blocks: 143743
  Show dependency treegraph
 
Reported: 2015-08-24 03:58 PDT by Csaba Osztrogonác
Modified: 2015-08-26 00:36 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.58 KB, patch)
2015-08-25 06:10 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.49 KB, patch)
2015-08-25 19:03 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2015-08-24 03:58:28 PDT
https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/6629

error: Animation/balls.html
1   0x7fa009f39678
2   0x7fa005377eb0
3   0x420214 WTR::PlatformWebView::page()
4   0x4119b4 WTR::TestController::resetPreferencesToConsistentValues()
5   0x419921 WTR::TestController::initialize(int, char const**)
6   0x41a20a WTR::TestController::TestController(int, char const**)
7   0x40d0ab main
8   0x7fa005362ec5 __libc_start_main
9   0x40d107

- last known good revision: https://trac.webkit.org/changeset/188827
(however the testing failed with timeout during the test number 27)

- build failure between: r188828 - r188842

- first failing revision: https://trac.webkit.org/changeset/188843
Comment 1 Csaba Osztrogonác 2015-08-24 04:02:10 PDT
I think https://trac.webkit.org/changeset/188828 might be the culprit,
because it touches PlatformWebView code path in this revision interval.
Comment 2 Tim Horton 2015-08-24 10:13:10 PDT
Seems likely! I will look shortly.
Comment 3 Tim Horton 2015-08-24 10:58:22 PDT
It looks like we don't have a view yet when TestController::initialize tries to set some prefs (via resetPreferencesToConsistentValues).

This is OK on PLATFORM(COCOA) because we keep around a global WKWebViewConfiguration and use that when retrieving the WKPreferences. Maybe someone from EFL or GTK (who can actually test this) can do the same with the WKPageConfiguration?
Comment 4 Gyuyoung Kim 2015-08-25 01:00:19 PDT
(In reply to comment #3)
> It looks like we don't have a view yet when TestController::initialize tries
> to set some prefs (via resetPreferencesToConsistentValues).
> 
> This is OK on PLATFORM(COCOA) because we keep around a global
> WKWebViewConfiguration and use that when retrieving the WKPreferences. Maybe
> someone from EFL or GTK (who can actually test this) can do the same with
> the WKPageConfiguration?

I simply apply below fix in order to run layout test on EFL port.


--- a/Tools/WebKitTestRunner/TestController.cpp
+++ b/Tools/WebKitTestRunner/TestController.cpp
@@ -568,7 +568,11 @@ void TestController::ensureViewSupportsOptionsForTest(const TestInvocation& test
 void TestController::resetPreferencesToConsistentValues()
 {
     // Reset preferences
+#if PLATFORM(COCOA)
     WKPreferencesRef preferences = platformPreferences();
+#else
+    WKPreferencesRef preferences = WKPageGroupGetPreferences(m_pageGroup.get());
+#endif


EFL layout test begins to run again with this fix. However I'm not sure if this is correct fix for EFL and GTK. Tim, what do you think ?
Comment 5 Gyuyoung Kim 2015-08-25 06:05:53 PDT
*** Bug 148420 has been marked as a duplicate of this bug. ***
Comment 6 Gyuyoung Kim 2015-08-25 06:10:33 PDT
Created attachment 259840 [details]
Patch
Comment 7 Gyuyoung Kim 2015-08-25 06:11:32 PDT
I hope to fix EFL layout test and performance test as soon as possible. Tim, Ossy , please take a look this patch.
Comment 8 Anders Carlsson 2015-08-25 10:03:38 PDT
Comment on attachment 259840 [details]
Patch

Please implement platformPreferences() in TestControllerEfl.cpp instead of adding more #ifdefs.
Comment 9 Gyuyoung Kim 2015-08-25 19:03:52 PDT
Created attachment 259912 [details]
Patch
Comment 10 Gyuyoung Kim 2015-08-25 19:06:38 PDT
(In reply to comment #8)
> Comment on attachment 259840 [details]
> Patch
> 
> Please implement platformPreferences() in TestControllerEfl.cpp instead of
> adding more #ifdefs.

It seems GTK layout test also has been broken since r188828. Fix it too.
Comment 11 WebKit Commit Bot 2015-08-26 00:03:39 PDT
Comment on attachment 259912 [details]
Patch

Clearing flags on attachment: 259912

Committed r188948: <http://trac.webkit.org/changeset/188948>
Comment 12 WebKit Commit Bot 2015-08-26 00:03:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Csaba Osztrogonác 2015-08-26 00:16:23 PDT
Comment on attachment 259912 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:1609
> +#if !PLATFORM(EFL) && !PLATFORM(GTK) && !PLATFORM(COCOA)
>  WKPreferencesRef TestController::platformPreferences()
>  {
>      WKRetainPtr<WKPageConfigurationRef> configuration = adoptWK(WKPageCopyPageConfiguration(m_mainWebView->page())); 
>      return WKPageConfigurationGetPreferences(configuration.get());
>  }
> +#endif

Aren't these lines unused? As far as I know there isn't this kind 
of platform. WK2 platforms are only Mac and iOS (Cocoa), EFL, GTK.
Comment 14 Gyuyoung Kim 2015-08-26 00:19:55 PDT
Comment on attachment 259912 [details]
Patch

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

>> Tools/WebKitTestRunner/TestController.cpp:1609
>> +#endif
> 
> Aren't these lines unused? As far as I know there isn't this kind 
> of platform. WK2 platforms are only Mac and iOS (Cocoa), EFL, GTK.

If so, this is unused code. I didn't know it. I'm sorry.
Comment 15 Csaba Osztrogonác 2015-08-26 00:23:11 PDT
(In reply to comment #14)
> Comment on attachment 259912 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259912&action=review
> 
> >> Tools/WebKitTestRunner/TestController.cpp:1609
> >> +#endif
> > 
> > Aren't these lines unused? As far as I know there isn't this kind 
> > of platform. WK2 platforms are only Mac and iOS (Cocoa), EFL, GTK.
> 
> If so, this is unused code. I didn't know it. I'm sorry.

No problem, let me remove them in bug148467
Comment 16 Xabier Rodríguez Calvar 2015-08-26 00:36:54 PDT
*** Bug 148421 has been marked as a duplicate of this bug. ***