[Model] Create a sandbox extension for a temporary directory to store model resources
Created attachment 432178 [details] Patch
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.
Created attachment 432179 [details] Patch
Created attachment 432182 [details] Patch
(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.
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.
(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.
Committed r279267 (239146@main): <https://commits.webkit.org/239146@main>
<rdar://problem/79763702>
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.
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?
(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.
(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?
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.