Bug 195511 - Add a WKContentRuleList variant that uses copied memory instead of mmap'd shared memory for class A containerized apps
Summary: Add a WKContentRuleList variant that uses copied memory instead of mmap'd sha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 21:05 PST by Alex Christensen
Modified: 2019-03-11 13:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (46.95 KB, patch)
2019-03-08 21:08 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (46.92 KB, patch)
2019-03-08 21:18 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (46.90 KB, patch)
2019-03-10 22:12 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
crash log (70.44 KB, text/plain)
2019-03-11 11:50 PDT, Truitt Savell
no flags Details
Patch (46.94 KB, patch)
2019-03-11 12:49 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-03-08 21:05:04 PST
Add a WKContentRuleList variant that uses copied memory instead of mmap'd shared memory for class A containerized apps
Comment 1 Alex Christensen 2019-03-08 21:08:36 PST
Created attachment 364112 [details]
Patch
Comment 2 Alex Christensen 2019-03-08 21:08:38 PST
<rdar://problem/44873269>
Comment 3 Alex Christensen 2019-03-08 21:18:49 PST
Created attachment 364114 [details]
Patch
Comment 4 Darin Adler 2019-03-09 09:45:12 PST
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.
Comment 5 Alex Christensen 2019-03-10 22:10:41 PDT
(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.
Comment 6 Alex Christensen 2019-03-10 22:12:21 PDT
Created attachment 364233 [details]
Patch
Comment 7 WebKit Commit Bot 2019-03-10 23:03:22 PDT
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 8 WebKit Commit Bot 2019-03-10 23:04:06 PDT
Comment on attachment 364233 [details]
Patch

Clearing flags on attachment: 364233

Committed r242698: <https://trac.webkit.org/changeset/242698>
Comment 9 WebKit Commit Bot 2019-03-10 23:04:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Truitt Savell 2019-03-11 08:10:54 PDT
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 11 Darin Adler 2019-03-11 09:46:45 PDT
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.
Comment 12 Alex Christensen 2019-03-11 09:51:06 PDT
Reverted in http://trac.webkit.org/r242710 to investigate why the test crashed on the bots.
Comment 13 Truitt Savell 2019-03-11 11:50:27 PDT
Created attachment 364274 [details]
crash log

Crash log off of bot.
Comment 14 Alex Christensen 2019-03-11 12:48:29 PDT
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!
Comment 15 Alex Christensen 2019-03-11 12:49:23 PDT
Reopening to attach new patch.
Comment 16 Alex Christensen 2019-03-11 12:49:24 PDT
Created attachment 364279 [details]
Patch
Comment 17 WebKit Commit Bot 2019-03-11 13:21:46 PDT
Comment on attachment 364279 [details]
Patch

Clearing flags on attachment: 364279

Committed r242735: <https://trac.webkit.org/changeset/242735>
Comment 18 WebKit Commit Bot 2019-03-11 13:21:48 PDT
All reviewed patches have been landed.  Closing bug.