Bug 89666 - Separate WebKit2 instances use the same local storage
Summary: Separate WebKit2 instances use the same local storage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 99445 99448
Blocks: 77730
  Show dependency treegraph
 
Reported: 2012-06-21 08:18 PDT by Szilard Ledan
Modified: 2012-10-16 03:34 PDT (History)
21 users (show)

See Also:


Attachments
patch (3.67 KB, patch)
2012-06-21 08:19 PDT, Szilard Ledan
kbalazs: review-
Details | Formatted Diff | Diff
patch (17.15 KB, patch)
2012-06-22 04:48 PDT, Szilard Ledan
kbalazs: review-
Details | Formatted Diff | Diff
patch (4.60 KB, patch)
2012-06-27 08:39 PDT, Szilard Ledan
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
patch for ews (11.41 KB, patch)
2012-07-12 05:05 PDT, Szilard Ledan
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch for ews (12.99 KB, patch)
2012-07-25 08:41 PDT, Szilard Ledan
no flags Details | Formatted Diff | Diff
patch for ews (12.70 KB, patch)
2012-07-26 05:34 PDT, Szilard Ledan
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch for ews (15.36 KB, patch)
2012-08-13 06:13 PDT, Szilard Ledan
gns: commit-queue-
Details | Formatted Diff | Diff
patch for ews (15.34 KB, patch)
2012-08-13 07:31 PDT, Szilard Ledan
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
patch (19.08 KB, patch)
2012-09-01 05:15 PDT, Szilard Ledan
no flags Details | Formatted Diff | Diff
patch v2 (19.54 KB, patch)
2012-09-10 00:30 PDT, Szilard Ledan
no flags Details | Formatted Diff | Diff
patch for ews (22.99 KB, patch)
2012-10-04 01:53 PDT, Szilard Ledan
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch (26.31 KB, patch)
2012-10-04 06:34 PDT, Szilard Ledan
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch (26.30 KB, patch)
2012-10-08 06:18 PDT, Szilard Ledan
hausmann: review+
hausmann: commit-queue-
Details | Formatted Diff | Diff
patch for commit (26.45 KB, patch)
2012-10-15 02:50 PDT, Szilard Ledan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Szilard Ledan 2012-06-21 08:18:39 PDT
WK1 uses separate temporary directories while WK2 does not. Similar to WK1, WK2 should also deal with own temporary directories.
Comment 1 Szilard Ledan 2012-06-21 08:19:45 PDT
Created attachment 148807 [details]
patch
Comment 2 WebKit Review Bot 2012-06-21 08:21:25 PDT
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.
Comment 3 Balazs Kelemen 2012-06-21 08:37:49 PDT
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.
Comment 4 Balazs Kelemen 2012-06-21 08:38:45 PDT
> 
> > 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.
Comment 5 Szilard Ledan 2012-06-22 04:48:46 PDT
Created attachment 148997 [details]
patch

Thanks your advice. I fixed the patch.
Comment 6 Balazs Kelemen 2012-06-23 04:17:00 PDT
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.
Comment 7 Szilard Ledan 2012-06-27 08:39:15 PDT
Created attachment 149754 [details]
patch
Comment 8 Balazs Kelemen 2012-06-27 09:23:18 PDT
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
Comment 9 Simon Hausmann 2012-06-27 22:18:02 PDT
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" :)
Comment 10 Szilard Ledan 2012-07-12 05:05:03 PDT
Created attachment 151914 [details]
patch for ews
Comment 11 Build Bot 2012-07-12 05:11:58 PDT
Comment on attachment 151914 [details]
patch for ews

Attachment 151914 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13198745
Comment 12 Build Bot 2012-07-12 05:33:09 PDT
Comment on attachment 151914 [details]
patch for ews

Attachment 151914 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13208668
Comment 13 Szilard Ledan 2012-07-25 08:41:07 PDT
Created attachment 154367 [details]
patch for ews
Comment 14 Balazs Kelemen 2012-07-26 02:30:40 PDT
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. :)
Comment 15 Balazs Kelemen 2012-07-26 02:39:58 PDT
Adding API maintainers because this effects WebKit2 C API.
Comment 16 Szilard Ledan 2012-07-26 05:34:37 PDT
Created attachment 154618 [details]
patch for ews

Thanks for your advice!
Comment 17 Balazs Kelemen 2012-07-26 06:05:07 PDT
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.
Comment 18 Build Bot 2012-07-26 14:17:43 PDT
Comment on attachment 154618 [details]
patch for ews

Attachment 154618 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13368050
Comment 19 Szilard Ledan 2012-08-13 06:13:50 PDT
Created attachment 157977 [details]
patch for ews
Comment 20 Gustavo Noronha (kov) 2012-08-13 06:18:45 PDT
Comment on attachment 157977 [details]
patch for ews

Attachment 157977 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13493071
Comment 21 Szilard Ledan 2012-08-13 07:31:36 PDT
Created attachment 157992 [details]
patch for ews
Comment 22 Simon Hausmann 2012-08-23 23:09:31 PDT
Comment on attachment 149754 [details]
patch

Marking obviously obsolete attachmen to be obsolete :)
Comment 23 Simon Hausmann 2012-08-23 23:13:57 PDT
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.
Comment 24 Csaba Osztrogonác 2012-08-31 07:03:17 PDT
(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.
Comment 25 Szilard Ledan 2012-08-31 07:24:01 PDT
(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.
Comment 26 Sam Weinig 2012-08-31 15:17:03 PDT
Removing [Qt] as this touches platform independent code.
Comment 27 Szilard Ledan 2012-09-01 05:15:43 PDT
Created attachment 161824 [details]
patch
Comment 28 Szilard Ledan 2012-09-10 00:30:36 PDT
Created attachment 163054 [details]
patch v2
Comment 29 Andras Becsi 2012-09-10 03:37:44 PDT
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?
Comment 30 Szilard Ledan 2012-10-04 01:53:03 PDT
Created attachment 167054 [details]
patch for ews
Comment 31 Build Bot 2012-10-04 02:07:51 PDT
Comment on attachment 167054 [details]
patch for ews

Attachment 167054 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14175021
Comment 32 Szilard Ledan 2012-10-04 06:34:42 PDT
Created attachment 167094 [details]
patch
Comment 33 Build Bot 2012-10-04 06:54:12 PDT
Comment on attachment 167094 [details]
patch

Attachment 167094 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14171223
Comment 34 Szilard Ledan 2012-10-08 06:18:28 PDT
Created attachment 167530 [details]
patch
Comment 35 Simon Hausmann 2012-10-12 03:27:08 PDT
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 :)
Comment 36 Csaba Osztrogonác 2012-10-12 03:56:48 PDT
I tried the patch, but it seems WTR still uses this file:
~/.local/share/WebKitTestRunner/.QtWebKit/WebpageIcons.db

Could you check it?
Comment 37 Balazs Kelemen 2012-10-12 04:36:48 PDT
(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.
Comment 38 Csaba Osztrogonác 2012-10-12 04:38:32 PDT
(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.
Comment 39 Szilard Ledan 2012-10-15 02:50:45 PDT
Created attachment 168660 [details]
patch for commit

Thanks for your advice!
Comment 40 Csaba Osztrogonác 2012-10-16 01:43:10 PDT
Comment on attachment 168660 [details]
patch for commit

Clearing flags on attachment: 168660

Committed r131428: <http://trac.webkit.org/changeset/131428>
Comment 41 Csaba Osztrogonác 2012-10-16 01:43:23 PDT
All reviewed patches have been landed.  Closing bug.