[GTK][WPE] All resource load statistics tests added in r212183 crash in GTK bots, timeout in GTK and WPE bots since r219049

[Carlos Garcia Campos]

It's crashing the UI process in ResourceLoadStatisticsStore::ensureResourceStatisticsForPrimaryDomain when calling HashMap::ensure(). It's weird, maybe it's yet another issue that only happens with GCC.

[John Wilander]

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

[John Wilander]

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.

[Carlos Garcia Campos]

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.

[John Wilander]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Charlie Turner]

Created attachment 315114 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315114 [details]
Patch

Clearing flags on attachment: 315114

Committed r219345: <http://trac.webkit.org/changeset/219345>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

Reopening: r219345 was a gardening commit

[Carlos Garcia Campos]

Created attachment 322538 [details]
Patch

[Build Bot]

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.

[Chris Dumez]

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.

[Build Bot]

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

[Build Bot]

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

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

Created attachment 322637 [details]
Fix the build

[Build Bot]

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.

[Michael Catanzaro]

(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...?

[Carlos Garcia Campos]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

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);

[Carlos Garcia Campos]

Created attachment 322806 [details]
Patch

[Chris Dumez]

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);

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

(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

[Chris Dumez]

(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 :)

[Chris Dumez]

(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.

[Carlos Garcia Campos]

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!

[Carlos Garcia Campos]

Created attachment 322853 [details]
Patch for landing

Read the explicit comment after submitting the patch, added too.

[Carlos Garcia Campos]

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

[Radar WebKit Bug Importer]

<rdar://problem/34851362>