Summary: | [EFL] REGRESSION (r178685): ASSERTION FAILED: !parameters.mediaKeyStorageDirectory.isEmpty() | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Paweł Forysiuk <p.forysiuk> | ||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, cdumez, commit-queue, darin, eric.carlson, g.czajkowski, gyuyoung.kim, jer.noble, kling, ossy, p.forysiuk, sam | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Paweł Forysiuk
2015-02-02 04:35:52 PST
Created attachment 245930 [details]
Patch
Could you explain why is it a regression of r178685 and why do you think if it is the proper fix? It's not clear for me why is it EFL specific. I haven't found any EFL specific thing near mediaKeyStorageDirectory. Well the patch uploaded was just a quick workaround to be honest. I don't know if it is just EFL specific but GTK port does not seem to be affected. (In reply to comment #2) > Could you explain why is it a regression of r178685 and why It started to occur after that commit, i bisected it and reverting that change would prevent the assertion. > do you think if it is the proper fix? > No. That was a quick workaround. I will attach a different patch, seems like a more proper solution but I'm still unsure. > It's not clear for me why is it EFL specific. I haven't found > any EFL specific thing near mediaKeyStorageDirectory. Don't know if it is just EFL specific but it does not occur on GTK port, they use slighly different codepaths when setting things up. Seems like GTK port somehow creates those folders when initialising Web process whlie EFL does not it seems. If I miss something obvious please tell me. Created attachment 246656 [details]
Updated patch with more generic code
Attachment 246656 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/efl/WebsiteDataStoreEFL.cpp:28: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/efl/WebsiteDataStoreEFL.cpp:29: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246657 [details]
Fixed up previous patch
Created attachment 246658 [details]
patch
(In reply to comment #3) > Well the patch uploaded was just a quick workaround to be honest. > > > I don't know if it is just EFL specific but GTK port > does not seem to be affected. > Probably because of they always create legacy options when context is constructed. See their webkitWebContextConstructed(GObject* object) in "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp" > > (In reply to comment #2) > > Could you explain why is it a regression of r178685 and why > > It started to occur after that commit, i bisected it and reverting that > change > would prevent the assertion. > > > do you think if it is the proper fix? > > > > No. That was a quick workaround. I will attach a different patch, seems like > a more proper solution but I'm still unsure. > > > It's not clear for me why is it EFL specific. I haven't found > > any EFL specific thing near mediaKeyStorageDirectory. > > Don't know if it is just EFL specific but it does not occur on GTK port, > they use slighly different codepaths when setting things up. Seems like GTK > port somehow creates those folders when initialising Web process whlie EFL > does not it seems. If I miss something obvious please tell me. I think it's good idea to implement WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation for EFL and have it WebKitTestRunner specific only. It looks like Mac already does, see Source/WebKit2/UIProcess/API/Cocoa/APIWebsiteDataStoreCocoa.mm After that, we will have separate paths for testing and potential applications. Comment on attachment 246658 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=246658&action=review > Source/WebKit2/ChangeLog:7 > + What to be nice to describe what is the purpose of this change. > Source/WebKit2/ChangeLog:10 > + * PlatformGTK.cmake: Ditto. According to your description, this line should go above. > Source/WebKit2/PlatformEfl.cmake:83 > + UIProcess/API/efl/WebsiteDataStoreEFL.cpp Try to keep alphabetical order. > Source/WebKit2/UIProcess/API/efl/WebsiteDataStoreEFL.cpp:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. Apple ? > Source/WebKit2/UIProcess/API/efl/WebsiteDataStoreEFL.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' Please use BSD license. > Source/WebKit2/UIProcess/API/efl/WebsiteDataStoreEFL.cpp:27 > +#include "APIWebsiteDataStore.h" An empty line is required here. > Source/WebKit2/UIProcess/API/efl/WebsiteDataStoreEFL.cpp:36 > + const String& path = String::fromUTF8(efreet_data_home_get()) + "/" + directoryName; Can we use more WebKitTestRunner specific path? How about taking it from DUMPRENDERTREE_TEMP? Ossy and Gyuyoung, what's your take on it? > Source/WebKit2/UIProcess/API/efl/WebsiteDataStoreEFL.cpp:48 > + configuration.localStorageDirectory = websiteDataDirectoryFileSystemRepresentation("LocalStorage"); Is this needed? Seems strange that we partially implement it. Either ensure all storage paths here or remove local storage path. Comment on attachment 246658 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=246658&action=review >> Source/WebKit2/UIProcess/API/efl/WebsiteDataStoreEFL.cpp:36 >> + const String& path = String::fromUTF8(efreet_data_home_get()) + "/" + directoryName; > > Can we use more WebKitTestRunner specific path? How about taking it from DUMPRENDERTREE_TEMP? Ossy and Gyuyoung, what's your take on it? Please ignore this comment, WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation is not WebKitTestRunner specific. (In reply to comment #8) > (In reply to comment #3) > > Well the patch uploaded was just a quick workaround to be honest. > > > > > > I don't know if it is just EFL specific but GTK port > > does not seem to be affected. > > > > Probably because of they always create legacy options when context is > constructed. See their webkitWebContextConstructed(GObject* object) in > "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp" > > > > > (In reply to comment #2) > > > Could you explain why is it a regression of r178685 and why > > > > It started to occur after that commit, i bisected it and reverting that > > change > > would prevent the assertion. > > > > > do you think if it is the proper fix? > > > > > > > No. That was a quick workaround. I will attach a different patch, seems like > > a more proper solution but I'm still unsure. > > > > > It's not clear for me why is it EFL specific. I haven't found > > > any EFL specific thing near mediaKeyStorageDirectory. > > > > Don't know if it is just EFL specific but it does not occur on GTK port, > > they use slighly different codepaths when setting things up. Seems like GTK > > port somehow creates those folders when initialising Web process whlie EFL > > does not it seems. If I miss something obvious please tell me. > > I think it's good idea to implement > WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation for EFL and > have it WebKitTestRunner specific only. It looks like Mac already does, see > Source/WebKit2/UIProcess/API/Cocoa/APIWebsiteDataStoreCocoa.mm > > After that, we will have separate paths for testing and potential > applications. Please ignore this comment, WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation is not WebKitTestRunner specific. Created attachment 247853 [details]
Patch
IMHO, looks sensibly to set MediaKeys as well as other DataBase paths in WebKitTestRunner code. Since it affects Mac port as well I'm adding Apple developers. We can not run-webkit-tests in debug build because of it :( This is a non-EFL specific WebKitTestRunner change, maybe Alexey can take a look? +Eric and Jer. Comment on attachment 247853 [details]
Patch
LGTM. r+.
Comment on attachment 247853 [details]
Patch
Thank you for fixing it!
Comment on attachment 247853 [details] Patch Clearing flags on attachment: 247853 Committed r181071: <http://trac.webkit.org/changeset/181071> All reviewed patches have been landed. Closing bug. |