Bug 143123 – [Content Extensions] Flesh out the UserContentExtensionStore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[Content Extensions] Flesh out the UserContentExtensionStore

[Sam Weinig]

Created attachment 249572 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:75:  make_error_code is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:83:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:50:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:65:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:88:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:114:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:135:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:128:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:128:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:344:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 13 in 13 files


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

[Alex Christensen]

Why is this called UserContentExtensionStore instead of ContentExtensionStore?

[Chris Dumez]

Comment on attachment 249572 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:51
> +    static UserContentExtensionStore* defaultStore = adoptRef(new UserContentExtensionStore).leakRef();

nit: Why not use NeverDestroyed here?

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:61
> +UserContentExtensionStore::UserContentExtensionStore(const WTF::String& storePath)

Is WTF:: really needed here?

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:101
> +        // The file data should be mapped into one continuous memory segrment so the size

"segment"

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:116
> +        return false;

Why are we returning false here?

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:123
> +    int fd = ::open(path.data(), O_RDONLY, 0);

We might want to use our FileSystem abstraction because this method is currently POSIX only and this file does not have POSIX in the name.

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:219
> +        close(temporaryFileHandle);

WebCore::closeFile()

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:224
> +    close(temporaryFileHandle);

WebCore::closeFile()

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:242
> +    m_readQueue->dispatch([self, identifierCapture, pathCapture, completionHandler] {

Capturing self here does not look safe as RefPtr ref counting is not thread-safe.

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:273
> +void UserContentExtensionStore::compileContentExtension(const WTF::String& identifier, const WTF::String& json, std::function<void(RefPtr<API::UserContentExtension>, std::error_code)> completionHandler)

Do we really need WTF:: ?

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:280
> +    m_compileQueue->dispatch([self, identifierCapture, jsonCapture, pathCapture, completionHandler] {

Capturing self here does not look safe as RefPtr ref counting is not thread-safe.

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:318
> +    m_removeQueue->dispatch([self, identifierCapture, pathCapture, completionHandler] {

Capturing self here does not look safe as RefPtr ref counting is not thread-safe.

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:321
> +        if (unlink(path.data()) == -1) {

WebCore::deleteFile()

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:54
> +    explicit UserContentExtensionStore(const WTF::String& storePath);

Are all these WTF:: really needed?

[Chris Dumez]

+antti as this is using the NetworkCache code.

[Chris Dumez]

Comment on attachment 249572 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:100
> +    fileData.apply([&metaData, &success, &fileData](const uint8_t* data, size_t size) {

Do we really need this success variable? Why not return the boolean returned by Data::apply() ?

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:148
> +    fileData.apply([fd, &success](const uint8_t* data, size_t size) {

Do we really need this success variable? Why not return the boolean returned by Data::apply() ?

[Benjamin Poulain]

Comment on attachment 249572 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:35
> +#include "NetworkCacheData.h"
> +#include "NetworkCacheDecoder.h"
> +#include "NetworkCacheEncoder.h"
> +#include "NetworkCacheFileSystemPosix.h"

Uh?

Shouldn't this move out of NetworkCache?

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:75
> +    return WebCore::fileSystemRepresentation(WebCore::pathByAppendingComponent(base, "ContentExtension-" + identifier));

Using the identifier in the filename seems a big risky to me. HFS is notorious for its crazy handling of unicode.

We could have a map identifier->filename in a separate file.
Alternatively, we could sanitize the identifier names to be a strict subset of ASCII.

>> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:101
>> +        // The file data should be mapped into one continuous memory segrment so the size
> 
> "segment"

Typo: segrment

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:128
> +    if (::fstat(fd, &stat) != 0) {        

WebKit style for checking null values here.

[Antti Koivisto]

> Capturing self here does not look safe as RefPtr ref counting is not
> thread-safe.

There is nothing non-thread-safe about RefPtr itself. Using it like this is ok as long as as the type uses atomic refcounting (by inheriting ThreadSafeRefCounted for example) and is ok to be destructed in a different thread.

[Chris Dumez]

(In reply to comment #8)
> > Capturing self here does not look safe as RefPtr ref counting is not
> > thread-safe.
> 
> There is nothing non-thread-safe about RefPtr itself. Using it like this is
> ok as long as as the type uses atomic refcounting (by inheriting
> ThreadSafeRefCounted for example) and is ok to be destructed in a different
> thread.

Oh, I see that that it is subclassing ObjectImpl<> which subclasses ThreadSafeRefCounted. Please disregard my comment then.

[Sam Weinig]

Created attachment 249666 [details]
Patch

[Sam Weinig]

(In reply to comment #3)
> Why is this called UserContentExtensionStore instead of
> ContentExtensionStore?

That is what we are calling it currently at the API/WebKit level.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/Shared/WebCompiledContentExtensionData.h:47:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:77:  make_error_code is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:50:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:65:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:88:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:114:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:135:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:331:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 12 in 21 files


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

[Sam Weinig]

Created attachment 249667 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/Shared/WebCompiledContentExtensionData.h:47:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:77:  make_error_code is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:50:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:65:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:88:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:114:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:135:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:331:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 12 in 21 files


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

[Sam Weinig]

(In reply to comment #4)
> Comment on attachment 249572 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249572&action=review
> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:51
> > +    static UserContentExtensionStore* defaultStore = adoptRef(new UserContentExtensionStore).leakRef();
> 
> nit: Why not use NeverDestroyed here?

Fixed.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:61
> > +UserContentExtensionStore::UserContentExtensionStore(const WTF::String& storePath)
> 
> Is WTF:: really needed here?

Yes. Otherwise, it assumes you mean API::String.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:101
> > +        // The file data should be mapped into one continuous memory segrment so the size
> 
> "segment"

Fixed.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:116
> > +        return false;
> 
> Why are we returning false here?

This false means don't continue traversing data segments. In this case, we should only ever traverse a single data segment, but we also don't want to traverse more than one if something goes wrong.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:123
> > +    int fd = ::open(path.data(), O_RDONLY, 0);
> 
> We might want to use our FileSystem abstraction because this method is
> currently POSIX only and this file does not have POSIX in the name.

Fixed.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:219
> > +        close(temporaryFileHandle);
> 
> WebCore::closeFile()

Fixed.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:224
> > +    close(temporaryFileHandle);
> 
> WebCore::closeFile()

Fixed.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:242
> > +    m_readQueue->dispatch([self, identifierCapture, pathCapture, completionHandler] {
> 
> Capturing self here does not look safe as RefPtr ref counting is not
> thread-safe.

As Antti noted, it's cool.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:321
> > +        if (unlink(path.data()) == -1) {
> 
> WebCore::deleteFile()

Fixed.

[Sam Weinig]

(In reply to comment #7)
> Comment on attachment 249572 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249572&action=review
> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:35
> > +#include "NetworkCacheData.h"
> > +#include "NetworkCacheDecoder.h"
> > +#include "NetworkCacheEncoder.h"
> > +#include "NetworkCacheFileSystemPosix.h"
> 
> Uh?
> 
> Shouldn't this move out of NetworkCache?

We should, and I will move that code soon.

> 
> > Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:75
> > +    return WebCore::fileSystemRepresentation(WebCore::pathByAppendingComponent(base, "ContentExtension-" + identifier));
> 
> Using the identifier in the filename seems a big risky to me. HFS is
> notorious for its crazy handling of unicode.
> 
> We could have a map identifier->filename in a separate file.
> Alternatively, we could sanitize the identifier names to be a strict subset
> of ASCII.

I fixed this to use the WebCore::WebCore::encodeForFileName() function from FileSystem.h that we use elsewhere.

[Sam Weinig]

Created attachment 249668 [details]
Patch

[Sam Weinig]

Created attachment 249669 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/Shared/WebCompiledContentExtensionData.h:47:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:75:  make_error_code is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.h:83:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:50:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:65:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:88:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:114:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:135:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:332:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 12 in 21 files


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

[Benjamin Poulain]

I have been thinking a bit about this. I think we should also keep a copy of the original input to have a simpler update path.

If we change anything in the content extension engine, we can just nuke the existing compiled files and compile new ones from scratch.

What do you think?

[Sam Weinig]

(In reply to comment #20)
> I have been thinking a bit about this. I think we should also keep a copy of
> the original input to have a simpler update path.
> 
> If we change anything in the content extension engine, we can just nuke the
> existing compiled files and compile new ones from scratch.
> 
> What do you think?

Seems reasonable. That said, I would like to do that in a separate patch.

[Benjamin Poulain]

Comment on attachment 249669 [details]
Patch

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

The patch looks good to me.

I have no idea how the public API works but: shouldn't we make sure we tell all web processes to unload the extension before removing the file? It would be weird to have a WebProcess still executing extensions after they were removed.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:77
> +    if (!oldPathFsRep.data() || oldPathFsRep.data()[0] == '\0')

CString::length() can tell you that. The terminating \0 is not included in the size returned by length().

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:81
> +    if (!newPathFsRep.data() || newPathFsRep.data()[0] == '\0')

Ditto.

> Source/WebKit2/Shared/WebCompiledContentExtension.cpp:49
>  Ref<WebCompiledContentExtension> WebCompiledContentExtension::createFromCompiledContentExtensionData(const WebCore::ContentExtensions::CompiledContentExtensionData& compilerData)

Is this one still useful?

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:234
> +static RefPtr<API::UserContentExtension> createExtension(const String& identifier, const ContentExtensionMetaData& metaData, const Data& fileData)

createUserContentExtension()?
createUserContentExtensionFromFile()?
createUserContentExtensionFromMappedFile()?

[WebKit Commit Bot]

Comment on attachment 249669 [details]
Patch

Clearing flags on attachment: 249669

Committed r182161: <http://trac.webkit.org/changeset/182161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This patch broke Media controls on Windows. Ugh!

[Brent Fulgham]

It looks like RenderThemeWin (and other sites) call into this new no-op function to locate theme information prior to rendering.

[Brent Fulgham]

Fixed in r182757: <http://trac.webkit.org/changeset/182757>