Bug 168171

Sorry about that. Are you able to mark them as skipped for now? Thanks!

The feature is disabled by default as can be seen in the test cases' call to enable it through internals so you shouldn't risk anything by skipping the tests for now. We'll have to look for the root cause.

I marked them as crashing in GTK+ expectations. I spent some time debugging it but couldn't find anything, that's why I thought it could be a compiler issue. Also tried different approaches, copying the temporary string, using add instead of ensure and some other things, and it always crashed when adding the item to the HashMap.

Thanks! I'll see what we can do next week. I'm not an expert on the differences in the two compilers but I have come across certain compiler errors specific to GTK and Efl for this part of the code. I believe it was an implicit this-pointer in a lambda that confused GCC.

These tests are now all timing out instead of crashing. So we will probably need to fix the timeout in order to get them crashing again in order to fix the crash. The timeouts started somewhere between r219010 and r219065, inclusive. Unfortunately that's a huge range as our test bot has been having memory issues recently. The failures look like this: --- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-mixed-statistics-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-mixed-statistics-actual.txt @@ -4,5 +4,5 @@ main frame - didFinishDocumentLoadForFrame main frame - didHandleOnloadEventsForFrame main frame - didFinishLoadForFrame -PASS Host did not get classified as prevalent resource. +FAIL: Timed out waiting for notifyDone to be called Updating expectations accordingly.

Aha, the WPE bot caught it! They regressed in r219049 "Replace ResourceLoadStatisticsStore C API with Cocoa SPI".

Looks like the TestController implementation for non-Cocoa platforms was purposefully removed. We'll have to implement it. Supporting resource load statistics is interesting for us anyway; we get feature requests for this all the time.

Created attachment 315114 [details] Patch

Comment on attachment 315114 [details] Patch Clearing flags on attachment: 315114 Committed r219345: <http://trac.webkit.org/changeset/219345>

All reviewed patches have been landed. Closing bug.

Reopening: r219345 was a gardening commit

Created attachment 322538 [details] Patch

Attachment 322538 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:53: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:55: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:122: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:147: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:169: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:343: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 9 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 322538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322538&action=review > Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:51 > + void setStatisticsLastSeen(const String&, double); Since there are a lot of these and they are only used for testing, I had intentionally not added them to APIWebSiteDataStore.h. Instead, I had the API side (see existing ObjC API such as _resourceLoadStatisticsClearInMemoryAndPersistentStoreModifiedSinceHours) get the resource load statistics from the WebSiteDataStore and use the statistics object directly. > Tools/WebKitTestRunner/TestController.cpp:-2295 > -#if !PLATFORM(COCOA) || !WK_API_ENABLED I believe this change breaks the mac-32bit EWS.

Comment on attachment 322538 [details] Patch Attachment 322538 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4744706 New failing tests: workers/wasm-hashset.html

Created attachment 322577 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6

(In reply to Chris Dumez from comment #14) > Comment on attachment 322538 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322538&action=review > > > Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:51 > > + void setStatisticsLastSeen(const String&, double); > > Since there are a lot of these and they are only used for testing, I had > intentionally not added them to APIWebSiteDataStore.h. Instead, I had the > API side (see existing ObjC API such as > _resourceLoadStatisticsClearInMemoryAndPersistentStoreModifiedSinceHours) > get the resource load statistics from the WebSiteDataStore and use the > statistics object directly. I know, but we don't have other C API to do the same, so I don't know how to implement the test controller methods without adding C API. > > Tools/WebKitTestRunner/TestController.cpp:-2295 > > -#if !PLATFORM(COCOA) || !WK_API_ENABLED > > I believe this change breaks the mac-32bit EWS. Right, I guess I need to ifdef the implementation of every method now, I'll try.

(In reply to Carlos Garcia Campos from comment #17) > > > Tools/WebKitTestRunner/TestController.cpp:-2295 > > > -#if !PLATFORM(COCOA) || !WK_API_ENABLED > > > > I believe this change breaks the mac-32bit EWS. > > Right, I guess I need to ifdef the implementation of every method now, I'll > try. I can leave it as it was since we don't define WK_API_ENABLED

Created attachment 322637 [details] Fix the build

Attachment 322637 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:53: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:55: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:57: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:122: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:147: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:169: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/API/APIWebsiteDataStore.cpp:343: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 9 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Carlos Garcia Campos from comment #17) > I know, but we don't have other C API to do the same, so I don't know how to > implement the test controller methods without adding C API. I'm pretty sure we do not want to start using GLib API from the test controller...?

(In reply to Michael Catanzaro from comment #21) > (In reply to Carlos Garcia Campos from comment #17) > > I know, but we don't have other C API to do the same, so I don't know how to > > implement the test controller methods without adding C API. > > I'm pretty sure we do not want to start using GLib API from the test > controller...? We can't. There's no easy way to add private api to the glib API just for WTR.

(In reply to Carlos Garcia Campos from comment #17) > (In reply to Chris Dumez from comment #14) > > Comment on attachment 322538 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=322538&action=review > > > > > Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:51 > > > + void setStatisticsLastSeen(const String&, double); > > > > Since there are a lot of these and they are only used for testing, I had > > intentionally not added them to APIWebSiteDataStore.h. Instead, I had the > > API side (see existing ObjC API such as > > _resourceLoadStatisticsClearInMemoryAndPersistentStoreModifiedSinceHours) > > get the resource load statistics from the WebSiteDataStore and use the > > statistics object directly. > > I know, but we don't have other C API to do the same, so I don't know how to > implement the test controller methods without adding C API. You misunderstood my comment I believe. I am fine with you adding C API. I just want this C API to use the ResourceLoadStatisticsStore directly (like we do in the ObjC API) instead of adding ALL those methods on APIWebsiteDataStore.

Comment on attachment 322637 [details] Fix the build View in context: https://bugs.webkit.org/attachment.cgi?id=322637&action=review Please apply previous review comments. > Source/WebKit/UIProcess/API/APIWebsiteDataStore.h:51 > + void setStatisticsLastSeen(const String&, double); We do not need all those methods. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:61 > + WebKit::toImpl(dataStoreRef)->setStatisticsLastSeen(WebKit::toImpl(host)->string(), seconds); auto& statistics = WebKit::toImpl(dataStoreRef)->resourceLoadStatistics(); statistics.setStatisticsLastSeen(WebKit::toImpl(host)->string(), seconds);

Created attachment 322806 [details] Patch

Comment on attachment 322806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322806&action=review r=me with review comments. > Source/WebCore/platform/glib/FileMonitorGLib.cpp:38 > + if (path.isEmpty() || !modificationHandler) This is a bad check. !modificationHandler will always be true since you WTFMove() it above.. I think this show this is not tested. This is supposed to be covered by Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp API test which is probably not enabled for GTK port? > Source/WebCore/platform/glib/FileMonitorGLib.cpp:41 > + GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(fileSystemRepresentation(path).data())); auto file ? > Source/WebCore/platform/glib/FileMonitorGLib.cpp:60 > + if (event == G_FILE_MONITOR_EVENT_DELETED) We may not want to call the callback if event == G_FILE_MONITOR_EVENT_ATTRIBUTE_CHANGED ? > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:44 > +typedef void (*WKWebsiteDataStoreIsStatisticsPrevalentResourceFunction)(bool isPrevalentResource, WKErrorRef error, void* functionContext); Do we ever use those WKErrorRef parameters? If not, I would suggest dropping them. > Tools/WebKitTestRunner/TestController.cpp:2312 > +struct ResourceStatisticsCallbackContext { I would give this a constructor that takes a TestController& in parameter and initializes both done / result to false. ResourceStatisticsCallbackContext(TestController& testController) : testController(&testController) { } otherwise, the call sites are not very readable. > Tools/WebKitTestRunner/TestController.cpp:2314 > + bool done; bool done { false }; > Tools/WebKitTestRunner/TestController.cpp:2315 > + bool result; bool result { false }; > Tools/WebKitTestRunner/TestController.cpp:2329 > + ResourceStatisticsCallbackContext context = { this, false, false }; ResourceStatisticsCallbackContext context(*this); > Tools/WebKitTestRunner/TestController.cpp:2345 > + ResourceStatisticsCallbackContext context = { this, false, false }; ResourceStatisticsCallbackContext context(*this); > Tools/WebKitTestRunner/TestController.cpp:2361 > + ResourceStatisticsCallbackContext context = { this, false, false }; ResourceStatisticsCallbackContext context(*this);

Comment on attachment 322806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322806&action=review >> Source/WebCore/platform/glib/FileMonitorGLib.cpp:38 >> + if (path.isEmpty() || !modificationHandler) > > This is a bad check. !modificationHandler will always be true since you WTFMove() it above.. > > I think this show this is not tested. This is supposed to be covered by Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp API test which is probably not enabled for GTK port? Oops good catch! :-P I'll enable the test too. >> Source/WebCore/platform/glib/FileMonitorGLib.cpp:60 >> + if (event == G_FILE_MONITOR_EVENT_DELETED) > > We may not want to call the callback if event == G_FILE_MONITOR_EVENT_ATTRIBUTE_CHANGED ? I was not sure, attribute change is a modification in the end.

(In reply to Chris Dumez from comment #26) > I would give this a constructor that takes a TestController& in parameter > and initializes both done / result to false. > ResourceStatisticsCallbackContext(TestController& testController) > : testController(&testController) > { } explicit

(In reply to Michael Catanzaro from comment #28) > (In reply to Chris Dumez from comment #26) > > I would give this a constructor that takes a TestController& in parameter > > and initializes both done / result to false. > > ResourceStatisticsCallbackContext(TestController& testController) > > : testController(&testController) > > { } > > explicit +1 :)

(In reply to Carlos Garcia Campos from comment #27) > Comment on attachment 322806 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322806&action=review > > >> Source/WebCore/platform/glib/FileMonitorGLib.cpp:38 > >> + if (path.isEmpty() || !modificationHandler) > > > > This is a bad check. !modificationHandler will always be true since you WTFMove() it above.. > > > > I think this show this is not tested. This is supposed to be covered by Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp API test which is probably not enabled for GTK port? > > Oops good catch! :-P I'll enable the test too. > > >> Source/WebCore/platform/glib/FileMonitorGLib.cpp:60 > >> + if (event == G_FILE_MONITOR_EVENT_DELETED) > > > > We may not want to call the callback if event == G_FILE_MONITOR_EVENT_ATTRIBUTE_CHANGED ? > > I was not sure, attribute change is a modification in the end. Fair enough.

Created attachment 322852 [details] Patch for landing I've addressed all review comments, added the FileMonitor test and fixed the impl to pass the test :-) Thanks Chris!

Created attachment 322853 [details] Patch for landing Read the explicit comment after submitting the patch, added too.

Committed r222962: <http://trac.webkit.org/changeset/222962>

<rdar://problem/34851362>