Unify SharedMemory factory functions

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Adrian Perez]

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().

[Adrian Perez]

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).

[Adrian Perez]

Created attachment 357339 [details]
Patch

[Adrian Perez]

Created attachment 357342 [details]
Patch v2

[Don Olmstead]

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?

[Darin Adler]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(In reply to Don Olmstead from comment #4)
> Should we ever be using mmap directly?

How else to do shared memory?

[Darin Adler]

(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.

[Michael Catanzaro]

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.

[Adrian Perez]

(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 :)

[Adrian Perez]

(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.)

[Adrian Perez]

(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.

[Adrian Perez]

Created attachment 357418 [details]
Patch v3

[Darin Adler]

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).

[WebKit Commit Bot]

Comment on attachment 357418 [details]
Patch v3

Clearing flags on attachment: 357418

Committed r239260: <https://trac.webkit.org/changeset/239260>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46762407>

[Adrian Perez]

(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.

[Adrian Perez]

Created attachment 357419 [details]
Patch for landing

[Adrian Perez]

(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 :)

[WebKit Commit Bot]

Comment on attachment 357419 [details]
Patch for landing

Clearing flags on attachment: 357419

Committed r239261: <https://trac.webkit.org/changeset/239261>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.