100678 2012-10-29 08:36:01 -0700 [WTR] WebKitTestRunner is not cleaning up the icon database 2012-11-18 11:53:58 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 102616 100346 101182 1 tmpsantos tmpsantos ap beidson cdumez kenneth mikhail.pozdnyakov ossy rakuco webkit.review.bot zoltan.nyul oldest_to_newest 753472 0 tmpsantos 2012-10-29 08:36:01 -0700 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. 754245 1 171426 tmpsantos 2012-10-30 05:29:20 -0700 Created attachment 171426 Patch 754263 2 171426 beidson 2012-10-30 05:52:34 -0700 Comment on attachment 171426 Patch DId you test this on multiple platforms? 754266 3 171426 kenneth 2012-10-30 05:56:47 -0700 Comment on attachment 171426 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 754297 4 171426 tmpsantos 2012-10-30 06:32:59 -0700 Comment on attachment 171426 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 754304 5 kenneth 2012-10-30 06:37:12 -0700 (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. 754305 6 171426 kenneth 2012-10-30 06:37:41 -0700 Comment on attachment 171426 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... 755086 7 tmpsantos 2012-10-31 02:48:39 -0700 (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. 755137 8 171615 tmpsantos 2012-10-31 04:20:36 -0700 Created attachment 171615 Patch 755257 9 171615 webkit.review.bot 2012-10-31 07:02:04 -0700 Comment on attachment 171615 Patch Clearing flags on attachment: 171615 Committed r133024: <http://trac.webkit.org/changeset/133024> 755258 10 webkit.review.bot 2012-10-31 07:02:08 -0700 All reviewed patches have been landed. Closing bug. 756311 11 rakuco 2012-11-01 08:56:52 -0700 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. 756327 12 tmpsantos 2012-11-01 09:16:08 -0700 (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. 756334 13 171615 cdumez 2012-11-01 09:19:34 -0700 Comment on attachment 171615 Patch Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path? 756348 14 tmpsantos 2012-11-01 09:38:55 -0700 (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 756354 15 cdumez 2012-11-01 09:44:45 -0700 (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. 756385 16 tmpsantos 2012-11-01 10:03:48 -0700 (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. 756406 17 cdumez 2012-11-01 10:17:16 -0700 (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? 758107 18 ossy 2012-11-04 04:13:44 -0800 *** Bug 99448 has been marked as a duplicate of this bug. *** 171426 2012-10-30 05:29:20 -0700 2012-10-31 04:20:29 -0700 Patch bug-100678-20121030143134.patch text/plain 1995 tmpsantos U3VidmVyc2lvbiBSZXZpc2lvbjogMTMyODkzCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYTQ1NjcxMDk3ZDI3Mzg0MWVmMTczMGRkNjA3ZmZmNTRh YmRlZmI2Ny4uMzE2M2VlMWEyMDRhMDE1M2I4OGIwZjVmZjNmZWFlOTE4NGM3YmJkZCAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3 IEBACisyMDEyLTEwLTMwICBUaGlhZ28gTWFyY29zIFAuIFNhbnRvcyAgPHRoaWFnby5zYW50b3NA aW50ZWwuY29tPgorCisgICAgICAgIFtXVFJdIFdlYktpdFRlc3RSdW5uZXIgaXMgbm90IGNsZWFu aW5nIHVwIHRoZSBpY29uIGRhdGFiYXNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMDA2NzgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBUaGUgaWNvbiBkYXRhYmFzZSB3YXMgbm90IGJlaW5nIHNldCB0byB0 aGUgdGVtcG9yYXJ5IGZvbGRlciBjcmVhdGVkIGZvcgorICAgICAgICB0aGUgdGVzdCBydW5uZXIg YnkgdGhlIHRlc3QgZHJpdmVyLCB3aGljaCBpcyByZW1vdmVkIGF1dG9tYXRpY2FsbHkKKyAgICAg ICAgYWZ0ZXIgdGhlIHJ1bi4KKworICAgICAgICAqIFdlYktpdFRlc3RSdW5uZXIvVGVzdENvbnRy b2xsZXIuY3BwOgorICAgICAgICAoV1RSOjpUZXN0Q29udHJvbGxlcjo6aW5pdGlhbGl6ZSk6CisK IDIwMTItMTAtMzAgIEthbmdpbCBIYW4gIDxrYW5naWwuaGFuQHNhbXN1bmcuY29tPgogCiAgICAg ICAgIFtFRkxdW0RSVF0gQml0bWFwIHNob3VsZCBzaG93IHdob2xlIHZpZXcgYXJlYS4KZGlmZiAt LWdpdCBhL1Rvb2xzL1dlYktpdFRlc3RSdW5uZXIvVGVzdENvbnRyb2xsZXIuY3BwIGIvVG9vbHMv V2ViS2l0VGVzdFJ1bm5lci9UZXN0Q29udHJvbGxlci5jcHAKaW5kZXggMDQ3ZjVhMWJhZTA2NjNj MjdhZTgwZWIwYmNmZTg1ZWY5Yjk0NzJiOS4uNjgzZDJmZjU0MDU4YWJjMWEzMzdiOWU4NDYzM2Ew YmNjYmEzMmFlZSAxMDA2NDQKLS0tIGEvVG9vbHMvV2ViS2l0VGVzdFJ1bm5lci9UZXN0Q29udHJv bGxlci5jcHAKKysrIGIvVG9vbHMvV2ViS2l0VGVzdFJ1bm5lci9UZXN0Q29udHJvbGxlci5jcHAK QEAgLTMyMyw2ICszMjMsMTYgQEAgdm9pZCBUZXN0Q29udHJvbGxlcjo6aW5pdGlhbGl6ZShpbnQg YXJnYywgY29uc3QgY2hhciogYXJndltdKQogICAgICAgICBXS0NvbnRleHRTZXRMb2NhbFN0b3Jh Z2VEaXJlY3RvcnkobV9jb250ZXh0LmdldCgpLCBkdW1wUmVuZGVyVHJlZVRlbXBXSy5nZXQoKSk7 CiAgICAgICAgIFdLQ29udGV4dFNldERpc2tDYWNoZURpcmVjdG9yeShtX2NvbnRleHQuZ2V0KCks IGR1bXBSZW5kZXJUcmVlVGVtcFdLLmdldCgpKTsKICAgICAgICAgV0tDb250ZXh0U2V0Q29va2ll U3RvcmFnZURpcmVjdG9yeShtX2NvbnRleHQuZ2V0KCksIGR1bXBSZW5kZXJUcmVlVGVtcFdLLmdl dCgpKTsKKworICAgICAgICBzdGQ6OnN0cmluZyBpY29uRGF0YWJhc2VGaWxlVGVtcChkdW1wUmVu ZGVyVHJlZVRlbXApOworI2lmIE9TKFdJTkRPV1MpCisgICAgICAgIGNvbnN0IGNoYXIgc2VwYXJh dG9yID0gJ1xcJzsKKyNlbHNlCisgICAgICAgIGNvbnN0IGNoYXIgc2VwYXJhdG9yID0gJy8nOwor I2VuZGlmCisgICAgICAgIGljb25EYXRhYmFzZUZpbGVUZW1wID0gaWNvbkRhdGFiYXNlRmlsZVRl bXAgKyBzZXBhcmF0b3IgKyAiV2VicGFnZUljb25zLmRiIjsKKyAgICAgICAgV0tSZXRhaW5QdHI8 V0tTdHJpbmdSZWY+IGljb25EYXRhYmFzZUZpbGVUZW1wV0sgPSBXS1N0cmluZ0NyZWF0ZVdpdGhV VEY4Q1N0cmluZyhpY29uRGF0YWJhc2VGaWxlVGVtcC5jX3N0cigpKTsKKyAgICAgICAgV0tDb250 ZXh0U2V0SWNvbkRhdGFiYXNlUGF0aChtX2NvbnRleHQuZ2V0KCksIGljb25EYXRhYmFzZUZpbGVU ZW1wV0suZ2V0KCkpOwogICAgIH0KIAogICAgIHBsYXRmb3JtSW5pdGlhbGl6ZUNvbnRleHQoKTsK 171615 2012-10-31 04:20:36 -0700 2012-11-01 09:19:34 -0700 Patch bug-100678-20121031132249.patch text/plain 2182 tmpsantos U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzMDA2CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggNmZkNWExNGQ5NDkyMDQ3Y2FkMGFjMmNjZTMyNDIzMGY1 MmRhYmY1MC4uMDkwYTcxZDQxYzI1Zjk3NDAxZDhiZjZiMDhhYzVmZDZkZWIyNmQ2ZiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3 IEBACisyMDEyLTEwLTMxICBUaGlhZ28gTWFyY29zIFAuIFNhbnRvcyAgPHRoaWFnby5zYW50b3NA aW50ZWwuY29tPgorCisgICAgICAgIFtXVFJdIFdlYktpdFRlc3RSdW5uZXIgaXMgbm90IGNsZWFu aW5nIHVwIHRoZSBpY29uIGRhdGFiYXNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMDA2NzgKKworICAgICAgICBSZXZpZXdlZCBieSBLZW5uZXRoIFJv aGRlIENocmlzdGlhbnNlbi4KKworICAgICAgICBUaGUgaWNvbiBkYXRhYmFzZSB3YXMgbm90IGJl aW5nIHNldCB0byB0aGUgdGVtcG9yYXJ5IGZvbGRlciBjcmVhdGVkIGZvcgorICAgICAgICB0aGUg dGVzdCBydW5uZXIgYnkgdGhlIHRlc3QgZHJpdmVyLCB3aGljaCBpcyByZW1vdmVkIGF1dG9tYXRp Y2FsbHkKKyAgICAgICAgYWZ0ZXIgdGhlIHJ1bi4KKworICAgICAgICAqIFdlYktpdFRlc3RSdW5u ZXIvVGVzdENvbnRyb2xsZXIuY3BwOgorICAgICAgICAoV1RSOjpUZXN0Q29udHJvbGxlcjo6aW5p dGlhbGl6ZSk6CisKIDIwMTItMTAtMzEgIE1pa2UgV2VzdCAgPG1rd3N0QGNocm9taXVtLm9yZz4K IAogICAgICAgICBTY3JpcHQgcnVuIGZyb20gYW4gaXNvbGF0ZWQgd29ybGQgc2hvdWxkIGJ5cGFz cyBhIHBhZ2UncyBDU1AuCmRpZmYgLS1naXQgYS9Ub29scy9XZWJLaXRUZXN0UnVubmVyL1Rlc3RD b250cm9sbGVyLmNwcCBiL1Rvb2xzL1dlYktpdFRlc3RSdW5uZXIvVGVzdENvbnRyb2xsZXIuY3Bw CmluZGV4IDA0N2Y1YTFiYWUwNjYzYzI3YWU4MGViMGJjZmU4NWVmOWI5NDcyYjkuLjE2YzUxMjIw NmFjNzQ2OTJiYTgzOTg2OTdhM2NkMTRlNTAxZDY0YTUgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1dlYktp dFRlc3RSdW5uZXIvVGVzdENvbnRyb2xsZXIuY3BwCisrKyBiL1Rvb2xzL1dlYktpdFRlc3RSdW5u ZXIvVGVzdENvbnRyb2xsZXIuY3BwCkBAIC0zMjMsNiArMzIzLDE4IEBAIHZvaWQgVGVzdENvbnRy b2xsZXI6OmluaXRpYWxpemUoaW50IGFyZ2MsIGNvbnN0IGNoYXIqIGFyZ3ZbXSkKICAgICAgICAg V0tDb250ZXh0U2V0TG9jYWxTdG9yYWdlRGlyZWN0b3J5KG1fY29udGV4dC5nZXQoKSwgZHVtcFJl bmRlclRyZWVUZW1wV0suZ2V0KCkpOwogICAgICAgICBXS0NvbnRleHRTZXREaXNrQ2FjaGVEaXJl Y3RvcnkobV9jb250ZXh0LmdldCgpLCBkdW1wUmVuZGVyVHJlZVRlbXBXSy5nZXQoKSk7CiAgICAg ICAgIFdLQ29udGV4dFNldENvb2tpZVN0b3JhZ2VEaXJlY3RvcnkobV9jb250ZXh0LmdldCgpLCBk dW1wUmVuZGVyVHJlZVRlbXBXSy5nZXQoKSk7CisKKyAgICAgICAgc3RkOjpzdHJpbmcgaWNvbkRh dGFiYXNlRmlsZVRlbXAoZHVtcFJlbmRlclRyZWVUZW1wKTsKKyAgICAgICAgLy8gV2ViQ29yZTo6 cGF0aEJ5QXBwZW5kaW5nQ29tcG9uZW50IGlzIG5vdCB1c2VkIGhlcmUgYmVjYXVzZSBvZiB0aGUg bmFtZXNwYWNlLAorICAgICAgICAvLyB3aGljaCBsZWFkcyB1cyB0byB0aGlzIHVnbHkgI2lmZGVm IGFuZCBmaWxlIHBhdGggY29uY2F0ZW5hdGlvbi4KKyNpZiBPUyhXSU5ET1dTKQorICAgICAgICBj b25zdCBjaGFyIHNlcGFyYXRvciA9ICdcXCc7CisjZWxzZQorICAgICAgICBjb25zdCBjaGFyIHNl cGFyYXRvciA9ICcvJzsKKyNlbmRpZgorICAgICAgICBpY29uRGF0YWJhc2VGaWxlVGVtcCA9IGlj b25EYXRhYmFzZUZpbGVUZW1wICsgc2VwYXJhdG9yICsgIldlYnBhZ2VJY29ucy5kYiI7CisgICAg ICAgIFdLUmV0YWluUHRyPFdLU3RyaW5nUmVmPiBpY29uRGF0YWJhc2VGaWxlVGVtcFdLID0gV0tT dHJpbmdDcmVhdGVXaXRoVVRGOENTdHJpbmcoaWNvbkRhdGFiYXNlRmlsZVRlbXAuY19zdHIoKSk7 CisgICAgICAgIFdLQ29udGV4dFNldEljb25EYXRhYmFzZVBhdGgobV9jb250ZXh0LmdldCgpLCBp Y29uRGF0YWJhc2VGaWxlVGVtcFdLLmdldCgpKTsKICAgICB9CiAKICAgICBwbGF0Zm9ybUluaXRp YWxpemVDb250ZXh0KCk7Cg==