Closed Bug 1463587 Opened 6 years ago Closed 6 years ago

Refactor initial process data into a shared memory key-value store that lazily decodes

Categories

(Core :: IPC, enhancement)

Product:
Core
Component:
IPC
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:noted])

Attachments

(6 files)

Bug 1463587: Part 1 - Add helper class for creating snapshots of shared memory regions.
59 bytes, text/x-review-board-request
erahm
: review+
jld
: review+
Details
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.
59 bytes, text/x-review-board-request
bzbarsky
: review+
erahm
: review+
Details
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.
59 bytes, text/x-review-board-request
baku
: review+
erahm
: review+
bzbarsky
: review+
Details
Bug 1463587: Part 4 - Add blob support to SharedMap.
59 bytes, text/x-review-board-request
baku
: review+
erahm
: review+
Details
Bug 1463587: Part 5 - Add tests for SharedMap.
59 bytes, text/x-review-board-request
erahm
: review+
Details
Bug 1463587: Part 6 - Add an idle flush task to WritableSharedMap.
59 bytes, text/x-review-board-request
erahm
: review+
Details 
Right now, initial process data is sent as a single structured-cloned blob, which is eagerly decoded at process startup. While that's fine for a small amount of data needed at process startup, it's bad for larger amounts of data that need to be available synchronously when necessary, but may not be needed all at one, or in every process. It's also not great for synchronizing data updates across processes.

The particular use case I'm thinking of is extension locale data. The extension l10n APIs are synchronous, which means that we need to have the data available in case any extension needs it. But we may never load an extension content script in a process, and if we do, that content script may never need l10n data.

If we put that data in a shared-memory key-value store, though, we could decode it when needed, and throw it away when we're unlikely to need it again, without having to worry about sync IPC overhead.
There are some sandboxing implications here — we don't want one content process to be able to overwrite the shared memory and attack other processes.

On Windows it looks like DuplicateHandle can drop write access, so if it actually does that and there isn't some obscure way for the child process to give itself back those access rights (DuplicateHandle does have some footguns, but I think that's not one of them?), then that would work.

Unix is harder.  If the data won't change, then it could be mapped MAP_PRIVATE (and make absolutely sure the fd is closed before the process starts handling hostile data), but given the verbiage about “synchronizing data updates” it may need to be writable by the parent.  In that case, the only way to do this as far as I know is to create a temporary file and open it twice, as read/write and read-only, before unlinking it.  This also excludes optimizations like Linux's memfd_create and FreeBSD's SHM_ANON, which create truly anonymous shared memory.  (For memfd_create there's F_SEAL_WRITE but that applies to the file, not individual file descriptors.)

On MacOS we use Mach shared memory in IPC, and offhand I have no idea how that works as far as access rights, but POSIX APIs are also available.

Android may be a problem — the recommended API, ashmem, is similar to SHM_ANON/memfd_create and doesn't seem able to allow read-only access to a writable memory area.  There is ASHMEM_SET_PROT_MASK, but that looks like it applies to the file; it might not check for consistency with existing mappings like F_SEAL_WRITE does, but if so that might also be considered a bug that gets fixed someday.  I don't know if we'd have access to someplace where we can write temporary files.  We also don't have sandboxing for GeckoView yet, but we'll probably need to deal with this at some point.
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #1)
> There are some sandboxing implications here — we don't want one content
> process to be able to overwrite the shared memory and attack other processes.

Agreed. My intention is for the content side to be read-only, and for the data to only ever be updated in the parent process.

> Unix is harder.  If the data won't change, then it could be mapped
> MAP_PRIVATE (and make absolutely sure the fd is closed before the process
> starts handling hostile data), but given the verbiage about “synchronizing
> data updates” it may need to be writable by the parent.

Unix isn't that hard. We already do this in places (e.g., the script preloader). My intention was for updates to happen in the parent process and send a new blob of the entire key-value store to all child processes, which would then drop their references to the old blob.

Or something along those lines. I'm expecting most keys to be smaller than a single page, so I don't think piecemeal updates are a good idea.

> Android may be a problem — the recommended API, ashmem, is similar to
> SHM_ANON/memfd_create and doesn't seem able to allow read-only access to a
> writable memory area.

For now, I'm not super worried about Android. When we figure out what the e10s situation will be there, we can figure out how this should be handled, but for now, I'm not sure it should be a concern.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #2)
> Unix isn't that hard. We already do this in places (e.g., the script
> preloader). My intention was for updates to happen in the parent process and
> send a new blob of the entire key-value store to all child processes, which
> would then drop their references to the old blob.
> 
> Or something along those lines. I'm expecting most keys to be smaller than a
> single page, so I don't think piecemeal updates are a good idea.

That does simplify things, and thanks for the pointer to the script preloader / AutoMemMap, in which you're already doing the thing I was suggesting (and which I'd assumed would be a feature request for IPC shared memory, because I didn't know there was already code for it).
Blocks: 1470365
I'm probably going to refactor some of the mmap bits of this before too long. I wrote the Unix parts first, with the horribly misguided assumption that I'd be able to just pass around file descriptors for the mappings on Windows too. After I gave up on that plan, I wound up having to thread a size argument through all of the IPC bits, and add a gigantic special case for Windows to AutoMemMap.
Blocks: 1470783
Comment on attachment 8987317 [details]
Bug 1463587: Part 4 - Add blob support to SharedMap.

https://reviewboard.mozilla.org/r/252560/#review259122

::: dom/ipc/SharedMap.cpp:113
(Diff revision 1)
>    return *mMapFile;
>  }
>  
>  void
>  SharedMap::Update(const FileDescriptor& aMapFile, size_t aMapSize,
> +                   nsTArray<RefPtr<BlobImpl>>&& aBlobs,

indentation
Attachment #8987317 - Flags: review?(amarchesini) → review+
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review259124

::: dom/ipc/SharedMap.cpp:164
(Diff revision 1)
> +}
> +
> +JS::Value
> +SharedMap::GetValueAtIndex(uint32_t aIndex) const
> +{
> +  JSContext* cx = danger::GetJSContext();

I would like bz to review this.

::: dom/ipc/SharedMapChangeEvent.h:19
(Diff revision 1)
> +
> +namespace mozilla {
> +namespace dom {
> +namespace ipc {
> +
> +class SharedMapChangeEvent : public Event

final?
Attachment #8987316 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #12)
> ::: dom/ipc/SharedMap.cpp:164
> (Diff revision 1)
> > +}
> > +
> > +JS::Value
> > +SharedMap::GetValueAtIndex(uint32_t aIndex) const
> > +{
> > +  JSContext* cx = danger::GetJSContext();
> 
> I would like bz to review this.

So would I, but he's not accepting review requests at the moment... :(

Thanks.
Flags: needinfo?(bzbarsky)
Comment on attachment 8987314 [details]
Bug 1463587: Part 1 - Add helper class for creating snapshots of shared memory regions.

https://reviewboard.mozilla.org/r/252554/#review259844

Generally seems good, just a few minor issues.

::: dom/ipc/MemMapSnapshot.cpp:68
(Diff revision 1)
> +}
> +
> +Result<Ok, nsresult>
> +MemMapSnapshot::Freeze(AutoMemMap& aMem)
> +{
> +  auto orig = mFile.ref().ClonePlatformHandle();

It's a little odd that we duplicate the handle, close the original and then duplicate the handle again and close the duplicated duplicate handle.

::: dom/ipc/MemMapSnapshot.cpp:74
(Diff revision 1)
> +  mFile.reset();
> +
> +  HANDLE handle;
> +  if (!::DuplicateHandle(GetCurrentProcess(), orig.release(), GetCurrentProcess(),
> +                         &handle, GENERIC_READ | FILE_MAP_READ,
> +                         false, DUPLICATE_CLOSE_SOURCE)) {

We still want to close the handle right?

::: dom/ipc/MemMapSnapshot.cpp:118
(Diff revision 1)
> +  // since we open and share a read-only file descriptor, and then delete the
> +  // file. But it doesn't hurt, either.
> +  chmod(mPath.get(), 0400);
> +
> +  nsCOMPtr<nsIFile> file;
> +  MOZ_TRY(NS_NewNativeLocalFile(mPath, false, getter_AddRefs(file)));

nit: `/*whatever =*/ false`

::: js/xpconnect/loader/AutoMemMap.cpp:130
(Diff revision 1)
> +#else
> +
> +Result<Ok, nsresult>
> +AutoMemMap::init(const FileDescriptor& file, size_t size, PRFileMapProtect prot)
> +{
> +    return init(file, prot);

It doesn't seem right to ignore `size` here. If it's okay can you add a note? Also maybe assert that `_size == size`
Attachment #8987314 - Flags: review?(erahm) → review-
Comment on attachment 8987314 [details]
Bug 1463587: Part 1 - Add helper class for creating snapshots of shared memory regions.

https://reviewboard.mozilla.org/r/252554/#review259844

> It's a little odd that we duplicate the handle, close the original and then duplicate the handle again and close the duplicated duplicate handle.

It is odd, yes, but that is unfortunately the way the FileDescriptor class works. It clones any descriptor you give it, and you can only get a descriptor out of it by cloning. It's a bit annoying, but it's cheap enough.

> We still want to close the handle right?

You mean if we fail? Yes, DuplicateHandle is guaranteed to close the handle when you pass DUPLICATE_CLOSE_SOURCE even if it fails.

> It doesn't seem right to ignore `size` here. If it's okay can you add a note? Also maybe assert that `_size == size`

On *nix, we can always get the size by statting the file descriptor, so the size we pass doesn't matter. I thought about adding a version of `initInternal` that didn't stat, but it didn't seem worth the effort. I suppose asserting couldn't hurt, though.
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review259858

Overall looks okay, I just have a bunch of questions and minor suggestions.

::: dom/ipc/SharedMap.h:43
(Diff revision 1)
> + * future, it will happen automatically in idle tasks.
> + *
> + *
> + * Whenever a read-only SharedMap is updated, it dispatches a "change" event.
> + * The event contains a "changedKeys" property with a list of all keys which
> + * were changed in the last update batch. Change events are never dispatched to 

The comment appears to be truncated.

::: dom/ipc/SharedMap.h:45
(Diff revision 1)
> + *
> + * Whenever a read-only SharedMap is updated, it dispatches a "change" event.
> + * The event contains a "changedKeys" property with a list of all keys which
> + * were changed in the last update batch. Change events are never dispatched to 
> + */
> +class SharedMap : public nsISupports

Do we really require `nsISupports` for RefPtr's?

::: dom/ipc/SharedMap.h:54
(Diff revision 1)
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  SharedMap();
> +
> +  SharedMap(nsIGlobalObject* aGlobal, const FileDescriptor&, size_t);

I'm guessing we want to delete the copy/move ctors.

::: dom/ipc/SharedMap.h:67
(Diff revision 1)
> +  /**
> +   * Returns a copy of the read-only file descriptor which backs the shared
> +   * memory region for this map. The file descriptor may be passed between
> +   * processes, and used to update corresponding instances in child preocesses.
> +   */
> +  FileDescriptor GetMapFile();

Maybe call this `CloneMapFile`?

::: dom/ipc/SharedMap.h:151
(Diff revision 1)
> +  private:
> +    SharedMap& mMap;
> +
> +    nsCString mName;
> +
> +    MaybeOneOf<uint32_t, StructuredCloneData> mData;

Can you add a comment as to what the state of this means?

::: dom/ipc/SharedMap.cpp:40
(Diff revision 1)
> +
> +
> +SharedMap::SharedMap()
> +{}
> +
> +SharedMap::SharedMap(nsIGlobalObject* aGlobal, const FileDescriptor& aMapFile,

Why is `aGlobal` ignored?

::: dom/ipc/SharedMap.cpp:51
(Diff revision 1)
> +
> +
> +bool
> +SharedMap::Has(const nsACString& aName)
> +{
> +  return mEntries.Get(aName);

nit: `Contains` is a little more appropriate.

::: dom/ipc/SharedMap.cpp:87
(Diff revision 1)
> +    holder.Read(aCx, aRetVal, aRv);
> +    return;
> +  } else {
> +    StructuredCloneData holder;
> +    if (holder.CopyExternalData(Data(), Size())) {
> +      holder.Read(aCx, aRetVal, aRv);

De we want to set `mData` to this as well?

::: dom/ipc/SharedMap.cpp:105
(Diff revision 1)
> +  return *mMapFile;
> +}
> +
> +void
> +SharedMap::Update(const FileDescriptor& aMapFile, size_t aMapSize,
> +                  nsTArray<nsCString>&& aChangedKeys)

Why are we ignoring `aChangedKeys`?

::: dom/ipc/SharedMap.cpp:110
(Diff revision 1)
> +                  nsTArray<nsCString>&& aChangedKeys)
> +{
> +  MOZ_DIAGNOSTIC_ASSERT(!mWritable);
> +
> +  mMap.reset();
> +  mMapFile.reset(new FileDescriptor(aMapFile));

Could this just be `*mMapFile = aMapFile`?

::: dom/ipc/SharedMap.cpp:125
(Diff revision 1)
> +
> +  mSize = Holder().Data().Size();
> +}
> +
> +void
> +SharedMap::Entry::ExtractData(char* aDestPtr, uint32_t aNewOffset)

We should probably pass in `aDestPtr`'s size and make sure we don't write out of bounds.

::: dom/ipc/SharedMap.cpp:165
(Diff revision 1)
> +
> +  for (uint32_t i = 0; i < count; i++) {
> +    auto entry = MakeUnique<Entry>(*this);
> +    entry->Code(buffer);
> +
> +    // This buffer was created at runtime, duirng this session, so any errors

nit: typo, duiring

::: dom/ipc/SharedMap.cpp:169
(Diff revision 1)
> +
> +    // This buffer was created at runtime, duirng this session, so any errors
> +    // indicate memory corruption, and are fatal.
> +    MOZ_RELEASE_ASSERT(!buffer.error());
> +
> +    mEntries.Put(entry->Name(), entry.get());

You can just `release` here, `Put` is infallible.

::: dom/ipc/SharedMap.cpp:186
(Diff revision 1)
> +
> +WritableSharedMap::WritableSharedMap()
> +  : SharedMap()
> +{
> +  mWritable = true;
> +  Unused << Serialize();

Can you add a note about why we're serializing an empty map? Is it just to create the mapping?

::: dom/ipc/SharedMap.cpp:251
(Diff revision 1)
> +{
> +  if (mChangedKeys.IsEmpty()) {
> +    return;
> +  }
> +
> +  Unused << Serialize();

Do we broadcast if `Serialize` fails?
Attachment #8987315 - Flags: review?(erahm) → review-
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review259858

> The comment appears to be truncated.

Hm. Yes. Rebase botch, I think.

> Do we really require `nsISupports` for RefPtr's?

We require it for DOM classes, and the next revision adds DOM bindings to this class.

> I'm guessing we want to delete the copy/move ctors.

They should be implicitly deleted for any class with an explicit desctuctor.

> Why is `aGlobal` ignored?

It's used in the next patch when I add EventEmitter bindings.

> De we want to set `mData` to this as well?

Nope. That would wind up making a long-lived process-local copy of the mmapped data. It's better that we just create a new holder the next time we need to decode the data.

> Why are we ignoring `aChangedKeys`?

It's used in the next patch where I add event dispatch.

> Could this just be `*mMapFile = aMapFile`?

Only if there's already a FileDescriptor there. I suppose it might make sense to use the copy constructor in that case, though.

> You can just `release` here, `Put` is infallible.

You'd *think*, but annoyingly, order of evaluation isn't guaranteed here, and if I .release() instead of .get(), in some builds, entry->Name() winds up with a null entry.

> Can you add a note about why we're serializing an empty map? Is it just to create the mapping?

Yeah.

> Do we broadcast if `Serialize` fails?

Yeah. I suppose it makes more sense to return instead...
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review260168

::: dom/ipc/SharedMap.cpp:87
(Diff revision 1)
> +    holder.Read(aCx, aRetVal, aRv);
> +    return;
> +  } else {
> +    StructuredCloneData holder;
> +    if (holder.CopyExternalData(Data(), Size())) {
> +      holder.Read(aCx, aRetVal, aRv);

Okay so it's only a StructuredCloneData if `Set` was called. Can you add a note that we choose to rebuild rather than waste memory?

::: dom/ipc/SharedMap.cpp:169
(Diff revision 1)
> +
> +    // This buffer was created at runtime, duirng this session, so any errors
> +    // indicate memory corruption, and are fatal.
> +    MOZ_RELEASE_ASSERT(!buffer.error());
> +
> +    mEntries.Put(entry->Name(), entry.get());

Ugh yeah. This brings up something that was bugging, but wasn't a huge deal: we're duplicating the entry name -- it might get shared but it's still kinda wasteful. Improving that would probably be more of a follow up.
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review260180

I don't feel confident reviewing the webidl bits, I'll defer to baku and bz on that.

::: dom/ipc/SharedMap.h:174
(Diff revision 1)
>      MaybeOneOf<uint32_t, StructuredCloneData> mData;
>  
>      uint32_t mSize = 0;
>    };
>  
> +  nsTArray<Entry*>& EntryArray() const;

Can this be a const ref instead?

::: dom/ipc/SharedMap.cpp:168
(Diff revision 1)
> +{
> +  JSContext* cx = danger::GetJSContext();
> +
> +  IgnoredErrorResult rv;
> +  JS::RootedValue val(cx);
> +  EntryArray()[aIndex]->Read(cx, &val, rv);

This will result in a crash if the index is out of bounds, it's not clear to me whether or not that's desired behavior for webidl things.
Attachment #8987316 - Flags: review?(erahm) → review+
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review260180

Fair enough. That's why I put the WebIDL bits in a separate patch.

> Can this be a const ref instead?

Probably. I always get a headache trying to figure out the constness of the pointer target with a `const nsTArray<*>` ...

> This will result in a crash if the index is out of bounds, it's not clear to me whether or not that's desired behavior for webidl things.

The binding layer always checks the index against `GetIterableLength()` before calling key or value getters. I would have added an assertion for that, but `nsTArray` already does bounds checks, so it seemed redundant.

I can add a comment, if you'd like, though.
Comment on attachment 8987317 [details]
Bug 1463587: Part 4 - Add blob support to SharedMap.

https://reviewboard.mozilla.org/r/252560/#review260188

Looks good, just some minor comments.

::: dom/ipc/SharedMap.h:165
(Diff revision 1)
>      }
>  
> +    uint16_t BlobOffset() const { return mBlobOffset; }
> +    uint16_t BlobCount() const { return mBlobCount; }
> +
> +    RefPtr<BlobImpl>* Blobs()

Pointer to a RefPtr is a bit odd. I guess I see what's going on, maybe make this private? Bah, I guess that doesn't work either. Returning a span instead might be cleaner, but I don't feel to strongly.

::: dom/ipc/SharedMap.cpp:349
(Diff revision 1)
> +        continue;
> +      }
> +    }
> +
>      Unused << parent->SendUpdateSharedData(GetMapFile(), mMap.size(),
> -                                           mChangedKeys);
> +                                           blobs, mChangedKeys);

Can we `move` the `blobs`? Or does it not matter.
Attachment #8987317 - Flags: review?(erahm) → review+
Comment on attachment 8987317 [details]
Bug 1463587: Part 4 - Add blob support to SharedMap.

https://reviewboard.mozilla.org/r/252560/#review260188

> Pointer to a RefPtr is a bit odd. I guess I see what's going on, maybe make this private? Bah, I guess that doesn't work either. Returning a span instead might be cleaner, but I don't feel to strongly.

I'll see if I can make it a span without too much effort. I agree it's a bit odd.

It can't be made private, since the outer class uses it. The whole Entry type is private, though.

> Can we `move` the `blobs`? Or does it not matter.

Nope. The IPC layer doesn't support moves on these types of arguments. But it also doesn't matter, since they get serialized into the IPC buffer directly from the array reference we pass.
Comment on attachment 8987318 [details]
Bug 1463587: Part 5 - Add tests for SharedMap.

https://reviewboard.mozilla.org/r/252562/#review260200

Thanks for adding the tests! Do you think we should add blobs as well? Or should that be a separate follow up?

::: dom/ipc/tests/test_sharedMap.js:73
(Diff revision 1)
> +  let expected = [
> +    ["foo-a", {"foo": "a"}],
> +    ["foo-b", {"foo": "b"}],
> +    ["bar-c", null],
> +    ["bar-d", 42],
> +  ];

Okay so we're testing structured clones, but what about blobs?

::: dom/ipc/tests/test_sharedMap.js:85
(Diff revision 1)
> +  function deleteKey(key) {
> +    sharedData.delete(key);
> +    expected = expected.filter(([k]) => k != key);
> +  }
> +
> +  for (let [key, val] of expected) {

Can you add some comments about what each block is testing? As "set keys, expect parent to update but not children," "expect children to update after explicit flush," "test updating a a key," etc
Attachment #8987318 - Flags: review?(erahm) → review+
Comment on attachment 8987318 [details]
Bug 1463587: Part 5 - Add tests for SharedMap.

https://reviewboard.mozilla.org/r/252562/#review260200

> Okay so we're testing structured clones, but what about blobs?

Yeah, was kind of on the fence about testing blobs. I added blob support after I wrote this test, and thought about adding additional tests for it. I think the tests for the current code that uses blobs with initialProcessData may be enough, since this API is only used by in-tree code. But they'd be easy enough to add, so I suppose I may as well.
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review259858

> We should probably pass in `aDestPtr`'s size and make sure we don't write out of bounds.

The size of the buffer is based on mSize, which comes from Holder.Data()'s size. So the space we have available is always equal to mSize. But it probably wouldn't hurt to assert that the total written size matches mSize.
Comment on attachment 8987319 [details]
Bug 1463587: Part 6 - Add an idle flush task to WritableSharedMap.

https://reviewboard.mozilla.org/r/252564/#review260206

Looks good, just a few questions.

::: dom/ipc/SharedMap.cpp:403
(Diff revision 1)
>  }
>  
>  void
> +WritableSharedMap::IdleFlush()
> +{
> +  mPendingFlush = false;

This seems like we're going to have `IdleFlush` and `Flush` racing. Maybe in practice that's a non-issue b/c `BroadcastChanges` only broadcasts if there are actual changes.

::: dom/ipc/SharedMap.cpp:416
(Diff revision 1)
>      mChangedKeys.InsertElementSorted(aName);
>    }
>    mEntryArray.reset();
> +
> +  if (!mPendingFlush) {
> +      MOZ_TRY(NS_IdleDispatchToCurrentThread(

I guess there are two considerations here:
  1. are we worried about this firing too often?
  2. are we worried about this getting starved?
Attachment #8987319 - Flags: review?(erahm) → review+
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review260168

> Ugh yeah. This brings up something that was bugging, but wasn't a huge deal: we're duplicating the entry name -- it might get shared but it's still kinda wasteful. Improving that would probably be more of a follow up.

I'm pretty sure the string data is guaranteed to be shared in this case. We do have an extra nsCString instance for the key, but with the number of keys we should be creating in general, I don't think that should add more than a handful of bytes per process. And even so, we wind up with much less per-process memory overhead here than we had with initialProcessData.
Comment on attachment 8987319 [details]
Bug 1463587: Part 6 - Add an idle flush task to WritableSharedMap.

https://reviewboard.mozilla.org/r/252564/#review260206

> This seems like we're going to have `IdleFlush` and `Flush` racing. Maybe in practice that's a non-issue b/c `BroadcastChanges` only broadcasts if there are actual changes.

Right. We only broadcast if there are changes, so if a Flush() happens while an idle flush is pending, the idle flush becomes a no-op. If there were a way to cancel pending idle tasks, I'd do that instead. But there isn't.

> I guess there are two considerations here:
>   1. are we worried about this firing too often?
>   2. are we worried about this getting starved?

I'm not worried about it firing too often, no. I think key changes in this map should be relatively rare, and should generally happen in groups if many changes are happening in a short amount of time. That's the case with the code that I've converted so far anyway.

And the flush operation should be relatively cheap, so given that it's happening in idle slices anyway, I don't think it should be a concern.

I'm *slightly* worried about it getting starved, yes. Code that depends on the flush happening in a specific timeframe should just explicitly flush anyway, but we should make sure that other flushes happen in a reasonable timeframe.

But unfortunately, there isn't really a useful helper for scheduling idle tasks that have a deadline except from JS, so I think this is good enough for the initial implementation. We can tweak later if necessary.
Comment on attachment 8987314 [details]
Bug 1463587: Part 1 - Add helper class for creating snapshots of shared memory regions.

https://reviewboard.mozilla.org/r/252554/#review260222

Thanks for the udpates.
Attachment #8987314 - Flags: review?(erahm) → review+
Attachment #8987315 - Flags: review?(erahm)
Attachment #8987316 - Flags: review?(bzbarsky)
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review260606

::: commit-message-6ea9c:3
(Diff revision 2)
> +This is the first basic implementation of a shared-memory key-value store JS
> +message managers. It has one read-write endpoint in the parent process, and

It seems like there's a preposition missing somewhere in there.  "for JS message managers"?  "in JS message managers"?  Something else?

::: dom/chrome-webidl/MozSharedMap.webidl:11
(Diff revision 2)
> +
> +typedef any StructuredClonable;
> +
> +[ChromeOnly]
> +interface MozSharedMapChangeEvent : Event {
> +  [Cached, Pure]

Pure or Constant?

::: dom/chrome-webidl/MozSharedMap.webidl:12
(Diff revision 2)
> +typedef any StructuredClonable;
> +
> +[ChromeOnly]
> +interface MozSharedMapChangeEvent : Event {
> +  [Cached, Pure]
> +  readonly attribute sequence<ByteString> changedKeys;

ByteString, not DOMString?  These are just opaque byte buffers?

::: dom/chrome-webidl/MozSharedMap.webidl:26
(Diff revision 2)
> +  boolean has(ByteString name);
> +
> +  [Throws]
> +  StructuredClonable get(ByteString name);
> +
> +  iterable<DOMString, StructuredClonable>;

DOMString, not ByteString?  Shouldn't we be consistent about this part?

::: dom/ipc/SharedMap.cpp:136
(Diff revision 2)
> +  init.mBubbles = false;
> +  init.mCancelable = false;

Those are the default values, so they're already set.

::: dom/ipc/SharedMap.cpp:170
(Diff revision 2)
> +}
> +
> +const nsString
> +SharedMap::GetKeyAtIndex(uint32_t aIndex) const
> +{
> +  return NS_ConvertUTF8toUTF16(EntryArray()[aIndex]->Name());

Could just return a reference with no copying here if the type in the IDL matched out stored type.

::: dom/ipc/SharedMap.cpp:178
(Diff revision 2)
> +JS::Value
> +SharedMap::GetValueAtIndex(uint32_t aIndex) const
> +{
> +  JSContext* cx = danger::GetJSContext();
> +
> +  IgnoredErrorResult rv;

Just use IgnoreErrors() at the callsite.

::: dom/ipc/SharedMap.cpp:180
(Diff revision 2)
> +{
> +  JSContext* cx = danger::GetJSContext();
> +
> +  IgnoredErrorResult rv;
> +  JS::RootedValue val(cx);
> +  EntryArray()[aIndex]->Read(cx, &val, rv);

So this is relying on cx being in the right compartment already, right?  That's pretty fishy, generally speaking.  Probably true for the binding callers, sort of... or at least in _a_ compartment.

How do you feel about explicitly entering the compartment of our wrapper here, failing out (e.g. returning undefined) if we have no wrapper?

::: dom/ipc/SharedMap.cpp:180
(Diff revision 2)
> +{
> +  JSContext* cx = danger::GetJSContext();
> +
> +  IgnoredErrorResult rv;
> +  JS::RootedValue val(cx);
> +  EntryArray()[aIndex]->Read(cx, &val, rv);

If Read() fails, you presumably want to clear the exception and return undefined or some such....  Or can Read not leave an exception on the JSContext?

::: dom/ipc/SharedMap.cpp:415
(Diff revision 2)
> +  bool trusted = event->Init(aEventTarget);
> +  event->InitEvent(aType, aInit.mBubbles, aInit.mCancelable);
> +  event->SetTrusted(trusted);
> +  event->SetComposed(aInit.mComposed);
> +
> +  event->mChangedKeys.AppendElements(aInit.mChangedKeys);

AppendElements, or just operator=?
Attachment #8987316 - Flags: review?(bzbarsky) → review-
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review260606

> Pure or Constant?

Should probably be `Constant, Frozen`, yeah. When I added `Cached`, the error message told me to add `Pure`. *shrug*

> ByteString, not DOMString?  These are just opaque byte buffers?

They're UTF-8 strings, which is what ByteString means in practice. Changing it to DOMString would require making an extra temporary array in order to initialize the event.

> DOMString, not ByteString?  Shouldn't we be consistent about this part?

We *should*, yes. But unfortunately, ToJSValue doesn't work with nsCStrings, so I had to change this in order for it to compile.

> Could just return a reference with no copying here if the type in the IDL matched out stored type.

We could if ToJSValue supported nsCStrings.

> So this is relying on cx being in the right compartment already, right?  That's pretty fishy, generally speaking.  Probably true for the binding callers, sort of... or at least in _a_ compartment.
> 
> How do you feel about explicitly entering the compartment of our wrapper here, failing out (e.g. returning undefined) if we have no wrapper?

I think this is a valid assumption for the sake of IDL callers. Those callers call ToJSValue on the return type with the current context, so if they're not in the right compartment already, we're in trouble.

I don't think that assertion would be valid for all callers here. The same SharedMap will be used by, say, the shared JSM compartment and arbitrary frame scripts, which will be in different compartments. I'm not actually sure any of them will be in the same compartment as the ppmm wrapper...

> If Read() fails, you presumably want to clear the exception and return undefined or some such....  Or can Read not leave an exception on the JSContext?

I don't think it can, but I'll double check.
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review260606

> I think this is a valid assumption for the sake of IDL callers. Those callers call ToJSValue on the return type with the current context, so if they're not in the right compartment already, we're in trouble.
> 
> I don't think that assertion would be valid for all callers here. The same SharedMap will be used by, say, the shared JSM compartment and arbitrary frame scripts, which will be in different compartments. I'm not actually sure any of them will be in the same compartment as the ppmm wrapper...

Hm. Actually, I guess since we'd be accessing the API through an ordinary CCW in that case rather than an X-ray, the context should be in the same compartment as the wrapper.
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review260606

> Should probably be `Constant, Frozen`, yeah. When I added `Cached`, the error message told me to add `Pure`. *shrug*

> Should probably be Constant, Frozen, yeah.

I wouldn't bother with Frozen.  It's more work for no real benefit here.

The error message should maybe be clearer that what's needed is "Pure or stronger"....

> They're UTF-8 strings, which is what ByteString means in practice. Changing it to DOMString would require making an extra temporary array in order to initialize the event.

> They're UTF-8 strings, which is what ByteString means in practice.

No, ByteString means bag of bytes.  The conversion from JS string to ByteString just drops the high byte from every 16-bit code unit.  If you're ASCII, you're OK.  If you're within the Latin1 range, you get valid Latin1.  If you're other non-ASCII you get dataloss.  You definitely do not get out UTF-8 except in the ASCII case.

> We *should*, yes. But unfortunately, ToJSValue doesn't work with nsCStrings, so I had to change this in order for it to compile.

> But unfortunately, ToJSValue doesn't work with nsCStrings

Ah, right, because on the C++ side we don't differentiate between "UTF-8 string" and "bag of bytes" on the typesystem level in a way that ToJSValue could then use to do the right conversion.

The conversion here assumes UTF-8, which in practice means that if there are any non-ASCII keys you will get mojibake.  I'm pretty much assuming that everything involved is ASCII; that may be worth explicitly documenting.  And maybe using NS_ConvertASCIItoUTF16 so at least the incoming and outgoing conversions are the same.

> I don't think it can, but I'll double check.

I just checked.  We bottom out in StructuredCloneHolder::ReadFromBuffer which clears the JSContext exception.  Good.

> Hm. Actually, I guess since we'd be accessing the API through an ordinary CCW in that case rather than an X-ray, the context should be in the same compartment as the wrapper.

> I think this is a valid assumption for the sake of IDL callers.

Right.  Basically, it bothers me to have the danger::GetJSContext here, but the way the iterable infrastructure works makes it hard to do anything else.  We should at the very least document clearly why this is safe in this instance and why it has to be done at all.  And maybe file a followup bug to have IterableIterator be specialized for the JSObject/JS::Value cases to pass in a JSContext?  I have some ideas on how to fix that...

> I don't think that assertion would be valid for all callers here

Which assertion?

Anyway, if we do the IterableIterator fix above this will become moot.
Flags: needinfo?(bzbarsky)
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review260606

> > They're UTF-8 strings, which is what ByteString means in practice.
> 
> No, ByteString means bag of bytes.  The conversion from JS string to ByteString just drops the high byte from every 16-bit code unit.  If you're ASCII, you're OK.  If you're within the Latin1 range, you get valid Latin1.  If you're other non-ASCII you get dataloss.  You definitely do not get out UTF-8 except in the ASCII case.

Hm. So it does.

> > I think this is a valid assumption for the sake of IDL callers.
> 
> Right.  Basically, it bothers me to have the danger::GetJSContext here, but the way the iterable infrastructure works makes it hard to do anything else.  We should at the very least document clearly why this is safe in this instance and why it has to be done at all.  And maybe file a followup bug to have IterableIterator be specialized for the JSObject/JS::Value cases to pass in a JSContext?  I have some ideas on how to fix that...
> 
> > I don't think that assertion would be valid for all callers here
> 
> Which assertion?
> 
> Anyway, if we do the IterableIterator fix above this will become moot.

I'll file a follow-up.
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review260224

Looks good, thanks.
Attachment #8987315 - Flags: review?(erahm) → review+
See Also: → 1472342
Comment on attachment 8987314 [details]
Bug 1463587: Part 1 - Add helper class for creating snapshots of shared memory regions.

https://reviewboard.mozilla.org/r/252554/#review260814

::: dom/ipc/MemMapSnapshot.cpp:87
(Diff revision 2)
> +
> +Result<Ok, nsresult>
> +MemMapSnapshot::Create(size_t aSize)
> +{
> +  FilePath path;
> +  ScopedCloseFile fd(file_util::CreateAndOpenTemporaryShmemFile(&path));

This is going to make my life harder at some point in the distant future when I can get back to bug 1426526.

::: dom/ipc/MemMapSnapshot.cpp:92
(Diff revision 2)
> +  ScopedCloseFile fd(file_util::CreateAndOpenTemporaryShmemFile(&path));
> +  if (!fd) {
> +    return Err(NS_ERROR_FAILURE);
> +  }
> +
> +  if (ftruncate(fileno(fd), aSize) != 0) {

You might want to `HANDLE_EINTR` this.

::: js/xpconnect/loader/AutoMemMap.h:42
(Diff revision 2)
> -        init(const ipc::FileDescriptor& file);
> +        init(const ipc::FileDescriptor& file,
> +             PRFileMapProtect prot = PR_PROT_READONLY,
> +             size_t expectedSize = 0);
> +
> +        Result<Ok, nsresult>
> +        init(const ipc::FileDescriptor& file, size_t size,

These two `init` signatures are very similar and it's not obvious which one does what.  Maybe one of them could have a different name?

::: js/xpconnect/loader/AutoMemMap.h:74
(Diff revision 2)
>  
>      private:
> -        Result<Ok, nsresult> initInternal(PRFileMapProtect prot = PR_PROT_READONLY);
> +        Result<Ok, nsresult> initInternal(PRFileMapProtect prot = PR_PROT_READONLY,
> +                                          size_t expectedSize = 0);
> +
> +        Result<Ok, nsresult> initInternal(PRFileMapProtect prot, size_t size);

I'm confused: aren't these two `initInternal` signatures the same?  Am I missing something?  Why does the compiler allow defining it twice?
Attachment #8987314 - Flags: review?(jld) → review+
Comment on attachment 8987314 [details]
Bug 1463587: Part 1 - Add helper class for creating snapshots of shared memory regions.

https://reviewboard.mozilla.org/r/252554/#review260814

> This is going to make my life harder at some point in the distant future when I can get back to bug 1426526.

I'll probably update it to use POSIX/Android SHM APIs before that point, if I have the time.

> I'm confused: aren't these two `initInternal` signatures the same?  Am I missing something?  Why does the compiler allow defining it twice?

Ugh. Yes, some sort of weird rebase bustage happened here. There's only one initInternal method with this signature. The removal of the duplicate wound up in a completely unrelated commit.
Keywords: leave-open
Comment on attachment 8987314 [details]
Bug 1463587: Part 1 - Add helper class for creating snapshots of shared memory regions.

https://reviewboard.mozilla.org/r/252554/#review260814

> These two `init` signatures are very similar and it's not obvious which one does what.  Maybe one of them could have a different name?

Renamed this one to `initWithHandle` and added a comment about the difference.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5bf63c13445d926553f85ed9381ca38d15a0781
Bug 1463587: Part 1 - Add helper class for creating snapshots of shared memory regions. r=jld,erahm
Keywords: leave-open
Whiteboard: [overhead:noted]
Blocks: 1474163
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review262982

::: dom/ipc/SharedMap.h:46
(Diff revision 3)
> + * Whenever a read-only SharedMap is updated, it dispatches a "change" event.
> + * The event contains a "changedKeys" property with a list of all keys which
> + * were changed in the last update batch. Change events are never dispatched to
> + * WritableSharedMap instances.
> + */
> +class SharedMap : public nsISupports

Does this actually need to be nsISupports.  It's probably fine to land like this but worth thinking about whether that can be removed.

::: dom/ipc/SharedMap.h:57
(Diff revision 3)
> +
> +  SharedMap();
> +
> +  SharedMap(nsIGlobalObject* aGlobal, const FileDescriptor&, size_t);
> +
> +  bool Has(const nsACString& name);

Should document encoding of that name string, given that our keys are UTF-16.  Presumably UTF-8 encoded?

::: dom/ipc/SharedMap.h:59
(Diff revision 3)
> +
> +  SharedMap(nsIGlobalObject* aGlobal, const FileDescriptor&, size_t);
> +
> +  bool Has(const nsACString& name);
> +
> +  void Get(JSContext* cx, const nsACString& name, JS::MutableHandleValue aRetVal,

Again, encoding should be documented.

::: dom/ipc/SharedMap.h:70
(Diff revision 3)
> +   * memory region for this map. The file descriptor may be passed between
> +   * processes, and used to update corresponding instances in child preocesses.
> +   */
> +  FileDescriptor CloneMapFile();
> +
> +  size_t GetMapSize() const { return mMap.size(); }

It might be nicer to name this MapSize()?

::: dom/ipc/SharedMap.h:77
(Diff revision 3)
> +  /**
> +   * Updates this instance to reflect the contents of the shared memory region
> +   * in the given map file, and broadcasts a change event for the given set of
> +   * changed keys.
> +   */
> +  void Update(const FileDescriptor& aMapFile, size_t aMapSize,

UTF-8 keys?

::: dom/ipc/SharedMap.h:99
(Diff revision 3)
> +    }
> +
> +    ~Entry() = default;
> +
> +    template<typename Buffer>
> +    void Code(Buffer& buffer)

Please document?

::: dom/ipc/SharedMap.h:107
(Diff revision 3)
> +
> +      buffer.codeString(mName);
> +      buffer.codeUint32(DataOffset());
> +      buffer.codeUint32(mSize);
> +
> +      MOZ_ASSERT(buffer.cursor() = startOffset + HeaderSize());

==, not =, right?

::: dom/ipc/SharedMap.h:117
(Diff revision 3)
> +      return (sizeof(uint16_t) + mName.Length() +
> +              sizeof(DataOffset()) +
> +              sizeof(mSize));
> +    }
> +
> +    void TakeData(StructuredCloneData&&);

Please document.

::: dom/ipc/SharedMap.h:119
(Diff revision 3)
> +              sizeof(mSize));
> +    }
> +
> +    void TakeData(StructuredCloneData&&);
> +
> +    void ExtractData(char* aDestPtr, uint32_t aNewOffset);

Please document.  Especially in terms of ownership model and allocator behavior for aDestPtr.  Most simply, is it allocated with malloc, or new, or new[] or something else?  That affects how it should be freed....

::: dom/ipc/SharedMap.h:123
(Diff revision 3)
> +
> +    void ExtractData(char* aDestPtr, uint32_t aNewOffset);
> +
> +    const nsCString& Name() const { return mName; }
> +
> +    void Read(JSContext* aCx, JS::MutableHandleValue aRetVal,

Document.

::: dom/ipc/SharedMap.h:131
(Diff revision 3)
> +    const char* Data() const
> +    {
> +      return mMap.Data() + DataOffset();
> +    }
> +
> +    uint32_t& DataOffset()

So I'm at a bit of a loss as to how this function can be used.  It's a public function, but it asserts that the Entry is in the uint32_t state.  At the same time there is no obvious public way to check what state the Entry is in.  How can a consumer know whether this function is OK to call?

Similar for most of the other APIs that touch mData.

::: dom/ipc/SharedMap.h:150
(Diff revision 3)
> +    uint32_t Size() const { return mSize; }
> +
> +  private:
> +    SharedMap& mMap;
> +
> +    nsCString mName;

Is this the same as the key?  Should probably document what this member means.

::: dom/ipc/SharedMap.h:167
(Diff revision 3)
> +     *   data. This will be discarded the next time the map is serialized, and
> +     *   replaced with a buffer offset, as described above.
> +     */
> +    MaybeOneOf<uint32_t, StructuredCloneData> mData;
> +
> +    uint32_t mSize = 0;

What does this member mean?

::: dom/ipc/SharedMap.h:178
(Diff revision 3)
> +  // Note: This header is included by WebIDL binding headers, and therefore
> +  // can't include "windows.h". Since FileDescriptor.h does include "windows.h"
> +  // on Windows, we can only forward declare FileDescriptor, and can't include
> +  // it as an inline member.
> +  UniquePtr<FileDescriptor> mMapFile;
> +  size_t mMapSize = 0;

Please document what this size is measuring.  Number of entries?  Something else?  Presumably not number of entries, since mEntries should know that itself?

::: dom/ipc/SharedMap.h:194
(Diff revision 3)
> +class WritableSharedMap final : public SharedMap
> +{
> +public:
> +  WritableSharedMap();
> +
> +  void Set(JSContext* cx, const nsACString& name, JS::HandleValue value, ErrorResult& aRv);

Document the charset for name?

::: dom/ipc/SharedMap.h:196
(Diff revision 3)
> +public:
> +  WritableSharedMap();
> +
> +  void Set(JSContext* cx, const nsACString& name, JS::HandleValue value, ErrorResult& aRv);
> +
> +  void Delete(const nsACString& name);

Document charset.

::: dom/ipc/SharedMap.h:211
(Diff revision 3)
> +
> +protected:
> +  ~WritableSharedMap() override = default;
> +
> +private:
> +  nsTArray<nsCString> mChangedKeys;

Document charset.

::: dom/ipc/SharedMap.h:219
(Diff revision 3)
> +
> +  Result<Ok, nsresult> Serialize();
> +
> +  void BroadcastChanges();
> +
> +  void KeyChanged(const nsACString& aName);

Document charset.

::: dom/ipc/SharedMap.cpp:30
(Diff revision 3)
> +// necessary, though.
> +constexpr size_t kStructuredCloneAlign = sizeof(uintptr_t);
> +
> +
> +static inline size_t
> +GetAlignmentOffset(size_t aOffset, size_t aAlign)

I wonder whether this would be more clearly named as GetAlignmentPadding.  In either case it's worth documenting that it's the amount of padding we need to add to aOffset to make sure that it becomes aligned to aAlign.

Actually, now that I look at the consumers, they all have this form:

  x += GetAlignmentOffset(x, something);
  
Given that, maybe the API should instead be called something like AlignTo and either take a mutable ref for the first arg or be called as:

  x = AlignTo(x, something);

with the addition happening inside AlignTo.  That would make it a bit clearer what's really being done.

::: dom/ipc/SharedMap.cpp:87
(Diff revision 3)
> +    // snapshot. Just decode it directly.
> +
> +    auto& holder = mData.ref<StructuredCloneData>();
> +    holder.Read(aCx, aRetVal, aRv);
> +    return;
> +  } else {

Else after return....

::: dom/ipc/SharedMap.cpp:94
(Diff revision 3)
> +    // clone data. Create a temporary buffer to decode that data, and then
> +    // discard it so that we don't keep a separate process-local copy around any
> +    // longer than necessary.
> +
> +    StructuredCloneData holder;
> +    if (holder.CopyExternalData(Data(), Size())) {

Worth filing a bug to enable creation of a "dependent" StructuredCloneData that doesn't need to copy as long as the data pointer outlives the StructuredCloneData?

::: dom/ipc/SharedMap.cpp:99
(Diff revision 3)
> +    if (holder.CopyExternalData(Data(), Size())) {
> +      holder.Read(aCx, aRetVal, aRv);
> +      return;
> +    }
> +  }
> +  aRv.Throw(NS_ERROR_OUT_OF_MEMORY);

I think it would be clear to have this over where the OOM happened, so reverse the sense of the CopyExternalData check and throw-and-return-early if that tests false.

::: dom/ipc/SharedMap.cpp:113
(Diff revision 3)
> +  return *mMapFile;
> +}
> +
> +void
> +SharedMap::Update(const FileDescriptor& aMapFile, size_t aMapSize,
> +                  nsTArray<nsCString>&& aChangedKeys)

aChangedKeys is unused.   I guess the next changeset starts using it?  Worth mentioning that in the commit message.

::: dom/ipc/SharedMap.cpp:130
(Diff revision 3)
> +}
> +
> +void
> +SharedMap::Entry::TakeData(StructuredCloneData&& aHolder)
> +{
> +  mData.destroyIfConstructed();

I believe mData is always constructed: you construct it in the Entry ctor and always construct it immediately after destroyIfConstructed() calls.  So you could just call destroy() here.

But that brings up a related question: Why is this a MaybeOneOf and not a Variant, if it's always present?

::: dom/ipc/SharedMap.cpp:137
(Diff revision 3)
> +
> +  mSize = Holder().Data().Size();
> +}
> +
> +void
> +SharedMap::Entry::ExtractData(char* aDestPtr, uint32_t aNewOffset)

Yeah, this function definitely needs documentation in the header about its preconditions (e.g. that there are Size() bytes available to write to at aDestPtr), the meaning of the arguments, etc.

::: dom/ipc/SharedMap.cpp:165
(Diff revision 3)
> +  }
> +
> +  MOZ_TRY(mMap.init(*mMapFile, mMapSize));
> +  mMapFile.reset();
> +
> +  // We should be able to pass this as an initializer list or an immediate

It's not clear to me what the "this" is here.  The Range<uint8_t>?  Something else?

::: dom/ipc/SharedMap.cpp:172
(Diff revision 3)
> +  // initializes everything to 0.
> +  Range<uint8_t> range(&mMap.get<uint8_t>()[0], mMap.size());
> +  InputBuffer buffer(range);
> +
> +  uint32_t count;
> +  buffer.codeUint32(count);

So I think this would be a lot easier to follow and check if there were a pointer from here and from Serialize() to what the serialization format looks like...  I assume this is in fact reversing what Serialize() does?

::: dom/ipc/SharedMap.cpp:176
(Diff revision 3)
> +  uint32_t count;
> +  buffer.codeUint32(count);
> +
> +  for (uint32_t i = 0; i < count; i++) {
> +    auto entry = MakeUnique<Entry>(*this);
> +    entry->Code(buffer);

So this reads the entry header from the buffer right?

What about reading the entry data?  I don't see where that happens. I assume it must happen somewhere and I'm just missing it...

::: dom/ipc/SharedMap.cpp:182
(Diff revision 3)
> +    mEntries.Put(entry->Name(), entry.get());
> +    Unused << entry.release();

Why not:

  mEntries.Put(entry->Name(), entry.release());
  
?  If there's a reason, please comment it.

::: dom/ipc/SharedMap.cpp:217
(Diff revision 3)
> +}
> +
> +Result<Ok, nsresult>
> +WritableSharedMap::Serialize()
> +{
> +  uint32_t count = mEntries.Count();

I think this function could benefit from a comment descibing the serialization format, either here or somewhere else with this pointing to it.  I _think_ it's (count, concatenation of headers, alignment padding, concatenation of data with alignment paddings in between).  But the way it's being output makes this a bit hard to figure out, so it would be good to make it clear and document what the various offset computations are about.

It might also help to have a nicer name for "offset".  Possibly by first having a headerSize thing or something that gets built up and then initializing a dataOffset from it and incrementing that as we serialize the entries.

::: dom/ipc/SharedMap.cpp:290
(Diff revision 3)
> +void
> +WritableSharedMap::Delete(const nsACString& aName)
> +{
> +  mEntries.Remove(aName);
> +
> +  KeyChanged(aName);

Maybe only call KeyChanged if Remove() returns true?
Attachment #8987315 - Flags: review?(bzbarsky) → review-
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review262982

> Does this actually need to be nsISupports.  It's probably fine to land like this but worth thinking about whether that can be removed.

The next patch changes it to DOMEventTargetHelper. I tried to keep the binding parts separate to make it easier for the non-DOM reviewers to follow.

> ==, not =, right?

Yeah, sorry, I fixed this locally as soon as I did a debug build. I didn't realize that was after my last review push.

> Worth filing a bug to enable creation of a "dependent" StructuredCloneData that doesn't need to copy as long as the data pointer outlives the StructuredCloneData?

Yeah, probably.

> aChangedKeys is unused.   I guess the next changeset starts using it?  Worth mentioning that in the commit message.

In the next changeset, yes. I thought I had mentioned it in the commit message...

> So I think this would be a lot easier to follow and check if there were a pointer from here and from Serialize() to what the serialization format looks like...  I assume this is in fact reversing what Serialize() does?

Yes

> So this reads the entry header from the buffer right?
> 
> What about reading the entry data?  I don't see where that happens. I assume it must happen somewhere and I'm just missing it...

The data is accessed directly from the SharedMap's mmap buffer when it's needed

> Why not:
> 
>   mEntries.Put(entry->Name(), entry.release());
>   
> ?  If there's a reason, please comment it.

Because order of evaluation of function arguments isn't guaranteed, and in some GCC builds, the entry.release() call ends up executing before entry->Name()
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review262982

> The data is accessed directly from the SharedMap's mmap buffer when it's needed

Ah, because we've deserialized offset.  Might be worth a comment.
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review263128

r=me.  Thank you for bearing with the terrible lag!

::: dom/ipc/SharedMap.h:110
(Diff revisions 3 - 4)
>      }
>  
>      ~Entry() = default;
>  
> +    /**
> +     * Encodes or decodes this entry into or from the given buffer.

Might be worth documenting that Buffer should be an InputBuffer or OutputBuffer?

::: dom/ipc/SharedMap.h:137
(Diff revisions 3 - 4)
>                sizeof(mSize));
>      }
>  
> +    /**
> +     * Updates the value of this entry to the given structured clone data, of
> +     * which it takes ownership. The passed StructuredCloneData object may not

"must not", maybe?

::: dom/ipc/SharedMap.h:168
(Diff revisions 3 - 4)
> +    // Decodes the entry's value into the current Realm of the given JS context
> +    // and puts the result in aRetVal on success.
>      void Read(JSContext* aCx, JS::MutableHandleValue aRetVal,
>                ErrorResult& aRv);
>  
> +    // Returns the byte size size of the entry's raw structured clone data.

s/size size/size/

::: dom/ipc/SharedMap.cpp:232
(Diff revisions 3 - 4)
> +  // new snapshot.
> +  //
> +  // The layout of the snapshot is as follows:
> +  //
> +  // - A header containing a uint32 count field containing the number of
> +  //   entries in the map, followed by that number of serialized entries, as

"serialized entry headers", not "serialized entries", right?  I guess it depends on whether you consider the entry data to be part of the entry...

::: dom/ipc/SharedMap.cpp:234
(Diff revisions 3 - 4)
> +  // The layout of the snapshot is as follows:
> +  //
> +  // - A header containing a uint32 count field containing the number of
> +  //   entries in the map, followed by that number of serialized entries, as
> +  //   produced by Entry::Code.
> +  //

Somewhere here should mention alignment padding right before every data block.

::: dom/ipc/SharedMap.cpp:271
(Diff revisions 3 - 4)
>    MOZ_TRY(mem.Init(offset + dataSize));
>  
>    auto ptr = mem.Get<char>();
>  
>    for (auto& entry : IterHash(mEntries)) {
> -    offset += GetAlignmentOffset(offset, kStructuredCloneAlign);
> +    AlignTo(&offset, kStructuredCloneAlign);

When we enter the loop, "offset" is already guaranteed to be aligned.  Would it make sense to do the aligning at the end of the loop block, right after we modify it?

I guess doing it the way you have it ensures that we align right before we use it...
Attachment #8987315 - Flags: review?(bzbarsky) → review+
Comment on attachment 8987316 [details]
Bug 1463587: Part 3 - Add bindings for SharedMap, and expose it via process message managers.

https://reviewboard.mozilla.org/r/252558/#review263156

::: dom/ipc/SharedMap.cpp:185
(Diff revisions 2 - 4)
> +             "wrapper");
> +  if (!wrapper) {
> +    return JS::NullValue();
> +  }
> +
> +  AutoJSContext cx;

This is fine for now; we should fix that followup to pass in the JSContext.
Attachment #8987316 - Flags: review?(bzbarsky) → review+
Comment on attachment 8987315 [details]
Bug 1463587: Part 2 - Add a shared-memory structured clone key-value store.

https://reviewboard.mozilla.org/r/252556/#review263128

> When we enter the loop, "offset" is already guaranteed to be aligned.  Would it make sense to do the aligning at the end of the loop block, right after we modify it?
> 
> I guess doing it the way you have it ensures that we align right before we use it...

We wind up making the same number of align calls either way, and I generally find it easier if the align call comes before the data being written, since that makes it clearer which data it applies to. So I think I prefer to keep the alignment at the start of the block.

Also, if we wind up adding more data blocks after the structured clone values later on, there's no point in applying the structured clone padding after the last entry if the next block's constraints are smaller. (So far, all of the other shared memory data stores I've done have string tables that only need 1 or 2 byte alignment, for instance.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64e1c150db2bfc8bbcfa737f845d86436aad488
Bug 1463587: Follow-up: Disable OOP test on Android. r=me,test-only DONTBUILD
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd2080bd727aec1f438df3fa480e77c0156fcef3
Bug 1463587: Follow-up: Clear mSharedData before CC shutdown in debug/asan builds. r=bustage CLOSED TREE
Blocks: 1475970
No longer blocks: 1475970
Depends on: 1475970
Depends on: 1477129
Depends on: 1477753
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: