[Content Extensions] Flesh out the UserContentExtensionStore
Created attachment 249572 [details] Patch
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.
Why is this called UserContentExtensionStore instead of ContentExtensionStore?
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?
+antti as this is using the NetworkCache code.
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() ?
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.
> 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.
(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.
Created attachment 249666 [details] Patch
(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.
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.
Created attachment 249667 [details] Patch
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.
(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.
(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.
Created attachment 249668 [details] Patch
Created attachment 249669 [details] Patch
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.
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?
(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.
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()?
Comment on attachment 249669 [details] Patch Clearing flags on attachment: 249669 Committed r182161: <http://trac.webkit.org/changeset/182161>
All reviewed patches have been landed. Closing bug.
This patch broke Media controls on Windows. Ugh!
It looks like RenderThemeWin (and other sites) call into this new no-op function to locate theme information prior to rendering.
Fixed in r182757: <http://trac.webkit.org/changeset/182757>