Bug 195511

Created attachment 364112 [details] Patch

<rdar://problem/44873269>

Created attachment 364114 [details] Patch

Comment on attachment 364114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364114&action=review > Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:46 > + return static_cast<const void*>(sharedMemoryOrBuffer->data()); Surprised that a typecast is needed here. Is there a way to write it without the static_cast? > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:167 > + function(reinterpret_cast<const uint8_t*>(data.data()), data.size()); Use of SharedBuffer::data is normally discouraged; instead we try to use iteration. I suppose, though, that in this case the reasons for that don’t really apply. Make me wonder about the future of the SharedBuffer interface.

(In reply to Darin Adler from comment #4) > Comment on attachment 364114 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=364114&action=review > > > Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:46 > > + return static_cast<const void*>(sharedMemoryOrBuffer->data()); > > Surprised that a typecast is needed here. Is there a way to write it without > the static_cast? It couldn't infer whether the type was I'll use a trailing return type on the lambda instead. Otherwise it tries to guess whether it needs to return a void* (from SharedMemory::data) or a const char* (from SharedBuffer::data). > > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:167 > > + function(reinterpret_cast<const uint8_t*>(data.data()), data.size()); > > Use of SharedBuffer::data is normally discouraged; instead we try to use > iteration. I suppose, though, that in this case the reasons for that don’t > really apply. Make me wonder about the future of the SharedBuffer interface. We should just make SharedBuffer::DataSegment a first-class serializable object and use that instead of a SharedBuffer which we know has only one segment. Because of the nature of this radar, I'm going to not do that in this patch.

Created attachment 364233 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 364233 [details]: legacy-animation-engine/imported/blink/animations/animation-events-unprefixed-02.html bug 195545 (authors: dino@apple.com and graouts@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 364233 [details] Patch Clearing flags on attachment: 364233 Committed r242698: <https://trac.webkit.org/changeset/242698>

All reviewed patches have been landed. Closing bug.

The changes in https://trac.webkit.org/changeset/242698/webkit Appear to have caused an API test crash on Mac and iOS: Mac Crashed TestWebKitAPI.WKContentRuleListStoreTest.UnsafeMMap _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. iOS Crashed TestWebKitAPI.WKContentRuleListStoreTest.UnsafeMMap Child process terminated with signal 11: Segmentation fault Build: https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/3519

Comment on attachment 364114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364114&action=review >>> Source/WebKit/Shared/WebCompiledContentRuleListData.cpp:46 >>> + return static_cast<const void*>(sharedMemoryOrBuffer->data()); >> >> Surprised that a typecast is needed here. Is there a way to write it without the static_cast? > > It couldn't infer whether the type was I'll use a trailing return type on the lambda instead. Otherwise it tries to guess whether it needs to return a void* (from SharedMemory::data) or a const char* (from SharedBuffer::data). Yes, the return type on the lambda was what I (vaguely) had in mind. >>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:167 >>> + function(reinterpret_cast<const uint8_t*>(data.data()), data.size()); >> >> Use of SharedBuffer::data is normally discouraged; instead we try to use iteration. I suppose, though, that in this case the reasons for that don’t really apply. Make me wonder about the future of the SharedBuffer interface. > > We should just make SharedBuffer::DataSegment a first-class serializable object and use that instead of a SharedBuffer which we know has only one segment. Because of the nature of this radar, I'm going to not do that in this patch. Does sound like the best solution in the longer term.

Reverted in http://trac.webkit.org/r242710 to investigate why the test crashed on the bots.

Created attachment 364274 [details] crash log Crash log off of bot.

Comment on attachment 364233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364233&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKContentExtensionStore.mm:408 > + NSString *ruleListSourceString = @"[{\"action\":{\"type\":\"block\"},\"trigger\":{\"url-filter\":\"blockedsubresource\"}}]"; I could reproduce this on my High Sierra machine. My NSStrings were not being retained. That was causing a UAF crash after using them after the autoreleasepool had been drained. I'm fixing this by using "static NSString *" and RetainPtr. The problem was just with the test, not with the change. Whew!

Reopening to attach new patch.

Created attachment 364279 [details] Patch

Comment on attachment 364279 [details] Patch Clearing flags on attachment: 364279 Committed r242735: <https://trac.webkit.org/changeset/242735>

All reviewed patches have been landed. Closing bug.