Bug 227359 – [Model] Create a sandbox extension for a temporary directory to store model resources

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

[Antoine Quint]

[Model] Create a sandbox extension for a temporary directory to store model resources

[Antoine Quint]

Created attachment 432178 [details]
Patch

[Sam Weinig]

Comment on attachment 432178 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:77
> +@property (nonatomic, copy, setter=_setModelElementCacheDirectory:) NSURL *_modelElementCacheDirectory WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Given we don't intend to ever ship this kind of thing, I would not add any SPI for this.

[Antoine Quint]

Created attachment 432179 [details]
Patch

[Antoine Quint]

Created attachment 432182 [details]
Patch

[Antoine Quint]

(In reply to Sam Weinig from comment #2)
> Comment on attachment 432178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432178&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:77
> > +@property (nonatomic, copy, setter=_setModelElementCacheDirectory:) NSURL *_modelElementCacheDirectory WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Given we don't intend to ever ship this kind of thing, I would not add any
> SPI for this.

I removed this method and its implementation.

[Simon Fraser (smfr)]

Comment on attachment 432182 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.cpp:576
> +    if (!parameters.modelElementCacheDirectory.isEmpty())

This looks like it's querying the filesystem to ask if the directory is empty, but I think it's asking if the string is empty.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 432182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432182&action=review
> 
> > Source/WebKit/WebProcess/WebProcess.cpp:576
> > +    if (!parameters.modelElementCacheDirectory.isEmpty())
> 
> This looks like it's querying the filesystem to ask if the directory is
> empty, but I think it's asking if the string is empty.

I believe that is correct, we only want to set the name of the directory if it's not an empty string. This follows the approach taken for the media cache directory just before this call.

[Antoine Quint]

Committed r279267 (239146@main): <https://commits.webkit.org/239146@main>

[Radar WebKit Bug Importer]

<rdar://problem/79763702>

[Geoffrey Garen]

Comment on attachment 432182 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:327
> +    return tempDirectoryFileSystemRepresentation("ModelElement", ShouldCreateDirectory::No);

Let's call this "ModelElementCache", since that's what we call it in code.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:270
> +    if (!m_configuration->modelElementCacheDirectory().isEmpty())
> +        m_resolvedConfiguration->setModelElementCacheDirectory(resolveAndCreateReadWriteDirectoryForSandboxExtension(m_configuration->modelElementCacheDirectory()));

I think we need to nuke this directory unconditionally on launch; else we have a privacy bug if we ever crash or leak.

[Geoffrey Garen]

Comment on attachment 432182 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.cpp:52
> +#if ENABLE(MODEL_ELEMENT)
> +        setModelElementCacheDirectory(WebsiteDataStore::defaultModelElementCacheDirectory());
> +#endif

Since this is the persistent case only, does this mean that model elements will be broken in Private Browsing?

[Sam Weinig]

(In reply to Geoffrey Garen from comment #10)
> Comment on attachment 432182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432182&action=review
> 
> > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:327
> > +    return tempDirectoryFileSystemRepresentation("ModelElement", ShouldCreateDirectory::No);
> 
> Let's call this "ModelElementCache", since that's what we call it in code.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:270
> > +    if (!m_configuration->modelElementCacheDirectory().isEmpty())
> > +        m_resolvedConfiguration->setModelElementCacheDirectory(resolveAndCreateReadWriteDirectoryForSandboxExtension(m_configuration->modelElementCacheDirectory()));
> 
> I think we need to nuke this directory unconditionally on launch; else we
> have a privacy bug if we ever crash or leak.

That seems like a good idea, but NOTE, we know we cannot ship this. This is just in place until we have the SPI to do in memory loading.

[Simon Fraser (smfr)]

(In reply to Sam Weinig from comment #12)
> (In reply to Geoffrey Garen from comment #10)
> > Comment on attachment 432182 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=432182&action=review
> > 
> > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:327
> > > +    return tempDirectoryFileSystemRepresentation("ModelElement", ShouldCreateDirectory::No);
> > 
> > Let's call this "ModelElementCache", since that's what we call it in code.
> > 
> > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:270
> > > +    if (!m_configuration->modelElementCacheDirectory().isEmpty())
> > > +        m_resolvedConfiguration->setModelElementCacheDirectory(resolveAndCreateReadWriteDirectoryForSandboxExtension(m_configuration->modelElementCacheDirectory()));
> > 
> > I think we need to nuke this directory unconditionally on launch; else we
> > have a privacy bug if we ever crash or leak.
> 
> That seems like a good idea, but NOTE, we know we cannot ship this. This is
> just in place until we have the SPI to do in memory loading.

Perhaps we should wrap the temporary code in a HAVE_ macro then?

[Brent Fulgham]

Comment on attachment 432182 [details]
Patch

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

>>>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:327
>>>> +    return tempDirectoryFileSystemRepresentation("ModelElement", ShouldCreateDirectory::No);
>>> 
>>> Let's call this "ModelElementCache", since that's what we call it in code.
>> 
>> That seems like a good idea, but NOTE, we know we cannot ship this. This is just in place until we have the SPI to do in memory loading.
> 
> Perhaps we should wrap the temporary code in a HAVE_ macro then?

I'd suggest having a FIXME comment with a bug/radar reference to remove this temporary code once the SPI exists.