Separate WebKit2 instances use the same local storage

[Szilard Ledan]

WK1 uses separate temporary directories while WK2 does not. Similar to WK1, WK2 should also deal with own temporary directories.

[Szilard Ledan]

Created attachment 148807 [details]
patch

[WebKit Review Bot]

Attachment 148807 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/WebKitTestRunner/qt/TestControllerQt.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/qt/TestControllerQt.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/WebKitTestRunner/qt/TestControllerQt.cpp:56:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/WebKitTestRunner/qt/TestControllerQt.cpp:56:  Use 0 instead of NULL.  [readability/null] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit2/Shared/qt/QtDefaultDataLocation.h:38:  The parameter name "dataLocation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/qt/QtDefaultDataLocation.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Comment on attachment 148807 [details]
patch

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

The idea is good but you need to polish the patch. Please listen to the style bot, and use check-webkit-style next time. (I did not mentioned style issues in the review.)

> Source/WebKit2/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

Normally we summarize the change in a few sentences here.

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.cpp:49
> +    QString& s_dataLocation = globalDataLocation();

It's no more a static, so please remove the misleading s_ prefix.

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.cpp:65
> +    QString& s_dataLocation = globalDataLocation();

ditto

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.h:36
> +QString& s_dataLocation();

What is this??? You don't use it anywhere.

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.h:38
> +QWEBKIT_EXPORT void setDataLocation(QString dataLocation);

I would put this into global namespace. Maybe we could find a more expressing name, I'm not sure what could it be. setPersistentDataLocation maybe. (And than rename defaultDataLocation to persistentDataLocation and the files as well, in a following cleanup patch.)

> Tools/WebKitTestRunner/qt/TestControllerQt.cpp:57
> +    if (getenv("DUMPRENDERTREE_TEMP") != NULL)
> +        WebKit::setDataLocation(getenv("DUMPRENDERTREE_TEMP"));

remove != NULL, and avoid calling qgetenv twice, please.

[Balazs Kelemen]

> 
> > Source/WebKit2/Shared/qt/QtDefaultDataLocation.h:38
> > +QWEBKIT_EXPORT void setDataLocation(QString dataLocation);
> 

And this needs a comment saying it is private api for WebKitTestRunner.

[Szilard Ledan]

Created attachment 148997 [details]
patch

Thanks your advice. I fixed the patch.

[Balazs Kelemen]

Comment on attachment 148997 [details]
patch

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

I would like to see the change first, later we can do the renaming. So I would like you to upload a patch that contains only the behavioral change.

> Source/WebKit2/ChangeLog:5
> +        [Qt] Added a new API in the WebKit namespace
> +        for the WebKitTestRunner, which sets the DataDirectory.
> +        https://bugs.webkit.org/show_bug.cgi?id=89666

There should be the title of the bug, as prepare-changelog do it for you. You should not edit that part manually :)

> Source/WebKit2/ChangeLog:13
> +        I have renamed defaultDataLocation to persistentDataLocation.
> +        From now, with setPersistentDataLocation() we can set which
> +        directory should be used by WebKitTestRunner instead of the
> +        default one. Hence, parallelly run WKTRs don't use the same
> +        DataLocation.

I would like to first do the behavior change, than the renamings in a following cleanup patch. It's hard to see from the history what has changed if you do it together.

> Source/WebKit2/Shared/qt/QtPersistentDataLocation.cpp:63
> +QWEBKIT_EXPORT void setPersistentDataLocation(QString dataLocation)

Please move it to global namespace, we don't use internal namespaces for API (even if it's private).

> Tools/ChangeLog:6
> +        [Qt] The WebKitTestRunner uses the default LocalStorage
> +        directory.If ther is the DUMPRENDERTREE_TEMP environment
> +        variables, use that, instead of the default.
> +        https://bugs.webkit.org/show_bug.cgi?id=89666

Changelog error again. Put your description below the default prologue generated by prepare-changelog.

> Tools/WebKitTestRunner/qt/TestControllerQt.cpp:57
> +    if (!qgetenv("DUMPRENDERTREE_TEMP").isNull())
> +        WebKit::setPersistentDataLocation(qgetenv("DUMPRENDERTREE_TEMP"));

Don't call qgetenv twice.

[Szilard Ledan]

Created attachment 149754 [details]
patch

[Balazs Kelemen]

Comment on attachment 149754 [details]
patch

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

Looks good to me.

> Source/WebKit2/Shared/qt/QtDefaultDataLocation.cpp:65
> +// It is private api for WebKitTestRunner

API

> Tools/WebKitTestRunner/qt/TestControllerQt.cpp:61
> +    QString dumprendertreetemp = qgetenv("DUMPRENDERTREE_TEMP");
> +    if (!dumprendertreetemp.isNull())
> +        setPersistentDataLocation(dumprendertreetemp);

dumpRenderTreeTemp

[Simon Hausmann]

Comment on attachment 149754 [details]
patch

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

I do think that the idea is good, but I don't think the implementation is correct.

This is not an issue that is specific to the Qt port, so support for this should be present for all ports using WTR. And that appears to be the case, given that WTR's TestController.cpp already has code for reading the DUMPRENDERTREE_TEMP variable and then uses WKContextSetDatabaseDirectory to direct the databases to the location pointed to by the environment variable.

I suspect that what's happening in our case is that we end up running our initialization code afterwards and over-write the previously set database directory - for example the PlatformWebView is created after the call to the C API.

Consequently I think the cleanest fix would involve us correctly taking the existing cross-platform code path.

> Source/WebKit2/ChangeLog:4
> +        [Qt] Separate WebKit2 instances use the same local storage
> +        https://bugs.webkit.org/show_bug.cgi?id=89666

Can you elaborate a bit more (either in the changelog or bugzilla) what the _symptom_ is that you're trying to fix?

Is this an issue when using NWRT with multiple WTR instances writing to the same directory?

> Tools/ChangeLog:9
> +        directory.If ther is the DUMPRENDERTREE_TEMP environment

Space between punctuation and the If. And "ther" -> "there" :)

[Szilard Ledan]

Created attachment 151914 [details]
patch for ews

[Build Bot]

Comment on attachment 151914 [details]
patch for ews

Attachment 151914 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13198745

[Build Bot]

Comment on attachment 151914 [details]
patch for ews

Attachment 151914 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13208668

[Szilard Ledan]

Created attachment 154367 [details]
patch for ews

[Balazs Kelemen]

Comment on attachment 154367 [details]
patch for ews

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

LGTM with the need of some fix.

> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:52
> +    notImplemented();
> +    return "";

return String()

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:51
> +    // FIXME: Implement
> +    return WTF::String();

I don't think you need WTF:: here.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:-91
> +    parameters.parentProcessName = [[NSProcessInfo processInfo] processName];
>  
>      NSURLCache *urlCache = [NSURLCache sharedURLCache];
> -
> -    parameters.parentProcessName = [[NSProcessInfo processInfo] processName];    
> -    parameters.nsURLCachePath = [(NSString *)cachePath.get() stringByStandardizingPath];

You don't need to move the assigment of parentProcessName, it just adds noise to the patch.

> Source/WebKit2/WebProcess/mac/WebProcessMac.mm:216
> -    appendReadwriteSandboxDirectory(sandboxParameters, "NSURL_CACHE_DIR", parameters.nsURLCachePath);
> +    appendReadwriteSandboxDirectory(sandboxParameters, "NSURL_CACHE_DIR", static_cast<NSString*>(parameters.diskCacheDirectory));

Do you need to cast to NSString* here? Isn't appendRead... a webkit function that get's a String?

> Source/WebKit2/WebProcess/mac/WebProcessMac.mm:264
> -    if (!parameters.nsURLCachePath.isNull()) {
> +    if (!static_cast<NSString*>(parameters.diskCacheDirectory).isNull()) {
>          NSUInteger cacheMemoryCapacity = parameters.nsURLCacheMemoryCapacity;
>          NSUInteger cacheDiskCapacity = parameters.nsURLCacheDiskCapacity;

You don't need the cast here.

> Tools/WebKitTestRunner/TestController.cpp:315
> +    // Can we remove it?
> +//    const char* path = libraryPathForTesting();
> +//    if (path) {
> +//        Vector<char> databaseDirectory(strlen(path) + strlen("/Databases") + 1);
> +//        sprintf(databaseDirectory.data(), "%s%s", path, "/Databases");
> +//        WKRetainPtr<WKStringRef> databaseDirectoryWK(AdoptWK, WKStringCreateWithUTF8CString(databaseDirectory.data()));
> +//        WKContextSetDatabaseDirectory(m_context.get(), databaseDirectoryWK.get());
> +//    }

I think we can. :)

[Balazs Kelemen]

Adding API maintainers because this effects WebKit2 C API.

[Szilard Ledan]

Created attachment 154618 [details]
patch for ews

Thanks for your advice!

[Balazs Kelemen]

Comment on attachment 154618 [details]
patch for ews

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

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:51
> +    // FIXME: Implement
> +    return String();

Nit: missing a notImplemented() call.

[Build Bot]

Comment on attachment 154618 [details]
patch for ews

Attachment 154618 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13368050

[Szilard Ledan]

Created attachment 157977 [details]
patch for ews

[Gustavo Noronha (kov)]

Comment on attachment 157977 [details]
patch for ews

Attachment 157977 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13493071

[Szilard Ledan]

Created attachment 157992 [details]
patch for ews

[Simon Hausmann]

Comment on attachment 149754 [details]
patch

Marking obviously obsolete attachmen to be obsolete :)

[Simon Hausmann]

Comment on attachment 157992 [details]
patch for ews

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

This patch is missing a ChangeLog, which I suppose would provide an explanation as to why these changes are submitted? :)

I'm particularly curious why the disk cache directory needs to be per DRT/WTR instance.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:-95
> -    ASSERT(!parameters.nsURLCachePath.isEmpty());

Technically this ASSERT should move somewhere else, shouldn't it?

> Tools/WebKitTestRunner/TestController.cpp:320
> +        WKContextSetDatabaseDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp));
> +        WKContextSetLocalStorageDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp));
> +        WKContextSetDiskCacheDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp));

I think you need to store the strings created by WKStringCreate* in a RetainPtr to avoid them leaking.

[Csaba Osztrogonác]

(In reply to comment #23)
> (From update of attachment 157992 [details])
> I'm particularly curious why the disk cache directory needs to be per DRT/WTR instance.

Because we would like to go forward and run layout tests parallel (https://bugs.webkit.org/show_bug.cgi?id=77730). It isn't a good
thing if a WTR modifies another ones disk cache. For example purges
it between tests, but the other one still uses it. It was implemented
for WK1 long time ago and works fine.

Additionally we run more bots on same machine (only one WK2 tester now
because of this bug), and don't want to let a slave to break an other
slaves disk cache.

[Szilard Ledan]

(In reply to comment #23)
> (From update of attachment 157992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157992&action=review
> 
> This patch is missing a ChangeLog, which I suppose would provide an explanation as to why these changes are submitted? :)

Sorry for the late response!
These patches were only made for testing EWS, and I had the wrong intention that these patches need to be r=? and cq=? to test the EWS.
I'm going to apply your recommendations and hints in the final patch (that will also contain ChangeLogs of course :) ).

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:-95
> > -    ASSERT(!parameters.nsURLCachePath.isEmpty());
> 
> Technically this ASSERT should move somewhere else, shouldn't it?

From the final patch nsURLCachePath will be obsoleted.
 
> > Tools/WebKitTestRunner/TestController.cpp:320
> > +        WKContextSetDatabaseDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp));
> > +        WKContextSetLocalStorageDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp));
> > +        WKContextSetDiskCacheDirectory(m_context.get(), WKStringCreateWithUTF8CString(dumpRenderTreeTemp));
> 
> I think you need to store the strings created by WKStringCreate* in a RetainPtr to avoid them leaking.

I'm going to do it.

[Sam Weinig]

Removing [Qt] as this touches platform independent code.

[Szilard Ledan]

Created attachment 161824 [details]
patch

[Szilard Ledan]

Created attachment 163054 [details]
patch v2

[Andras Becsi]

Comment on attachment 163054 [details]
patch v2

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

I think this patch is an improvement, though I still find the ChangeLog a bit too brief about the issue that is being fixed.

> Source/WebKit2/ChangeLog:8
> +        WebContext has been extended for the paralelly started WTRs to use

*parallelly*

> Source/WebKit2/ChangeLog:12
> +        separate directories for their local storage.
> +        platformDiskCacheDirectory API has been added to the existing
> +        platformLocalStorageDirectory and platformDataBaseDirectory APIs.
> +

I think it would be good to explain a bit more why this change is needed and what the symptoms of the bug are.

Probably also refer to https://bugs.webkit.org/show_bug.cgi?id=77730 and the special for Qt to run multiple run-webkit-test instances in parallel.

> Source/WebKit2/Shared/WebProcessCreationParameters.h:-104
>      // FIXME: These should be merged with CFURLCache counterparts below.
> -    String nsURLCachePath;
> -    SandboxExtension::Handle nsURLCachePathExtensionHandle;

Following the FIXME comment to me it looks like the cfURLCachePath below is used for the same purpose on Windows as nsURLCachePath on Mac.

Doesn't this patch also affect the need for cfURLCachePath on Windows?

[Szilard Ledan]

Created attachment 167054 [details]
patch for ews

[Build Bot]

Comment on attachment 167054 [details]
patch for ews

Attachment 167054 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14175021

[Szilard Ledan]

Created attachment 167094 [details]
patch

[Build Bot]

Comment on attachment 167094 [details]
patch

Attachment 167094 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14171223

[Szilard Ledan]

Created attachment 167530 [details]
patch

[Simon Hausmann]

Comment on attachment 167530 [details]
patch

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

I think the patch looks good. The only two issues I can find are the missing notImplemented() and the static_cast that appears to be unnecessary. Please fix those before landing :)

(And I suppose this needs very careful perhaps manual landing, since it affects the quite a few ports)

> Source/WebKit2/UIProcess/win/WebContextWin.cpp:107
> +    // FIXME: not implemented!

Use notImplemented(); here?

> Source/WebKit2/WebProcess/mac/WebProcessMac.mm:271
> -        RetainPtr<NSURLCache> parentProcessURLCache(AdoptNS, [[NSURLCache alloc] initWithMemoryCapacity:cacheMemoryCapacity diskCapacity:cacheDiskCapacity diskPath:parameters.nsURLCachePath]);
> +        RetainPtr<NSURLCache> parentProcessURLCache(AdoptNS, [[NSURLCache alloc] initWithMemoryCapacity:cacheMemoryCapacity diskCapacity:cacheDiskCapacity diskPath:static_cast<NSString*>(parameters.diskCacheDirectory)]);

Why is the static_cast needed here?

It seems that with

    diskPath:parameters.nsURLCachePath

the nsURLCachePath as a String. With 

    diskPath:static_cast<NSString*>(parameters.diskCacheDirectory)

the diskCacheDirectory appears to be a String, too. So I don't understand why the cast is needed :)

[Csaba Osztrogonác]

I tried the patch, but it seems WTR still uses this file:
~/.local/share/WebKitTestRunner/.QtWebKit/WebpageIcons.db

Could you check it?

[Balazs Kelemen]

(In reply to comment #36)
> I tried the patch, but it seems WTR still uses this file:
> ~/.local/share/WebKitTestRunner/.QtWebKit/WebpageIcons.db
> 
> Could you check it?

I guess we should override the icondatabase path as well. But it should not stop this patch from landing, we can do that in a follow-up.

[Csaba Osztrogonác]

(In reply to comment #37)
> (In reply to comment #36)
> > I tried the patch, but it seems WTR still uses this file:
> > ~/.local/share/WebKitTestRunner/.QtWebKit/WebpageIcons.db
> > 
> > Could you check it?
> 
> I guess we should override the icondatabase path as well. But it should not stop this patch from landing, we can do that in a follow-up.

Sure, we can do it later.

[Szilard Ledan]

Created attachment 168660 [details]
patch for commit

Thanks for your advice!

[Csaba Osztrogonác]

Comment on attachment 168660 [details]
patch for commit

Clearing flags on attachment: 168660

Committed r131428: <http://trac.webkit.org/changeset/131428>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.