Bug 100678 – [WTR] WebKitTestRunner is not cleaning up the icon database

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

The icon database is not being set to the temporary folder created for the test runner by the test driver, which is removed automatically after the run.

[Thiago Marcos P. Santos]

Created attachment 171426 [details]
Patch

[Brady Eidson]

Comment on attachment 171426 [details]
Patch

DId you test this on multiple platforms?

[Kenneth Rohde Christiansen]

Comment on attachment 171426 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:334
> +#if OS(WINDOWS)
> +        const char separator = '\\';
> +#else
> +        const char separator = '/';
> +#endif
> +        iconDatabaseFileTemp = iconDatabaseFileTemp + separator + "WebpageIcons.db";
> +        WKRetainPtr<WKStringRef> iconDatabaseFileTempWK = WKStringCreateWithUTF8CString(iconDatabaseFileTemp.c_str());

Really? WebCore has methods for doing these things, look in FileSystem.h

[Thiago Marcos P. Santos]

Comment on attachment 171426 [details]
Patch

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

>> Tools/WebKitTestRunner/TestController.cpp:334
>> +        WKRetainPtr<WKStringRef> iconDatabaseFileTempWK = WKStringCreateWithUTF8CString(iconDatabaseFileTemp.c_str());
> 
> Really? WebCore has methods for doing these things, look in FileSystem.h

I would love to use pathByAppendingComponent here, but we can't use the namespace here. I had to do the same way as Tools/WebKitTestRunner/TestInvocation.cpp

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (From update of attachment 171426 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171426&action=review
> 
> >> Tools/WebKitTestRunner/TestController.cpp:334
> >> +        WKRetainPtr<WKStringRef> iconDatabaseFileTempWK = WKStringCreateWithUTF8CString(iconDatabaseFileTemp.c_str());
> > 
> > Really? WebCore has methods for doing these things, look in FileSystem.h
> 
> I would love to use pathByAppendingComponent here, but we can't use the namespace here. I had to do the same way as Tools/WebKitTestRunner/TestInvocation.cpp

Maybe it is time we move FileSystem.h to WTF. This is not the first case.

[Kenneth Rohde Christiansen]

Comment on attachment 171426 [details]
Patch

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

r=me then

> Tools/WebKitTestRunner/TestController.cpp:328
> +#if OS(WINDOWS)

Add a comment why not using pathByAppending...

[Thiago Marcos P. Santos]

(In reply to comment #2)
> (From update of attachment 171426 [details])
> DId you test this on multiple platforms?

I tested it on Linux only with the following ports: Qt, GTK and EFL.

[Thiago Marcos P. Santos]

Created attachment 171615 [details]
Patch

[WebKit Review Bot]

Comment on attachment 171615 [details]
Patch

Clearing flags on attachment: 171615

Committed r133024: <http://trac.webkit.org/changeset/133024>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

This patch is causing random crashes at least on the EFL port. See, for example, <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5289/steps/layout-test/logs/stdio>.

WKContextSetIconDatabasePath() sets a database path, but when we later create our ewk view the call to Ewk_Context::faviconDatabase() ends up calling it again and reaching the assertion in IconDatabase's destructor.

[Thiago Marcos P. Santos]

(In reply to comment #11)
> This patch is causing random crashes at least on the EFL port. See, for example, <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5289/steps/layout-test/logs/stdio>.
> 
> WKContextSetIconDatabasePath() sets a database path, but when we later create our ewk view the call to Ewk_Context::faviconDatabase() ends up calling it again and reaching the assertion in IconDatabase's destructor.

This is probably EFL specific. We should create a separated bug for that.

[Chris Dumez]

Comment on attachment 171615 [details]
Patch

Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path?

[Thiago Marcos P. Santos]

(In reply to comment #13)
> (From update of attachment 171615 [details])
> Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path?

Nobody is "messing with the paths" here. The WTR was creating folders on your computer for every run and not deleting them afterwards. This is what this patch is fixing, by making WTR handle the icon database path in the same way as it does for local storage, database, cookies, etc

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 171615 [details] [details])
> > Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path?
> 
> Nobody is "messing with the paths" here. The WTR was creating folders on your computer for every run and not deleting them afterwards. This is what this patch is fixing, by making WTR handle the icon database path in the same way as it does for local storage, database, cookies, etc

Well, this is not the way EFL / GTK ports have done this so far. We are using XDG paths for databases and config. We usually just override the XDG environment variable in the test runner python script:

Tools/Scripts/webkitpy/layout_tests/port/xvfbdriver.py:        environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')

We should probably override the 'XDG_DATA_HOME' environment variable to achieve the expected behavior on EFL and GTK ports.

[Thiago Marcos P. Santos]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (From update of attachment 171615 [details] [details] [details])
> > > Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path?
> > 
> > Nobody is "messing with the paths" here. The WTR was creating folders on your computer for every run and not deleting them afterwards. This is what this patch is fixing, by making WTR handle the icon database path in the same way as it does for local storage, database, cookies, etc
> 
> Well, this is not the way EFL / GTK ports have done this so far. We are using XDG paths for databases and config. We usually just override the XDG environment variable in the test runner python script:
> 
> Tools/Scripts/webkitpy/layout_tests/port/xvfbdriver.py:        environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')
> 
> We should probably override the 'XDG_DATA_HOME' environment variable to achieve the expected behavior on EFL and GTK ports.

No, we should not. If you "fix" this on xvfbdriver.py, if you happen to use another driver at some point, you will have to move these hacks around. If Chromium or Qt decides to use the xvfbdriver, they might get some unexpected behavior because of these variables on the system environment.

The only reason why we still have XDG_CACHE_HOME on the driver (I removed the others and fixed the XDG_CACHE_HOME - bug 100864 and bug 100864) is because we don't have an WK2 API for setting the application cache folder yet.

Please, let's keep the drivers and WTR platform neutral and EFL aligned with other ports.

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > (From update of attachment 171615 [details] [details] [details] [details])
> > > > Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path?
> > > 
> > > Nobody is "messing with the paths" here. The WTR was creating folders on your computer for every run and not deleting them afterwards. This is what this patch is fixing, by making WTR handle the icon database path in the same way as it does for local storage, database, cookies, etc
> > 
> > Well, this is not the way EFL / GTK ports have done this so far. We are using XDG paths for databases and config. We usually just override the XDG environment variable in the test runner python script:
> > 
> > Tools/Scripts/webkitpy/layout_tests/port/xvfbdriver.py:        environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache')
> > 
> > We should probably override the 'XDG_DATA_HOME' environment variable to achieve the expected behavior on EFL and GTK ports.
> 
> No, we should not. If you "fix" this on xvfbdriver.py, if you happen to use another driver at some point, you will have to move these hacks around. If Chromium or Qt decides to use the xvfbdriver, they might get some unexpected behavior because of these variables on the system environment.
> 
> The only reason why we still have XDG_CACHE_HOME on the driver (I removed the others and fixed the XDG_CACHE_HOME - bug 100864 and bug 100864) is because we don't have an WK2 API for setting the application cache folder yet.
> 
> Please, let's keep the drivers and WTR platform neutral and EFL aligned with other ports.

I see. The issue is that the database path can only be set once. If you set it in WKTR then we can no longer set it in WebKit2 EFL. We would probably need to expose a settings API to set the database path and let the application take care of it. Right?

[Csaba Osztrogonác]

*** Bug 99448 has been marked as a duplicate of this bug. ***