RESOLVED FIXED 91719
[EFL] EFL port should use XDG paths 
https://bugs.webkit.org/show_bug.cgi?id=91719
Summary [EFL] EFL port should use XDG paths
Zoltan Nyul
Reported Thursday, July 19, 2012 8:18:48 AM UTC
The testrunner script is may be running multiple processes in parallel, and it makes appcache tests fail if they are using the same directory. I modified the EFL's LayoutTestController to use the DUMPRENDERTREE_TEMP for application cache directory because it's different for each process. It fixes these tests: http/tests/appcache/access-via-redirect.php http/tests/appcache/auth.html http/tests/appcache/crash-when-navigating-away-then-back.html http/tests/appcache/credential-url.html http/tests/appcache/cyrillic-uri.html http/tests/appcache/different-scheme.html http/tests/appcache/document-write-html-element-2.html http/tests/appcache/empty-manifest.html http/tests/appcache/fail-on-update-2.html http/tests/appcache/fail-on-update.html http/tests/appcache/foreign-fallback.html http/tests/appcache/foreign-iframe-main.html http/tests/appcache/idempotent-update.html http/tests/appcache/interrupted-update.html http/tests/appcache/local-content.html http/tests/appcache/main-resource-hash.html http/tests/appcache/main-resource-redirect.html http/tests/appcache/manifest-containing-itself.html http/tests/appcache/manifest-parsing.html http/tests/appcache/manifest-with-empty-file.html http/tests/appcache/non-html.xhtml http/tests/appcache/offline-access.html http/tests/appcache/online-fallback-layering.html http/tests/appcache/online-whitelist.html http/tests/appcache/progress-counter.html http/tests/appcache/reload.html http/tests/appcache/remove-cache.html http/tests/appcache/simple.html http/tests/appcache/top-frame-1.html http/tests/appcache/top-frame-2.html http/tests/appcache/top-frame-3.html http/tests/appcache/top-frame-4.html http/tests/appcache/update-cache.html http/tests/appcache/whitelist-wildcard.html http/tests/appcache/wrong-content-type.html http/tests/appcache/xhr-foreign-resource.html
Attachments
patch (4.23 KB, patch)
2012-07-19 00:30 PDT, Zoltan Nyul 		no flags
DetailsFormatted DiffDiff
patch (11.40 KB, patch)
2012-07-26 00:48 PDT, Zoltan Nyul 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Zoltan Nyul
Comment 1 Thursday, July 19, 2012 8:30:17 AM UTC
Created attachment 153195 [details] patch
Chris Dumez
Comment 2 Thursday, July 19, 2012 8:48:07 AM UTC
Comment on attachment 153195 [details] patch LGTM. Good patch!
Ryuan Choi
Comment 3 Thursday, July 19, 2012 8:50:03 AM UTC
LGTM, too.
WebKit Review Bot
Comment 4 Thursday, July 19, 2012 10:59:51 AM UTC
Comment on attachment 153195 [details] patch Clearing flags on attachment: 153195 Committed r123085: <http://trac.webkit.org/changeset/123085>
WebKit Review Bot
Comment 5 Thursday, July 19, 2012 10:59:56 AM UTC
All reviewed patches have been landed. Closing bug.
Dominik Röttsches (drott)
Comment 6 Friday, July 20, 2012 10:41:37 AM UTC
Could you try $ run-webkit-tests -2 --efl --debug i.e. using this patch on a debug build? I get assertion failures: STDERR: ASSERTION FAILED: m_cacheDirectory.isNull() STDERR: /fast/dominik/dev/WebKitGit_EFL/Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp(366) : void WebCore::ApplicationCacheStorage::setCacheDirectory(const WTF::String&) STDERR: 1 0x7f572b3fe393 WebCore::ApplicationCacheStorage::setCacheDirectory(WTF::String const&) STDERR: 2 0x7f56d7f7bb19 WTR::LayoutTestController::platformInitialize() STDERR: 3 0x7f56d7f68e58 WTR::LayoutTestController::LayoutTestController() STDERR: 4 0x7f56d7f68d4c WTR::LayoutTestController::create()
Chris Dumez
Comment 7 Friday, July 20, 2012 10:57:28 AM UTC
(In reply to comment #6) > Could you try > $ run-webkit-tests -2 --efl --debug > i.e. using this patch on a debug build? I get assertion failures: > > STDERR: ASSERTION FAILED: m_cacheDirectory.isNull() > STDERR: /fast/dominik/dev/WebKitGit_EFL/Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp(366) : void WebCore::ApplicationCacheStorage::setCacheDirectory(const WTF::String&) > STDERR: 1 0x7f572b3fe393 WebCore::ApplicationCacheStorage::setCacheDirectory(WTF::String const&) > STDERR: 2 0x7f56d7f7bb19 WTR::LayoutTestController::platformInitialize() > STDERR: 3 0x7f56d7f68e58 WTR::LayoutTestController::LayoutTestController() > STDERR: 4 0x7f56d7f68d4c WTR::LayoutTestController::create() As I explained to Zoltan yesterday, the application cache directory can only be set once. If you try to set it more than once on a debug build, you will hit this ASSERT. The weird thing is that I saw Zoltan running all the tests and I thought he had debug enabled.
Chris Dumez
Comment 8 Friday, July 20, 2012 11:11:14 AM UTC
Comment on attachment 153195 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=153195&action=review > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:43 > + We need to return an empty String if WebCore::cacheStorage().cacheDirectory() is non-empty (else case). Otherwise, the appcache directory gets set twice and we crash in debug mode.
Gyuyoung Kim
Comment 9 Friday, July 20, 2012 11:29:23 AM UTC
Committed r123200: <http://trac.webkit.org/changeset/123200>
Chris Dumez
Comment 10 Friday, July 20, 2012 2:10:49 PM UTC
The patch has been rolled out, reopening the bug.
Chris Dumez
Comment 11 Monday, July 23, 2012 4:47:32 PM UTC
As discussed, the port port should use XDG paths (cache, config, data) instead of hardcoding $HOME+".webkit/xxx" everywhere. EFREET library should probably be used. We can then override the XDG_CACHE_HOME variable in the test runner to make sure each process has its own cache path.
Raphael Kubo da Costa (:rakuco)
Comment 12 Monday, July 23, 2012 8:24:12 PM UTC
Sorry for chiming in so late in the game. I'm all in for portability changes :-)
Zoltan Nyul
Comment 13 Thursday, July 26, 2012 8:48:28 AM UTC
Created attachment 154564 [details] patch I've modified the patch to use xdg paths instead of $HOME+".webkit/xxx and i've also fixed the assert.
Chris Dumez
Comment 14 Thursday, July 26, 2012 9:03:23 AM UTC
Comment on attachment 154564 [details] patch LGTM.
Gyuyoung Kim
Comment 15 Thursday, July 26, 2012 9:17:10 AM UTC
Comment on attachment 154564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=154564&action=review > Source/WebKit/efl/ewk/ewk_main.cpp:163 > + String localStorageDirectory = String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl/LocalStorage"; I would recommend to use makeString(). > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:37 > + return String::fromUTF8(efreet_cache_home_get()) + "/WebKitEfl/Applications"; ditto.
Chris Dumez
Comment 16 Thursday, July 26, 2012 9:20:07 AM UTC
Comment on attachment 154564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=154564&action=review >> Source/WebKit/efl/ewk/ewk_main.cpp:163 >> + String localStorageDirectory = String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl/LocalStorage"; > > I would recommend to use makeString(). MakeString is deprecated as it brings no performance improvement over + operator: https://bugs.webkit.org/show_bug.cgi?id=62527#c15
Gyuyoung Kim
Comment 17 Thursday, July 26, 2012 9:22:13 AM UTC
(In reply to comment #16) > (From update of attachment 154564 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154564&action=review > > >> Source/WebKit/efl/ewk/ewk_main.cpp:163 > >> + String localStorageDirectory = String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl/LocalStorage"; > > > > I would recommend to use makeString(). > > MakeString is deprecated as it brings no performance improvement over + operator: https://bugs.webkit.org/show_bug.cgi?id=62527#c15 Oops. makeString is deprecated. Looks fine now.
WebKit Review Bot
Comment 18 Thursday, July 26, 2012 1:01:59 PM UTC
Comment on attachment 154564 [details] patch Clearing flags on attachment: 154564 Committed r123731: <http://trac.webkit.org/changeset/123731>
WebKit Review Bot
Comment 19 Thursday, July 26, 2012 1:02:06 PM UTC
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.