Bug 195511 – Add a WKContentRuleList variant that uses copied memory instead of mmap'd shared memory for class A containerized apps

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

[Alex Christensen]

Add a WKContentRuleList variant that uses copied memory instead of mmap'd shared memory for class A containerized apps

[Alex Christensen]

Created attachment 364112 [details]
Patch

[Alex Christensen]

<rdar://problem/44873269>

[Alex Christensen]

Created attachment 364114 [details]
Patch

[Darin Adler]

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.

[Alex Christensen]

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

[Alex Christensen]

Created attachment 364233 [details]
Patch

[WebKit Commit Bot]

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: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 364233 [details]
Patch

Clearing flags on attachment: 364233

Committed r242698: <https://trac.webkit.org/changeset/242698>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[Darin Adler]

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.

[Alex Christensen]

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

[Truitt Savell]

Created attachment 364274 [details]
crash log

Crash log off of bot.

[Alex Christensen]

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!

[Alex Christensen]

Reopening to attach new patch.

[Alex Christensen]

Created attachment 364279 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364279 [details]
Patch

Clearing flags on attachment: 364279

Committed r242735: <https://trac.webkit.org/changeset/242735>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.