Currently, the following factory functions are available: - SharedMemory::create(uint8_t*, size_t, Protection), used directly in NetworkCache::Data::tryCreateSharedMemory(), and in the content extensions code to obtain a SharedMemory object to pass to other processes from a mapped file. The latter is only used in the Cocoa port. - SharedMemory::wrapMap(uint8_t*, size_t, int), used only in the libsoup implementation of NetworkCache::Data::tryCreateSharedMemory(). This takes a mmap address+size and its file descriptor, and the intention is the same as Cocoa's ::create, but here being called ::wrapMap (which is incoherent). - SharedMemory::adopt(HANDLE, size_t, Protection) for the Windows implementation is used internally in SharedMemoryWin.cpp but nowhere else, and the intention seems to be different from the two previous ones: in this case it takes a file handle, and memory-maps it (instead of taking an *existing* map). This should be private. I would like to propose picking the same name for the first two cases, and IMHO the name ::wrapMap() better indicates the intention. Also, I want to make the functions private: the only use is inside NetworkCache::Data::tryCreateSharedMemory(), so that could be marked as “friend”; and the usage of ::create() in the content extensions converted to a call to ::tryCreateSharedMemory().
One more note: in case it's not clear, the intention is to NOT have a SharedMemory::create() because it's ambiguous: - Does it allocate and create a SharedMemory object? No, that is what ::allocate() does! - Does it create only a SharedMemory object wrapping something else? It depends! (Yes on Cocoa, no on Unix). Not having ::allocate() means that what will be left is either ::allocate() (and the intention is clear) or ::wrapMap() (for which the intention is always clear).
Created attachment 357339 [details] Patch
Created attachment 357342 [details] Patch v2
Comment on attachment 357342 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=357342&action=review Windows side looks fine to me just wondering about using mmap directly instead of the stuff in WTF. I know on Windows we hit stuff like WebAssembly using mmap directly instead of abstractions over that so we never bothered to turn it on. > Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:166 > + void* data = mmap(nullptr, size, accessModeMMap(SharedMemory::Protection::ReadWrite), MAP_SHARED, fileDescriptor, 0); Should we ever be using mmap directly?
Comment on attachment 357342 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=357342&action=review > Source/WebKit/Platform/SharedMemory.h:57 > +}; Please don’t include that semicolon. See <https://trac.webkit.org/changeset/239017> for example. > Source/WebKit/Platform/SharedMemory.h:146 > + // NetworkCache::Data::tryCreateSharedMemory() is the only function > + // the ::wrapMap(), so mark it as friend and keep the factory private, > + // to make sure it is not used accidentally by any other code. Grammar error here: "is the only function the ::wrapMap()". >> Source/WebKit/Platform/unix/SharedMemoryUnix.cpp:166 >> + void* data = mmap(nullptr, size, accessModeMMap(SharedMemory::Protection::ReadWrite), MAP_SHARED, fileDescriptor, 0); > > Should we ever be using mmap directly? If we do stick with mmap, then I think it’s peculiar to write accessModeMMap(SharedMemory::Protection::ReadWrite) instead of just writing PROT_READ | PROT_WRITE. Also, I think that likely we don’t need the accessModeMMap function any more.
(In reply to Adrian Perez from comment #0) > Also, I want to make the functions private: the only use is inside > NetworkCache::Data::tryCreateSharedMemory(), so that could be marked > as “friend”; and the usage of ::create() in the content extensions > converted to a call to ::tryCreateSharedMemory(). Why does this make sense? It seems OK to have public functions that are only used in a singular location, especially since in the future they could be useful elsewhere. At least, that seems nicer than friend, especially since the classes are not closely-related here.
(In reply to Don Olmstead from comment #4) > Should we ever be using mmap directly? How else to do shared memory?
(In reply to Michael Catanzaro from comment #6) > It seems OK to have public functions that are only > used in a singular location, especially since in the future they could be > useful elsewhere. At least, that seems nicer than friend, especially since > the classes are not closely-related here. Good point. I agree.
Comment on attachment 357342 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=357342&action=review > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:373 > + auto sharedMemory = fileData.tryCreateSharedMemory(); > + ASSERT(sharedMemory); Why is this ASSERT OK? You have to be certain that fileData.tryCreateSharedMemory() will never fail, but the name of the function starts with "try" so this is suspicious.
(In reply to Darin Adler from comment #8) > (In reply to Michael Catanzaro from comment #6) > > It seems OK to have public functions that are only > > used in a singular location, especially since in the future they could be > > useful elsewhere. At least, that seems nicer than friend, especially since > > the classes are not closely-related here. > > Good point. I agree. My initial idea with this was to avoid shooting ourselves on the foot and thinking twice before using the factory functions -- the direct usage of SharedMemory::create() while working on #167941 took me quite some time to figure out, because on Linux it would create a new memory area instead of wrapping the existing file memory map. With this patch addressing the naming of the functions and making it consistent across platforms probably the point for making the factory functions private is weaker, so let's just leave them public :)
(In reply to Michael Catanzaro from comment #7) > (In reply to Don Olmstead from comment #4) > > Should we ever be using mmap directly? > > How else to do shared memory? It looks to me that one of the reasons for having the SharedMemory wrapper objects is that then one can write IPC encoders and a decoders which operate on the SharedMemory type, instead of having to manually handle the set of <address, size, flags> indidivually each time one wants to send a shared memory buffer around. This is currently used in a few places (NetworkCache, for example). (Of course, for data which will not be send cross processes, one can use mmap directly, which I think can be part of the reason why JSC does so.)
(In reply to Michael Catanzaro from comment #9) > Comment on attachment 357342 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357342&action=review > > > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:373 > > + auto sharedMemory = fileData.tryCreateSharedMemory(); > > + ASSERT(sharedMemory); > > Why is this ASSERT OK? You have to be certain that > fileData.tryCreateSharedMemory() will never fail, but the name of the > function starts with "try" so this is suspicious. Content extensions are always saved to disk in compiled form before they are mmap'd and then sent to the WebProcess and NetworkProcess. Which means there is always a file which was previously mmap'd backing the NetworkCache::Data object, and ::tryCreateSharedMemory() cannot fail in that case, because the check it does is: RefPtr<SharedMemory> Data::tryCreateSharedMemory() const { if (isNull() || !isMap()) return nullptr; return SharedMemory::wrapMap(const_cast<char*>(m_buffer->data), m_buffer->length, m_fileDescriptor); } We should always have file-mapping, non-null NetworkCache::Data object here, so the first check cannot fail, and the SharedData::wrapMap() cannot fail either. I will add a comment about this.
Created attachment 357418 [details] Patch v3
Comment on attachment 357418 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=357418&action=review > Source/WebKit/Platform/win/SharedMemoryWin.cpp:135 > + HANDLE handle = ::CreateFileMappingW(INVALID_HANDLE_VALUE, 0, protectAttribute(SharedMemory::Protection::ReadWrite), 0, size, 0); Similar comment to the one I made earlier for the Unix code. This should say PAGE_READWRITE, not protectAttribute(SharedMemory::Protection::ReadWrite).
Comment on attachment 357418 [details] Patch v3 Clearing flags on attachment: 357418 Committed r239260: <https://trac.webkit.org/changeset/239260>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46762407>
(In reply to Darin Adler from comment #14) > Comment on attachment 357418 [details] > Patch v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357418&action=review > > > Source/WebKit/Platform/win/SharedMemoryWin.cpp:135 > > + HANDLE handle = ::CreateFileMappingW(INVALID_HANDLE_VALUE, 0, protectAttribute(SharedMemory::Protection::ReadWrite), 0, size, 0); > > Similar comment to the one I made earlier for the Unix code. This should say > PAGE_READWRITE, not protectAttribute(SharedMemory::Protection::ReadWrite). I post a follow-up patch with this change.
Created attachment 357419 [details] Patch for landing
(In reply to Adrian Perez from comment #19) > Created attachment 357419 [details] > Patch for landing As I don't have a Windows box around, I'll let the EWS go over the patch before landing :)
Comment on attachment 357419 [details] Patch for landing Clearing flags on attachment: 357419 Committed r239261: <https://trac.webkit.org/changeset/239261>