[GTK][WPE] Add content extensions support in WKTR and unskip layout tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adrian Perez]

This is intended to be landed before the API for the GTK+ and WPE
ports (see bug #167941 for that), in order to make sure that the
layout tests for content extensions also can be run by these two
ports.

[Adrian Perez]

Created attachment 359655 [details]
Patch


Initial version of the patch for review, needs adding some
commentary on the ChangeLogs, and for some reason I am having
some troubles to run the tests for the WPE port (WKTR is not
cooperating well with Weston in my machine), so I will give
it another go tomorrow to mark more tests as passing for WPE.

Anyway, this is basically working well to run the tests for
the GTK+ port, with some flakyness, and soon I should have
them running for WPE as well. So I think that reviewin can
start :-)

[EWS Watchlist]

Attachment 359655 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/TestController.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/TestController.cpp:1333:  Declaration has space between type name and * in auto *context  [whitespace/declaration] [3]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 359655 [details]
Patch

All EWS are red!

[Adrian Perez]

(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 359655 [details]
> Patch
> 
> All EWS are red!

Indeed :\

I think it's a small bit related to the older compiler version that
the EWS bots are using. For the Cocoa bots, there's some files now
listed in Sources.txt which have to be removed from SourcesCocoa.txt
to avoid the error about the duplicated source file. I'll also fix
the style bot complaints before reuploading :)

[Adrian Perez]

Created attachment 359674 [details]
Patch v2

Should fix build in the EWS bots, and also make
the style checker happy. ChangeLog entries now have descriptions, too.

[Adrian Perez]

Created attachment 359676 [details]
Patch v3

Should fix the build for th GTK+ and WPE bots now.

[Adrian Perez]

(In reply to Adrian Perez from comment #6)
> Created attachment 359676 [details]
> Patch v3
> 
> Should fix the build for th GTK+ and WPE bots now.

The build failure for the Cocoa ports is due to the following:

  In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource41.cpp:5:
  /Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:49:56:
  error: 'operator new' is a protected member of 'API::ObjectImpl<API::Object::Type::ContentRuleListStore>'
      RefPtr<API::ContentRuleListStore> store = adoptRef(new API::ContentRuleListStore(toWTFString(path), false));
                                                         ^
  In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource41.cpp:1:
  In file included from /Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/API/C/WKResourceCacheManager.cpp:29:
  In file included from /Volumes/Data/EWS/WebKit/Source/WebKit/Shared/API/APIArray.h:29:
  /Volumes/Data/EWS/WebKit/Source/WebKit/Shared/API/APIObject.h:265:11: note: declared protected here
      void* operator new(size_t size) { return newObject(size, APIType); }
            ^
  1 error generated.

I'll see to it and upload and updated patch.

[Adrian Perez]

Created attachment 359685 [details]
Patch v4


This version should solve the build issue for the Cocoa ports.

[Adrian Perez]

Created attachment 359687 [details]
Patch v5

[Adrian Perez]

Created attachment 359689 [details]
Patch v6

Avoids including WKAPICast.h in TestController.cpp
(which is unneeded anyway).

[EWS Watchlist]

Comment on attachment 359689 [details]
Patch v6

Attachment 359689 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10829510

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 359690 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 359689 [details]
Patch v6

Attachment 359689 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10829463

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 359691 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Adrian Perez]

Ping reviewers :)

[Michael Catanzaro]

(In reply to Build Bot from comment #13)
> Comment on attachment 359689 [details]
> Patch v6
> 
> Attachment 359689 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/10829463
> 
> Number of test failures exceeded the failure limit.

I think you broke WebKitTestRunner on the ios-sim and mac-wk2 EWS.

[Loïc Yhuel]

Comment on attachment 359689 [details]
Patch v6

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:37
> +#include <gio/gfiledescriptorbased.h>

I didn't try to build this patch on trunk (I'm testing on WPE 2.20), so I may have missed something, but how do you get GIO_UNIX_INCLUDE_DIRS here ?
I only see it in WebCore platform code.

> Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:133
> +#if USE(GLIB)

Shouldn't this be "#if USE(GLIB) && !PLATFORM(WIN)" to match the definition in Source/WebCore/platform/FileSystem.h ?
I'm not sure this source could be built on Windows, but using soup without USE(GLIB) probably wouldn't work either.

> Source/WebKit/UIProcess/API/glib/APIContentRuleListStoreGLib.cpp:31
> +String ContentRuleListStore::defaultStorePath(bool)

#if ENABLE(CONTENT_EXTENSIONS) is needed around the function definition, since the class isn't defined without the flag.

> Source/cmake/OptionsGTK.cmake:153
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CONTENT_EXTENSIONS PRIVATE ON)

It looks like the options should be in alphabetical order.

[Carlos Garcia Campos]

Comment on attachment 359689 [details]
Patch v6

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:139
> +Data adoptAndMapFile(GFileIOStream* stream, size_t offset, size_t size)
> +{
> +    GInputStream* inputStream = g_io_stream_get_input_stream(G_IO_STREAM(stream));
> +    int fd = g_file_descriptor_based_get_fd(G_FILE_DESCRIPTOR_BASED(inputStream));
> +    return adoptAndMapFile(fd, offset, size);
> +}

I find a bit weird that APIContentRuleListStore.cpp from UI process uses NetworekDataCache from the Network process. Would it be possible to use SharedBuffer::createWithContentsOfFile() instead? Otherwise I wonder if it would be better to add FileSystem::fileDescriptor(PlatformFileHandle); with this impl for glib based ports and use it in  APIContentRuleListStore.cpp.

> Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:60
> +    if (error) {

Ww could use an early return here if !error

> Tools/WebKitTestRunner/TestController.cpp:1095
> -        return std::string(buffer.data(), length);
> +        return std::string(buffer.data(), length - 1);

Can length be 0 here?

> Tools/WebKitTestRunner/TestController.cpp:1328
> +    WKRetainPtr<WKUserContentFilterRef> filter { nullptr };

No need to initialize to nullptr here, since that's the default of WKRetainPtr.

> Tools/WebKitTestRunner/TestController.cpp:1341
> +static std::string contentExtensionJsonPath(WKURLRef url)

Json -> JSON?

[Carlos Garcia Campos]

Comment on attachment 359689 [details]
Patch v6

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

> Tools/WebKitTestRunner/TestController.cpp:1414
> +    auto userContentController = adoptWK(WKPageConfigurationGetUserContentController(pageConfiguration.get()));
> +    ASSERT(userContentController);
> +
> +    WKUserContentControllerRemoveAllUserContentFilters(userContentController.get());

I think the problem in cocoa is that userContentController is nullptr here. Note that user content controller is set with WKPageConfigurationSetUserContentController in TestController::generatePageConfiguration(). The generated page configuration is then passed to platformCreateWebView() and the cocoa implementation simply ignores the given WKPageConfigurationRef and used a global WKWebViewConfiguration to create the PlatformWebView.

[Adrian Perez]

(In reply to Loïc Yhuel from comment #17)
> Comment on attachment 359689 [details]
> Patch v6
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359689&action=review
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:37
> > +#include <gio/gfiledescriptorbased.h>
> 
> I didn't try to build this patch on trunk (I'm testing on WPE 2.20), so I
> may have missed something, but how do you get GIO_UNIX_INCLUDE_DIRS here ?
> I only see it in WebCore platform code.

This is added for WebCore in “Source/WebCore/Platform{GTK,WPE}.cmake”,
and CMake propagates the build flags because the main WebKit library
depends on the WebCore one :)

> > Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:133
> > +#if USE(GLIB)
> 
> Shouldn't this be "#if USE(GLIB) && !PLATFORM(WIN)" to match the definition
> in Source/WebCore/platform/FileSystem.h ?
> I'm not sure this source could be built on Windows, but using soup without
> USE(GLIB) probably wouldn't work either.

Good point, I will be updating the patch to use this condition for the
guards (also above for the inclusion of “gfiledescriptorbased.h“, which
won't work in Windows either).

> > Source/WebKit/UIProcess/API/glib/APIContentRuleListStoreGLib.cpp:31
> > +String ContentRuleListStore::defaultStorePath(bool)
> 
> #if ENABLE(CONTENT_EXTENSIONS) is needed around the function definition,
> since the class isn't defined without the flag.

Or even better: guard building of the file altogether by using

  #if ENABLE_CONTENT_EXTENSIONS
  UIProcess/API/glib/APIContentRuleListStoreGLib.cpp @no-unify
  #endif

in the “Sources{GTK,WPE}.cmake” files -- one less file to build if the
feature is disabled!

> > Source/cmake/OptionsGTK.cmake:153
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CONTENT_EXTENSIONS PRIVATE ON)
> 
> It looks like the options should be in alphabetical order.

Indeed, I'll reorder those.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #18)
> Comment on attachment 359689 [details]
> Patch v6
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359689&action=review
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:139
> > +Data adoptAndMapFile(GFileIOStream* stream, size_t offset, size_t size)
> > +{
> > +    GInputStream* inputStream = g_io_stream_get_input_stream(G_IO_STREAM(stream));
> > +    int fd = g_file_descriptor_based_get_fd(G_FILE_DESCRIPTOR_BASED(inputStream));
> > +    return adoptAndMapFile(fd, offset, size);
> > +}
> 
> I find a bit weird that APIContentRuleListStore.cpp from UI process uses
> NetworekDataCache from the Network process. Would it be possible to use
> SharedBuffer::createWithContentsOfFile() instead? Otherwise I wonder if it
> would be better to add FileSystem::fileDescriptor(PlatformFileHandle); with
> this impl for glib based ports and use it in  APIContentRuleListStore.cpp.

My guess is that NetworkCache::Data was picked here was because of is
utility methods, which are used all over the place. While it looks like
it would be possible to refactor things around, I would rather do it in
a separate bug, and not at this moment where we are close to branching
for 2.24.x -- it seems risky if we want 2.24 to include content extension
support.

> > Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:60
> > +    if (error) {
> 
> Ww could use an early return here if !error

Will do.

> > Tools/WebKitTestRunner/TestController.cpp:1095
> > -        return std::string(buffer.data(), length);
> > +        return std::string(buffer.data(), length - 1);
> 
> Can length be 0 here?

No. Passing just “length” here is wrong, and took me a couple of days
to figure out this was the reason why paths were not being constructed
correctly: the calculated “length” *includes* one byte for the null
terminator ('\0'), so passing “length - 1” ensures that the null
terminator does NOT end up as part of the created std::string.

Let's say we have a WKStringRef which contains “foo”:

 1. The “length” will be 4 (three characters + null terminator).

 2. The created std::string will have contents “foo\0”

 3. When appending to it, for example to add the “.json” suffix, we
    end up with a new std:string with contents “foo\0.json”.

 4. Using printf("%s\n", fooWithSuffix.c_str()), the returned value
    points to a buffer which contains “foo\0.json\0” because the
    std::string::c_str() method adds another null terminator, but
    only “foo” is printed because C string functions will see only
    the data buffer until the first null terminator!

> > Tools/WebKitTestRunner/TestController.cpp:1328
> > +    WKRetainPtr<WKUserContentFilterRef> filter { nullptr };
> 
> No need to initialize to nullptr here, since that's the default of
> WKRetainPtr.

Indeed, thanks.

> > Tools/WebKitTestRunner/TestController.cpp:1341
> > +static std::string contentExtensionJsonPath(WKURLRef url)
> 
> Json -> JSON?

Yes, JSON seems to be more in tune with the style of the rest
of the WebKit code. I'll change this.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #19)
> Comment on attachment 359689 [details]
> Patch v6
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359689&action=review
> 
> > Tools/WebKitTestRunner/TestController.cpp:1414
> > +    auto userContentController = adoptWK(WKPageConfigurationGetUserContentController(pageConfiguration.get()));
> > +    ASSERT(userContentController);
> > +
> > +    WKUserContentControllerRemoveAllUserContentFilters(userContentController.get());
> 
> I think the problem in cocoa is that userContentController is nullptr here.
> Note that user content controller is set with
> WKPageConfigurationSetUserContentController in
> TestController::generatePageConfiguration(). The generated page
> configuration is then passed to platformCreateWebView() and the cocoa
> implementation simply ignores the given WKPageConfigurationRef and used a
> global WKWebViewConfiguration to create the PlatformWebView.

Ouch, ouch! It looks like this can be solved by adding a
TestController::userContentController() accessor, which for the
GTK/WPE ports will use the current code (that is: picking it from
the page configuration) and for Cocoa it picks the object from the
global WKWebViewConfiguration somehow.

[Adrian Perez]

Created attachment 360393 [details]
Patch v7


The Cocoa port uses a global WKWebViewConfiguration. Add an accesor
TestController::userContentController() which picks the user content
controller from the global for the Cocoa port, and from the current
page configuration for the other ports.

This also addresses the rest of review comments by Carlos García and
Loïc Yhuel (thanks!)

[EWS Watchlist]

Comment on attachment 360393 [details]
Patch v7

Attachment 360393 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10930350

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360405 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Adrian Perez]

(In reply to Adrian Perez from comment #23)
> Created attachment 360393 [details]
> Patch v7
>
> The Cocoa port uses a global WKWebViewConfiguration. Add an accesor
> TestController::userContentController() which picks the user content
> controller from the global for the Cocoa port, and from the current
> page configuration for the other ports.

That didn't cut it either, *sigh* :|

I am getting tempted to revert the changes to TestControllerCocoa.mm,
and leave it to use the previous existing code to configure content
extensions, and have the new bits in TestController.cpp in between
#if !PLATFORM(COCOA) guards; then unify the Cocoa and non-Cocoa setup
for content extensions in a follow-up patch with cleanups. At least
that would get this unblocked... Opinions?

[EWS Watchlist]

Comment on attachment 360393 [details]
Patch v7

Attachment 360393 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10935055

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360450 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Carlos Garcia Campos]

Comment on attachment 359689 [details]
Patch v6

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

>>> Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:139
>>> +}
>> 
>> I find a bit weird that APIContentRuleListStore.cpp from UI process uses NetworekDataCache from the Network process. Would it be possible to use SharedBuffer::createWithContentsOfFile() instead? Otherwise I wonder if it would be better to add FileSystem::fileDescriptor(PlatformFileHandle); with this impl for glib based ports and use it in  APIContentRuleListStore.cpp.
> 
> My guess is that NetworkCache::Data was picked here was because of is
> utility methods, which are used all over the place. While it looks like
> it would be possible to refactor things around, I would rather do it in
> a separate bug, and not at this moment where we are close to branching
> for 2.24.x -- it seems risky if we want 2.24 to include content extension
> support.

I agree, but I still think the code to get a fd from a PlatformFileHandle doesn't belong here, but in Filesystem.

>>> Tools/WebKitTestRunner/TestController.cpp:1095
>>> +        return std::string(buffer.data(), length - 1);
>> 
>> Can length be 0 here?
> 
> No. Passing just “length” here is wrong, and took me a couple of days
> to figure out this was the reason why paths were not being constructed
> correctly: the calculated “length” *includes* one byte for the null
> terminator ('\0'), so passing “length - 1” ensures that the null
> terminator does NOT end up as part of the created std::string.
> 
> Let's say we have a WKStringRef which contains “foo”:
> 
>  1. The “length” will be 4 (three characters + null terminator).
> 
>  2. The created std::string will have contents “foo\0”
> 
>  3. When appending to it, for example to add the “.json” suffix, we
>     end up with a new std:string with contents “foo\0.json”.
> 
>  4. Using printf("%s\n", fooWithSuffix.c_str()), the returned value
>     points to a buffer which contains “foo\0.json\0” because the
>     std::string::c_str() method adds another null terminator, but
>     only “foo” is printed because C string functions will see only
>     the data buffer until the first null terminator!

That was not my question. I wonder if length can be 0, because then we will end up passing -1 to std::string().

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #26)
> (In reply to Adrian Perez from comment #23)
> > Created attachment 360393 [details]
> > Patch v7
> >
> > The Cocoa port uses a global WKWebViewConfiguration. Add an accesor
> > TestController::userContentController() which picks the user content
> > controller from the global for the Cocoa port, and from the current
> > page configuration for the other ports.
> 
> That didn't cut it either, *sigh* :|
> 
> I am getting tempted to revert the changes to TestControllerCocoa.mm,
> and leave it to use the previous existing code to configure content
> extensions, and have the new bits in TestController.cpp in between
> #if !PLATFORM(COCOA) guards; then unify the Cocoa and non-Cocoa setup
> for content extensions in a follow-up patch with cleanups. At least
> that would get this unblocked... Opinions?

Sounds good.

[Loïc Yhuel]

(In reply to Carlos Garcia Campos from comment #18)
> I find a bit weird that APIContentRuleListStore.cpp from UI process uses
> NetworekDataCache from the Network process. Would it be possible to use
> SharedBuffer::createWithContentsOfFile() instead?
SharedBuffer::createWithContentsOfFile() would reopen the file, perhaps a new SharedBuffer::createFromFile(PlatformFileHandle&&).

(In reply to Adrian Perez from comment #20)
> (In reply to Loïc Yhuel from comment #17)
> > Comment on attachment 359689 [details]
> > Patch v6
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=359689&action=review
> > 
> > > Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:37
> > > +#include <gio/gfiledescriptorbased.h>
> > 
> > I didn't try to build this patch on trunk (I'm testing on WPE 2.20), so I
> > may have missed something, but how do you get GIO_UNIX_INCLUDE_DIRS here ?
> > I only see it in WebCore platform code.
> 
> This is added for WebCore in “Source/WebCore/Platform{GTK,WPE}.cmake”,
> and CMake propagates the build flags because the main WebKit library
> depends on the WebCore one :)
> 
But GIO_UNIX_INCLUDE_DIRS is in WebCore_SYSTEM_INCLUDE_DIRECTORIES, and the WEBKIT_FRAMEWORK macro declares those as PRIVATE.

[Adrian Perez]

(In reply to Loïc Yhuel from comment #31)
> (In reply to Carlos Garcia Campos from comment #18)
> > I find a bit weird that APIContentRuleListStore.cpp from UI process uses
> > NetworekDataCache from the Network process. Would it be possible to use
> > SharedBuffer::createWithContentsOfFile() instead?
> SharedBuffer::createWithContentsOfFile() would reopen the file, perhaps a
> new SharedBuffer::createFromFile(PlatformFileHandle&&).

In my mind the suggestion to add a SharedBuffer::createFromFile()
sounds the better. Would it be okay to over to that later on as a
follow-up patch?

> (In reply to Adrian Perez from comment #20)
> > (In reply to Loïc Yhuel from comment #17)
> > > Comment on attachment 359689 [details]
> > > Patch v6
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=359689&action=review
> > > 
> > > > Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:37
> > > > +#include <gio/gfiledescriptorbased.h>
> > > 
> > > I didn't try to build this patch on trunk (I'm testing on WPE 2.20), so I
> > > may have missed something, but how do you get GIO_UNIX_INCLUDE_DIRS here ?
> > > I only see it in WebCore platform code.
> > 
> > This is added for WebCore in “Source/WebCore/Platform{GTK,WPE}.cmake”,
> > and CMake propagates the build flags because the main WebKit library
> > depends on the WebCore one :)
> > 
> But GIO_UNIX_INCLUDE_DIRS is in WebCore_SYSTEM_INCLUDE_DIRECTORIES, and the
> WEBKIT_FRAMEWORK macro declares those as PRIVATE.

I'll try to take a look, but definitely the build works fine for both
GTK+ and WPE ports, and the bots seem to agree. ~_~

[Adrian Perez]

Created attachment 360861 [details]
Patch v8


Reverted the Cocoa/Mac bits, hopefully this will keep the EWS bots
happy and we can get this landed; then later on do a follow-up patch
to make Cocoa use the C API as well.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #29)
> Comment on attachment 359689 [details]
> Patch v6
>
> [...]
>
> >>> Tools/WebKitTestRunner/TestController.cpp:1095
> >>> +        return std::string(buffer.data(), length - 1);
> >> 
> >> Can length be 0 here?
> > 
> > No. Passing just “length” here is wrong, and took me a couple of days
> > to figure out this was the reason why paths were not being constructed
> > correctly: the calculated “length” *includes* one byte for the null
> > terminator ('\0'), so passing “length - 1” ensures that the null
> > terminator does NOT end up as part of the created std::string.
> > 
> > Let's say we have a WKStringRef which contains “foo”:
> > 
> >  1. The “length” will be 4 (three characters + null terminator).
> > 
> >  2. The created std::string will have contents “foo\0”
> > 
> >  3. When appending to it, for example to add the “.json” suffix, we
> >     end up with a new std:string with contents “foo\0.json”.
> > 
> >  4. Using printf("%s\n", fooWithSuffix.c_str()), the returned value
> >     points to a buffer which contains “foo\0.json\0” because the
> >     std::string::c_str() method adds another null terminator, but
> >     only “foo” is printed because C string functions will see only
> >     the data buffer until the first null terminator!
> 
> That was not my question. I wonder if length can be 0, because then we will
> end up passing -1 to std::string().

Very good question. I have been assuming that for an empty string
then WKStringGetMaximumUTF8CStringSize() would return “1”, so then
there's at least one byte where to place the null-terminator. I'll
double check before landing.

[Adrian Perez]

(In reply to Adrian Perez from comment #34)
> (In reply to Carlos Garcia Campos from comment #29)
> > Comment on attachment 359689 [details]
> > Patch v6
> >
> > [...]
> >
> > >>> Tools/WebKitTestRunner/TestController.cpp:1095
> > >>> +        return std::string(buffer.data(), length - 1);
> > >> 
> > >> Can length be 0 here?
> > > 
> > > No. Passing just “length” here is wrong, and took me a couple of days
> > > to figure out this was the reason why paths were not being constructed
> > > correctly: the calculated “length” *includes* one byte for the null
> > > terminator ('\0'), so passing “length - 1” ensures that the null
> > > terminator does NOT end up as part of the created std::string.
> > > 
> > > Let's say we have a WKStringRef which contains “foo”:
> > > 
> > >  1. The “length” will be 4 (three characters + null terminator).
> > > 
> > >  2. The created std::string will have contents “foo\0”
> > > 
> > >  3. When appending to it, for example to add the “.json” suffix, we
> > >     end up with a new std:string with contents “foo\0.json”.
> > > 
> > >  4. Using printf("%s\n", fooWithSuffix.c_str()), the returned value
> > >     points to a buffer which contains “foo\0.json\0” because the
> > >     std::string::c_str() method adds another null terminator, but
> > >     only “foo” is printed because C string functions will see only
> > >     the data buffer until the first null terminator!
> > 
> > That was not my question. I wonder if length can be 0, because then we will
> > end up passing -1 to std::string().
> 
> Very good question. I have been assuming that for an empty string
> then WKStringGetMaximumUTF8CStringSize() would return “1”, so then
> there's at least one byte where to place the null-terminator. I'll
> double check before landing.

Indeed, the function to get the size of the bufer always adds one extra
for the null terminator:

  size_t WKStringGetMaximumUTF8CStringSize(WKStringRef stringRef)
  {
      return WebKit::toImpl(stringRef)->stringView().length() * 3 + 1;
  }

As for the “length” returned by WKStringGetUTF8CString(), it calls
an internal function which end like this:

  size_t WKStringGetUTF8CStringImpl(...)
  {
      /* ... */

      *p++ = '\0';
      return p - buffer;
  }

The “p” pointer stores has the address of the null terminator *plus one*,
which means that (p - buffer) returns 1 for an empty string. There are two
cases in which it can return zero, though: when the “bufferSize” parameter
passed to WKStringGetUTF8CString() is zero, or when conversion to UTF-8
fails -- and we should not be hitting any of those in WKTR, ever :)

I can add a check for “length > 0” (an assertion, maybe?) if you think
that would make things clearer.

[Michael Catanzaro]

Comment on attachment 360861 [details]
Patch v8

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

This needs owner approval to land, so please find an owner to approve it.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:241
> -        processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler), originalRequest = WTFMove(originalRequest)](auto result) mutable {
> +        this->processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler), originalRequest = WTFMove(originalRequest)](auto result) mutable {

You've already captured this. Why are you adding this here? Is it a bug in old GCC?

> Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:49
> +    RefPtr<API::ContentRuleListStore> store(API::ContentRuleListStore::storeWithPath(toWTFString(path), false));

I've never understood the C/C++ APIs... do you really not need to adopt the ref?

> Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.h:48
> +
> +

Is the style really two blank lines?

> Tools/Scripts/webkitperl/FeatureList.pm:65
> +    $contentExtensionsSupport,
>      $cloopSupport,

It belongs under cloop.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #36)
> Comment on attachment 360861 [details]
> Patch v8
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360861&action=review
> 
> This needs owner approval to land, so please find an owner to approve it.

Sure :)

> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:241
> > -        processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler), originalRequest = WTFMove(originalRequest)](auto result) mutable {
> > +        this->processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler), originalRequest = WTFMove(originalRequest)](auto result) mutable {
> 
> You've already captured this. Why are you adding this here? Is it a bug in
> old GCC?

Seems to be a bug in some versions of GCC (for example the version of
GCC6 in some EWS bots, and IIRC the one in the Ubuntu LTS one), which
will complain that the “processContentExtensionRulesForLoad()” method
is not defined in the current scope ¯\_(ツ)_/¯ 

> > Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:49
> > +    RefPtr<API::ContentRuleListStore> store(API::ContentRuleListStore::storeWithPath(toWTFString(path), false));
> 
> I've never understood the C/C++ APIs... do you really not need to adopt the
> ref?

There is no need to adopt. WTF::String itself is always stack-allocated,
so if you ever see a  RefPtr<String>... be suspicious.

(Longer explanation: the WTF::String contains internally a RefPtr<StringImpl>
member, and that is what gets used so multiple WTF::String instances can
share the same underlying contents; destroying and creating WTF::String
instances e.g. when passing copies to functions just changes the refcount
of the internal member.)

> > Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.h:48
> > +
> > +
> 
> Is the style really two blank lines?

Not really, but somehow the style checker didn't complain. I'll make
this just one extra empty line go away. Reading other files in the same
directory I see that usually the “typedef” is above the “enum” so I'll
change that as well.

> > Tools/Scripts/webkitperl/FeatureList.pm:65
> > +    $contentExtensionsSupport,
> >      $cloopSupport,
> 
> It belongs under cloop.

Yup, will change.

[Adrian Perez]

(In reply to Adrian Perez from comment #35)
 
> As for the “length” returned by WKStringGetUTF8CString() [...]
> 
> I can add a check for “length > 0” (an assertion, maybe?) if you think
> that would make things clearer.

The next version of the patch will have an assertion here to make
it explicit that the expectation is that the length is non-zero.

[Adrian Perez]

Created attachment 361013 [details]
Patch v9


This just contains a few minor edits: an assertion added in TestController.cpp
and a couple of for style issues not caught by the style checker.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #36)

> Comment on attachment 360861 [details]
> Patch v8
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360861&action=review
> 
> This needs owner approval to land, so please find an owner to approve it.

I had already Alex Christensen in CC, plus now added Chris Dumez
and Darin Adler who have been also making fixes and code reviews
for content extensions.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #37)
> > > Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:49
> > > +    RefPtr<API::ContentRuleListStore> store(API::ContentRuleListStore::storeWithPath(toWTFString(path), false));
> > 
> > I've never understood the C/C++ APIs... do you really not need to adopt the
> > ref?
> 
> There is no need to adopt. WTF::String itself is always stack-allocated,
> so if you ever see a  RefPtr<String>... be suspicious.

I meant: doesn't the result of API::ContentRuleListStore::storeWithPath already have a ref? Then the RefPtr takes a second ref, right? Then when you leakRef it on the next line, there are two outstanding? Instead of the one you expect?

[Michael Catanzaro]

Have you investigated why so many of the tests are crashing?

[Adrian Perez]

(In reply to Michael Catanzaro from comment #41)
> (In reply to Adrian Perez from comment #37)
> > > > Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:49
> > > > +    RefPtr<API::ContentRuleListStore> store(API::ContentRuleListStore::storeWithPath(toWTFString(path), false));
> > > 
> > > I've never understood the C/C++ APIs... do you really not need to adopt the
> > > ref?
> > 
> > There is no need to adopt. WTF::String itself is always stack-allocated,
> > so if you ever see a  RefPtr<String>... be suspicious.
> 
> I meant: doesn't the result of API::ContentRuleListStore::storeWithPath
> already have a ref? Then the RefPtr takes a second ref, right? Then when you
> leakRef it on the next line, there are two outstanding? Instead of the one
> you expect?

Ahh, I get it know. You are absolute right, this has to be changed.
Looking around the other source files in the directory, it seems that
the usual is directly returning without even keeping a RefPtr<> around
for one-liner constructors. Therefore I am going to change this into:

   return toAPI(API::ContentRuleListStore::storeWithPath(toWTFString(path), false);

[Adrian Perez]

(In reply to Adrian Perez from comment #43)
> (In reply to Michael Catanzaro from comment #41)
>
>> [...]
> 
> Ahh, I get it know. You are absolute right, this has to be changed.
> Looking around the other source files in the directory, it seems that
> the usual is directly returning without even keeping a RefPtr<> around
> for one-liner constructors. Therefore I am going to change this into:
> 
>    return toAPI(API::ContentRuleListStore::storeWithPath(toWTFString(path),
> false);

Correction:

  return toAPI(&API::ContentRuleListStore::storeWithPath(toWTFString(path), false).leakRef());

(:

[Adrian Perez]

Created attachment 361047 [details]
Patch v10


Fixes the WKUserContentExtensionStoreCreate() function to return new
objects with only one ref in them.

[Loïc Yhuel]

(In reply to Adrian Perez from comment #32)
> (In reply to Loïc Yhuel from comment #31)
> > (In reply to Adrian Perez from comment #20)
> > > (In reply to Loïc Yhuel from comment #17)
> > > > Comment on attachment 359689 [details]
> > > > Patch v6
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=359689&action=review
> > > > 
> > > > > Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp:37
> > > > > +#include <gio/gfiledescriptorbased.h>
> > > > 
> > > > I didn't try to build this patch on trunk (I'm testing on WPE 2.20), so I
> > > > may have missed something, but how do you get GIO_UNIX_INCLUDE_DIRS here ?
> > > > I only see it in WebCore platform code.
> > > 
> > > This is added for WebCore in “Source/WebCore/Platform{GTK,WPE}.cmake”,
> > > and CMake propagates the build flags because the main WebKit library
> > > depends on the WebCore one :)
> > > 
> > But GIO_UNIX_INCLUDE_DIRS is in WebCore_SYSTEM_INCLUDE_DIRECTORIES, and the
> > WEBKIT_FRAMEWORK macro declares those as PRIVATE.
> 
> I'll try to take a look, but definitely the build works fine for both
> GTK+ and WPE ports, and the bots seem to agree. ~_~

In fact :
 - for GTK, it's already indirectly in GTK_INCLUDE_DIRS and GTK_INCLUDE_DIRS
 - for WPE, it only relies on WebCore_SYSTEM_INCLUDE_DIRECTORIES being in WebCoreHeaderInterface, we is IHMO a little overkill (but since there are already quite a lot of include paths, it probably doesn't really matter)

[Michael Catanzaro]

Comment on attachment 361047 [details]
Patch v10

r=me is conditional on getting owner approval.

I do wish we knew why there is a huge amount of broken tests, but this commit just makes it possible to test, so no reason to delay on that.

[youenn fablet]

Comment on attachment 361047 [details]
Patch v10

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

> Tools/WebKitTestRunner/TestController.cpp:835
> +    m_serverTrustEvaluationCallbackCallsCount = 0;

Is this line needed?

> Tools/WebKitTestRunner/TestController.cpp:836
> +    resetContentExtensions();

Cocoa WTR currently reset these in platformResetStateToConsistentValues.
I guess we should try to align both approaches.
For instance, resetContentExtensions() could be called from resetStateToConsistentValues and cocoa would move its code to resetContentExtensions(), in this patch or as a follow-up.

> LayoutTests/platform/wpe/TestExpectations:1267
> +http/tests/contentextensions/async-xhr-onerror.html [ Crash ]

What is causing all these tests to crash?

[Alex Christensen]

Comment on attachment 361047 [details]
Patch v10

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

> Source/WebKit/SourcesGTK.txt:127
> +#if ENABLE_CONTENT_EXTENSIONS
> +UIProcess/API/glib/APIContentRuleListStoreGLib.cpp @no-unify

Just unconditionally include the file.  Preprocessor-like things seem out of place in this file.

> Source/WebKit/SourcesWPE.txt:112
> +#if ENABLE_CONTENT_EXTENSIONS

ditto

> Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:33
> +#include "WKRetainPtr.h"
> +#include <wtf/Function.h>

These don't seem used.

> LayoutTests/platform/gtk/TestExpectations:126
> +# Content extensions, the failures need up-to-date expectations. The crashing tests do so in a flaky way.

You should probably fix these "flaky crashes" before landing it.  Check that your memory mapped file is not being unmapped somehow.

[Loïc Yhuel]

(In reply to Alex Christensen from comment #49)
> Comment on attachment 361047 [details]
> Patch v10
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361047&action=review
> 
> > Source/WebKit/SourcesGTK.txt:127
> > +#if ENABLE_CONTENT_EXTENSIONS
> > +UIProcess/API/glib/APIContentRuleListStoreGLib.cpp @no-unify
> 
> Just unconditionally include the file.  Preprocessor-like things seem out of
> place in this file.
So you prefer an "#if ENABLE(CONTENT_EXTENSIONS)" in the source ?

[Michael Catanzaro]

I suppose. We don't really have a rule, but I agree the build system is easier to maintain if we build most files unconditionally and just have appropriate include guards in the source.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #51)
> I suppose. We don't really have a rule, but I agree the build system is
> easier to maintain if we build most files unconditionally and just have
> appropriate include guards in the source.

The reasons why I thought that it would be better to have the conditionals
inside Sources{GTK,WPE}.txt is that, when the feature is disabled, there
will be one less compiler job to launch: multiple fork+exec saved, which
means there is no need to the read source file and its chain of #includes
and expensive template parsing, and no need to generate a .o file.

That being said, I am fine putting back the ENABLE() guards in the .cpp
file -- I don't have a strong preference.

[Adrian Perez]

(In reply to youenn fablet from comment #48)
> Comment on attachment 361047 [details]
> Patch v10
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361047&action=review
> 
> > Tools/WebKitTestRunner/TestController.cpp:835
> > +    m_serverTrustEvaluationCallbackCallsCount = 0;
> 
> Is this line needed?

Definitely not needed. This slipped when making the rebase, I'll
remove it.

> > Tools/WebKitTestRunner/TestController.cpp:836
> > +    resetContentExtensions();
> 
> Cocoa WTR currently reset these in platformResetStateToConsistentValues.
> I guess we should try to align both approaches.
> For instance, resetContentExtensions() could be called from
> resetStateToConsistentValues and cocoa would move its code to
> resetContentExtensions(), in this patch or as a follow-up.

Sounds like a good idea. I'll give it a try to the part of moving
the Cocoa part to resetContentExtensions() in ./cocoa/TestController.cpp,
inside PLATFORM(COCOA) guards, and keep the shared WPE/GTK+ version
in ./TestController.cpp

The part of making the Cocoa implementation use the C API to make
the three ports share the same resetContentExtensions() is something
that I would still rather do in a follow-up.

> > LayoutTests/platform/wpe/TestExpectations:1267
> > +http/tests/contentextensions/async-xhr-onerror.html [ Crash ]
> 
> What is causing all these tests to crash?

I'll try and have another look at it.

[Adrian Perez]

(In reply to Alex Christensen from comment #49)
> Comment on attachment 361047 [details]
> Patch v10
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361047&action=review
> 
> > Source/WebKit/SourcesGTK.txt:127
> > +#if ENABLE_CONTENT_EXTENSIONS
> > +UIProcess/API/glib/APIContentRuleListStoreGLib.cpp @no-unify
> 
> Just unconditionally include the file.  Preprocessor-like things seem out of
> place in this file.

Okay :)

> > Source/WebKit/SourcesWPE.txt:112
> > +#if ENABLE_CONTENT_EXTENSIONS
> 
> ditto
> 
> > Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:33
> > +#include "WKRetainPtr.h"
> > +#include <wtf/Function.h>
> 
> These don't seem used.

Indeed. These are leftovers from from a previous iteration of the
patch, I'll remove them.

> > LayoutTests/platform/gtk/TestExpectations:126
> > +# Content extensions, the failures need up-to-date expectations. The crashing tests do so in a flaky way.
> 
> You should probably fix these "flaky crashes" before landing it.  Check that
> your memory mapped file is not being unmapped somehow.

[Adrian Perez]

Created attachment 361443 [details]
Patch v11


This version of the patch addresses all the reviews by owners.

I am still taking a look to see if I can figure out the crashes
from the layout tests.

[Adrian Perez]

Created attachment 361449 [details]
Patch v12


Attempt to fix the Cocoa build.

[Adrian Perez]

Created attachment 361454 [details]
Patch v13

[Michael Catanzaro]

EWS is red.

[Adrian Perez]

Created attachment 361612 [details]
Patch v14


Should fix the linker errors for the Cocoa and WinCairo EWS bots.

[Adrian Perez]

Created attachment 361626 [details]
Patch v15


Should fix the type conversion error for GTK EWS bot. It turns out
that including <wtf/Function.h> was needed after all for some older
GCC versions.

[Michael Catanzaro]

../../Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:79:6: error: no matching function for call to ‘API::ContentRuleListStore::compileContentRuleList(WTF::String, WTF::String, WKUserContentExtensionStoreCompile(WKUserContentExtensionStoreRef, WKStringRef, WKStringRef, void*, WKUserContentExtensionStoreFunction)::<lambda(WTF::RefPtr<API::ContentRuleList>, std::error_code)>)’
     });
      ^

[Adrian Perez]

(In reply to Michael Catanzaro from comment #61)
> ../../Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:79:6:
> error: no matching function for call to
> ‘API::ContentRuleListStore::compileContentRuleList(WTF::String, WTF::String,
> WKUserContentExtensionStoreCompile(WKUserContentExtensionStoreRef,
> WKStringRef, WKStringRef, void*,
> WKUserContentExtensionStoreFunction)::<lambda(WTF::RefPtr<API::
> ContentRuleList>, std::error_code)>)’
>      });
>       ^

I saw... This used to build fine with previous versions of the patch, and
definitely builds fine for me here. My suspicion is that it is yet another
Heisenbug “thanks to” the unified sources -- which I am slowly getting to
hate, one build failure at a time ¯\_(ツ)_/¯

[Adrian Perez]

(In reply to Adrian Perez from comment #62)
> (In reply to Michael Catanzaro from comment #61)
> > ../../Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:79:6:
> > error: no matching function for call to
> > ‘API::ContentRuleListStore::compileContentRuleList(WTF::String, WTF::String,
> > WKUserContentExtensionStoreCompile(WKUserContentExtensionStoreRef,
> > WKStringRef, WKStringRef, void*,
> > WKUserContentExtensionStoreFunction)::<lambda(WTF::RefPtr<API::
> > ContentRuleList>, std::error_code)>)’
> >      });
> >       ^
> 
> I saw... This used to build fine with previous versions of the patch, and
> definitely builds fine for me here. My suspicion is that it is yet another
> Heisenbug “thanks to” the unified sources -- which I am slowly getting to
> hate, one build failure at a time ¯\_(ツ)_/¯

Bingo! Adding “@no-unify” for this source file makes the compiler complain
locally as well, so at least I can try and figure out the reason easily
now :)

[Adrian Perez]

(In reply to Adrian Perez from comment #63)
> (In reply to Adrian Perez from comment #62)
> > (In reply to Michael Catanzaro from comment #61)
> > > ../../Source/WebKit/UIProcess/API/C/WKUserContentExtensionStoreRef.cpp:79:6:
> > > error: no matching function for call to
> > > ‘API::ContentRuleListStore::compileContentRuleList(WTF::String, WTF::String,
> > > WKUserContentExtensionStoreCompile(WKUserContentExtensionStoreRef,
> > > WKStringRef, WKStringRef, void*,
> > > WKUserContentExtensionStoreFunction)::<lambda(WTF::RefPtr<API::
> > > ContentRuleList>, std::error_code)>)’
> > >      });
> > >       ^
> > 
> > I saw... This used to build fine with previous versions of the patch, and
> > definitely builds fine for me here. My suspicion is that it is yet another
> > Heisenbug “thanks to” the unified sources -- which I am slowly getting to
> > hate, one build failure at a time ¯\_(ツ)_/¯
> 
> Bingo! Adding “@no-unify” for this source file makes the compiler complain
> locally as well, so at least I can try and figure out the reason easily
> now :)

The missing include turned out to be <wtf/CompletionHandler.h>, I am
posting a new version of the patch momentarily :)

[Adrian Perez]

Created attachment 361643 [details]
Patch v16


Adds missing <wtf/CompletionHandler.h> inclusion.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #49)
> You should probably fix these "flaky crashes" before landing it.  Check that
> your memory mapped file is not being unmapped somehow.

Adrian, do you at least have a backtrace to show why the tests are crashing? I'm OK with landing WKTR support before the crashes are fixed, but we should at least have a bug report.

[Michael Catanzaro]

Comment on attachment 361643 [details]
Patch v16

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

Thu number of crashing tests is really huge, so while I'm OK with landing WKTR support for content extensions now, we should look very closely at these failures before deciding to expose the public API for 2.24.

> Tools/WebKitTestRunner/TestController.cpp:1376
> +#if !PLATFORM(COCOA)

Should also have a bug report to make the cross-platform implementation work for Cocoa.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #66)
> (In reply to Alex Christensen from comment #49)
> > You should probably fix these "flaky crashes" before landing it.  Check that
> > your memory mapped file is not being unmapped somehow.
> 
> Adrian, do you at least have a backtrace to show why the tests are crashing?
> I'm OK with landing WKTR support before the crashes are fixed, but we should
> at least have a bug report.

I'm making a debug build to go over the crashes tomorrow, let's try and
figure them out before landing :)

[Adrian Perez]

(In reply to Adrian Perez from comment #68)
> (In reply to Michael Catanzaro from comment #66)
> > (In reply to Alex Christensen from comment #49)
> > > You should probably fix these "flaky crashes" before landing it.  Check that
> > > your memory mapped file is not being unmapped somehow.
> > 
> > Adrian, do you at least have a backtrace to show why the tests are crashing?
> > I'm OK with landing WKTR support before the crashes are fixed, but we should
> > at least have a bug report.
> 
> I'm making a debug build to go over the crashes tomorrow, let's try and
> figure them out before landing :)

As a side note: They really ought to be something related to WKTR, because
I have been using builds of Epiphany and WebKit with patches applied locally
for over a week and there are no crashes there. So at least we have that
going for us, which is nice =)

[Adrian Perez]

(In reply to Michael Catanzaro from comment #67)
> Comment on attachment 361643 [details]
> Patch v16
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361643&action=review
> 
> Thu number of crashing tests is really huge, so while I'm OK with landing
> WKTR support for content extensions now, we should look very closely at
> these failures before deciding to expose the public API for 2.24.

I have figured out those: it turned to be a refcounting issue inside
WKTR which made caused an use-after-free of WKUserContentControllerRef
inside TestController::resetContentExtensions(). Momentarily I will
upload the patch for landing, which fixes this issue.

With this fixed, now more than half of the layout tests automatically
pass (some still need platform specific expectations, which I will do
later as gardening patches), and there are no more crashes.

> > Tools/WebKitTestRunner/TestController.cpp:1376
> > +#if !PLATFORM(COCOA)
> 
> Should also have a bug report to make the cross-platform implementation work
> for Cocoa.

Filed as bug #194515 :)

[Adrian Perez]

Created attachment 361712 [details]
Patch v17


This fixes crashes in WKTR. Should be good for landing.

[WebKit Commit Bot]

Comment on attachment 361712 [details]
Patch v17

Clearing flags on attachment: 361712

Committed r241283: <https://trac.webkit.org/changeset/241283>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47982850>

[Michael Catanzaro]

Committed r241316: <https://trac.webkit.org/changeset/241316>