Bug 197078 - WKContentRuleLists should have a maximum FileProtection of CompleteUnlessOpen
Summary: WKContentRuleLists should have a maximum FileProtection of CompleteUnlessOpen
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-04-18 15:45 PDT by Alex Christensen
Modified: 2019-04-26 14:37 PDT (History)
6 users (show)

See Also:


Attachments
Patch (40.35 KB, patch)
2019-04-18 15:51 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (40.38 KB, patch)
2019-04-19 13:58 PDT, Alex Christensen
ggaren: review+
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-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