Bug 197078

Summary: WKContentRuleLists should have a maximum FileProtection of CompleteUnlessOpen
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, ews, ggaren, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch ggaren: review+

Description Alex Christensen 2019-04-18 15:45:46 PDT
WKContentRuleLists should have a maximum FileProtection of CompleteUnlessOpen
Comment 1 Alex Christensen 2019-04-18 15:51:23 PDT
Created attachment 367767 [details]
Patch
Comment 2 Alex Christensen 2019-04-18 15:51:27 PDT
<rdar://problem/49564348>
Comment 3 Alex Christensen 2019-04-19 13:58:51 PDT
Created attachment 367822 [details]
Patch
Comment 4 Geoffrey Garen 2019-04-22 10:01:36 PDT
Comment on attachment 367822 [details]
Patch

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

r=me

> Source/WebKit/NetworkProcess/cache/NetworkCacheFileSystemCocoa.mm:39
> +    // FIXME: For the network cache we should either use makeSafeToUseMemoryMapForPath instead of this
> +    // or we should listen to UIApplicationProtectedDataWillBecomeUnavailable and unmap files.

Can you address this FIXME in a follow-up patch? Should be pretty straight-forward. (We probably can't close the Radar until we fix both cases.)
Comment 5 Alex Christensen 2019-04-24 10:46:45 PDT
http://trac.webkit.org/r244597
Comment 6 Antti Koivisto 2019-04-25 00:35:00 PDT
Comment on attachment 367822 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheFileSystemCocoa.mm:41
> +    NSDictionary<NSFileAttributeKey, id> *attributes = [[NSFileManager defaultManager] attributesOfItemAtPath:path error:&error];

This constructs a dictionary with all kinds of attributes that we don't care about. Should we worry about performance here? How about using the C API like the existing code does?
Comment 7 Alex Christensen 2019-04-26 14:37:20 PDT
Comment on attachment 367822 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/NetworkCacheFileSystemCocoa.mm:41
>> +    NSDictionary<NSFileAttributeKey, id> *attributes = [[NSFileManager defaultManager] attributesOfItemAtPath:path error:&error];
> 
> This constructs a dictionary with all kinds of attributes that we don't care about. Should we worry about performance here? How about using the C API like the existing code does?

I don't think this is a significant performance regression.  We could switch back if we find it to be worth it, but the lack of magic numbers and mysterious structures here makes the code much more maintainable.
Comment 8 Alex Christensen 2019-04-26 14:37:52 PDT
(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 367822 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367822&action=review
> 
> r=me
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCacheFileSystemCocoa.mm:39
> > +    // FIXME: For the network cache we should either use makeSafeToUseMemoryMapForPath instead of this
> > +    // or we should listen to UIApplicationProtectedDataWillBecomeUnavailable and unmap files.
> 
> Can you address this FIXME in a follow-up patch? Should be pretty
> straight-forward. (We probably can't close the Radar until we fix both
> cases.)

Done in https://bugs.webkit.org/show_bug.cgi?id=197264