Closed Bug 1264642 Opened 8 years ago Closed 8 years ago

Reduce the contiguous address space needed for StructuredClone serialization

Categories

(Core :: DOM: Content Processes, defect, P1)

Product:
Core
Component:
DOM: Content Processes
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
e10s + ---
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: mccr8, Assigned: kanru)

References

Details

(Whiteboard: btpp-active, e10st?)

Attachments

(6 files, 2 obsolete files)

Bug 1264642 - Part 1. Remove unused methods from IDBObjectStore::StructuredCloneWriteInfo.
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.
58 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.
58 bytes, text/x-review-board-request
jorendorff
: review+
billm
: review+
baku
: review+
Details
Bug 1264642 - Part 3. Add BufferList::MoveFallible.
58 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1264642 - Part 5. Make SerializedStructuredClone{Read,Write}Info use SerializedStructuredCloneBuffer.
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1264642 - Part 6. Mark JSStructuredCloneData as MOZ_NON_MEMMOVABLE and add specializations in nsTArray.h.
58 bytes, text/x-review-board-request
froydnj
: review+
Details 
PBrowser::Msg_AsyncMessage is the most common source of large IPC messages by far, exceeding the next most common, some HTTP message, by a factor of 5 or more. http://mzl.la/1ScqmcX I'd guess this is mostly ClonedMessageData.

I filed bug 1262661 and bug 1263004 about reducing the size of messages of a few prolific message manager users, and bugs like bug 1262671 and bug 1253131 about reducing the impact of large messages at the IPC layer, but I think it makes sense to also have a bug about trying to reduce the size of messages within the message manager itself.

Bug 1263028 is mitigating the large message issue by breaking the data into chunks of a hundred kb or so at a time. Maybe we could do something similar for these messages. Ideally we want to uplift this to 47.

(There's some discussion of this in bug 1263004 and bug 1262661.)
Kan-ru, are you interested in working on this? In bug 1262661 it sounded like you were interested in reducing the message size at the structured clone level.
Flags: needinfo?(kchen)
This shouldn't we for the PBrowser only, nor asyncmessage only, but about all message manager messaging, since then in DOM layer we anyhow deal the messages mostly the same way.

The trickiest part here is to change structured clone handling in js engine.
Assignee: nobody → kchen
Flags: needinfo?(kchen)
tracking-e10s: ? → +
Priority: -- → P1
Whiteboard: btpp-active
I checked SCOutput. It uses Vector<uint64_t> as backing buffer and always serialize other types as multiple uint64_t so should be easy to change to SegmentedVector. However the buffer is exposed via JSAutoStructuredCloneBuffer::data(), which is a JS_PUBLIC_API. 

Gecko only use the raw buffer in StructuredCloneHolderBase and IDBObjectStore. The first usage is only used to construct a SerializedStructuredCloneBuffer. The second usage could easily be avoided.

Jason, it seems you reviewed or even wrote this code. Do you think it's safe to unexpose the JSAutoStructuredCloneBuffer::data() API?
Flags: needinfo?(jorendorff)
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #4)
> Jason, it seems you reviewed or even wrote this code. Do you think it's safe
> to unexpose the JSAutoStructuredCloneBuffer::data() API?

Yes, that would be fine.
Flags: needinfo?(jorendorff)
Depends on: 1262671
Kan-Ru, are you going to fix the extra copy you mentioned in bug 1262661 comment 10 in this bug? If not, I can file a separate bug for it and write a patch.
Flags: needinfo?(kchen)
(In reply to Andrew McCreight [:mccr8] from comment #6)
> Kan-Ru, are you going to fix the extra copy you mentioned in bug 1262661
> comment 10 in this bug? If not, I can file a separate bug for it and write a
> patch.

These code will be changed to not use contiguous raw buffers but it's likely it will take me a few more days to there so feel free to change that first if you want.
Flags: needinfo?(kchen)
The WIP patch still looks ugly it works. It passed all js structured clone tests and tests_postMessages.html tests so I think the conversion worked quite well.

Remaining works are
1. Convert IDBObjectStore to use the new format
2. Some optimization
3. Maybe also use non-contiguous buffer in IPC level.
It is great to see these patches!

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #11)
> 3. Maybe also use non-contiguous buffer in IPC level.

Bill is already working on making the storage for IPC::Message non-contiguous in bug 1262671, if that is what you mean.
Hi Kan-Ru,
I posted my current patches in bug 1262671. They're not finished, but hopefully you get an idea of where I'm going.

I made a new class, BufferList, to store arbitrary sized buffers. It's similar to SegmentedVector except that the buffers don't need to be all the same size. I would like to use this class for both structured clone data as well as IPC messages. I would like us to be able to borrow the set of buffers storing structured clone data from an IPC message in SerializedStructuredCloneBuffer::Read (instead of just copying the data). This class should allow us to do that.

Is there a time when we can talk about this stuff?
(In reply to Bill McCloskey (:billm) from comment #15)
> Hi Kan-Ru,
> I posted my current patches in bug 1262671. They're not finished, but
> hopefully you get an idea of where I'm going.
> 
> I made a new class, BufferList, to store arbitrary sized buffers. It's
> similar to SegmentedVector except that the buffers don't need to be all the
> same size. I would like to use this class for both structured clone data as
> well as IPC messages. I would like us to be able to borrow the set of
> buffers storing structured clone data from an IPC message in
> SerializedStructuredCloneBuffer::Read (instead of just copying the data).
> This class should allow us to do that.
> 
> Is there a time when we can talk about this stuff?

BufferList looks good, it has the methods I wanted to add to SegmentedVector. However it's also lacking some methods before I can rebase my patches on top of it. Most important is the iterator needs to be writable.

I sent a invitation to you, we can chat over vidyo.
I think this new summary is more descriptive of the work being done. Feel free to change it more if you would like.
Summary: Reduce the size of PBrowser::Msg_AsyncMessage IPC messages → Reduce the contiguous address space needed for StructuredClone serialization
See Also: → 1274075
Blocks: 1274075
See Also: 1274075 → 
Latest patches https://treeherder.mozilla.org/#/jobs?repo=try&revision=46dba86d73d3
I'm cleaning up the patches for review.
nsTArray<T> assumes T is safely memmove()'able but Vector is not. Vector
used a pointer to its mBegin member variable both for indicating if it's
using inline storage and indexing its elements.

This patch modified Vector to use a boolean variable to indicating if
it's using inline storage and always selects the appropriate base
address based on that.

Review commit: https://reviewboard.mozilla.org/r/51603/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51603/
Attachment #8748548 - Attachment description: MozReview Request: Bug 1264642 - Part 1. Remove unused methods IDBObjectStore::StructuredCloneWriteInfo → Bug 1264642 - Part 1. Remove unused methods from IDBObjectStore::StructuredCloneWriteInfo.
Attachment #8748549 - Attachment description: MozReview Request: Bug 1264642 - Part 2. Add empty() method to JSAutoStructuredCloneBuffer and use it. → Bug 1264642 - Part 2. Add empty() method to JSAutoStructuredCloneBuffer and use it.
Attachment #8748550 - Attachment description: MozReview Request: Bug 1264642 - WIP - Part 3. Use SegmentedVector<uint8_t> to replace raw buffers. → Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.
Attachment #8760666 - Flags: review?(nfroyd)
Attachment #8760667 - Flags: review?(wmccloskey)
Attachment #8760668 - Flags: review?(jorendorff)
Attachment #8760669 - Flags: review?(amarchesini)
Attachment #8748548 - Flags: review?(amarchesini)
Attachment #8748549 - Flags: review?(jorendorff)
Attachment #8748550 - Flags: review?(wmccloskey)
Attachment #8748550 - Flags: review?(jorendorff)
Attachment #8748550 - Flags: review?(amarchesini)
Comment on attachment 8748548 [details]
Bug 1264642 - Part 1. Remove unused methods from IDBObjectStore::StructuredCloneWriteInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50393/diff/1-2/
Comment on attachment 8748549 [details]
Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50395/diff/1-2/
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50397/diff/1-2/
Attachment #8748548 - Flags: review?(amarchesini) → review+
Comment on attachment 8748548 [details]
Bug 1264642 - Part 1. Remove unused methods from IDBObjectStore::StructuredCloneWriteInfo.

https://reviewboard.mozilla.org/r/50393/#review55044
Attachment #8760666 - Flags: review?(nfroyd)
Comment on attachment 8760666 [details]
Bug 1264642 - Part 3. Add BufferList::MoveFallible.

https://reviewboard.mozilla.org/r/51603/#review55056

The right way to do this is to specialize `nsTArrayCopyChooser` for `Vector` types:

http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#669
(In reply to Nathan Froyd [:froydnj] from comment #30)
> Comment on attachment 8760666 [details]
> Bug 1264642 - Part 3. Make mfbt/Vector safe to use in nsTArray
> 
> https://reviewboard.mozilla.org/r/51603/#review55056
> 
> The right way to do this is to specialize `nsTArrayCopyChooser` for `Vector`
> types:
> 
> http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#669

But I don't want to store Vector directly in nsTArray. Any class that use Vector will need to specialize nsTArrayCopyChooser. It might be layers away and not obvious. I think fix Vector is safer.
Flags: needinfo?(nfroyd)
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #31)
> (In reply to Nathan Froyd [:froydnj] from comment #30)
> > Comment on attachment 8760666 [details]
> > Bug 1264642 - Part 3. Make mfbt/Vector safe to use in nsTArray
> > 
> > https://reviewboard.mozilla.org/r/51603/#review55056
> > 
> > The right way to do this is to specialize `nsTArrayCopyChooser` for `Vector`
> > types:
> > 
> > http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#669
> 
> But I don't want to store Vector directly in nsTArray.

This statement and the patch title are contradictory; the patch title and the description very much make it sound like you want nsTArray<Vector<T>> to work.  What code are you trying to make work correctly?

> Any class that use Vector will need to specialize nsTArrayCopyChooser.

I don't see why this is the case.  Why would they need to do that?
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #32)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #31)
> > (In reply to Nathan Froyd [:froydnj] from comment #30)
> > > Comment on attachment 8760666 [details]
> > > Bug 1264642 - Part 3. Make mfbt/Vector safe to use in nsTArray
> > > 
> > > https://reviewboard.mozilla.org/r/51603/#review55056
> > > 
> > > The right way to do this is to specialize `nsTArrayCopyChooser` for `Vector`
> > > types:
> > > 
> > > http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#669
> > 
> > But I don't want to store Vector directly in nsTArray.
> 
> This statement and the patch title are contradictory; the patch title and
> the description very much make it sound like you want nsTArray<Vector<T>> to
> work.  What code are you trying to make work correctly?
> 
> > Any class that use Vector will need to specialize nsTArrayCopyChooser.
> 
> I don't see why this is the case.  Why would they need to do that?

For example, mfbt/BufferList has a Vector memember variable. In my patch I make dom::StructuredCloneData use a BufferList member variable. And somewhere in our code stores StructuredCloneData in a nsTArray. No compile time warning but might crash seemly randomly depends on if nsTArray grows storage. Same if a class Foo used StructuredCloneData and placed in a nsTArray. So any class that has Vector as member variables in the lowest level needs to know that it's unsafe to use in nsTArray and add specialization to nsTArrayCopyChooser. Unless we can detect that at compile time, IMO it's a footgun.
Flags: needinfo?(nfroyd)
No longer blocks: 1274075
One could use MOZ_NON_MEMMOVABLE on a special subtype of Vector created solely for BufferList's purposes:

  class SegmentVector MOZ_NON_MEMMOVABLE : public Vector<Segment, 1, AllocPolicy>
  {
  };

which should make BufferList and any classes that have it as a member not storable in nsTArray.  (Ideally we could add MOZ_NON_MEMMOVABLE to Vector, but since a Vector *can* be memmovable, we have to do this.)

The Right Thing to do here is to flip the default for nsTArray and opt-in to the memcpy/memmove optimizations for at least RefPtr/nsCOMPtr/UniquePtr/integers/floats.
Flags: needinfo?(nfroyd)
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review55858

::: dom/base/nsStructuredCloneContainer.cpp:143
(Diff revision 2)
>    }
>  
> -  nsAutoCString binaryData(reinterpret_cast<char*>(Data()), DataLength());
> +  const char* data;
> +  auto iter = Data().Iter();
> +  size_t size = Data().Size();
> +  Data().FlattenBytes(iter, &data, size);

Don't we have to deallocate 'data' ?

::: dom/indexedDB/IDBObjectStore.cpp:1071
(Diff revision 2)
>  
> -  MOZ_ASSERT(!(dataLen % sizeof(*data)));
> +  MOZ_ASSERT(!(dataLen % sizeof(uint64_t)));
>  
>    JSAutoRequest ar(aCx);
>  
> +  JSStructuredCloneData buf(0, 4096, 4096);

What are these numbers? We must write a comment. Or we should add a typedef/helper/something to 'hide' them and centralize them in 1 single place. These numbers are everywhere in this patch.

::: dom/indexedDB/IDBObjectStore.cpp:1116
(Diff revision 2)
> -
> -  MOZ_ASSERT(!(dataLen % sizeof(*data)));
>  
>    JSAutoRequest ar(aCx);
>  
> +  JSStructuredCloneData buf(0, 4096, 4096);

same here.

::: dom/indexedDB/IDBObjectStore.cpp:1316
(Diff revision 2)
>  
> -  // XXX Remove this
> -  memcpy(cloneData.Elements(), cloneWriteInfo.mCloneBuffer.data(),
> -         cloneWriteInfo.mCloneBuffer.nbytes());
> +  const char* buf;
> +  auto iter = cloneWriteInfo.mCloneBuffer.data().Iter();
> +  cloneWriteInfo.mCloneBuffer.data().FlattenBytes(iter, &buf, size);
> +
> +  // FIXME Bug XXXXXX Change SerializedStructuredCloneReadInfo and

have you filed this bug? Write the ID here before landing the patch.

::: dom/ipc/StructuredCloneData.h:34
(Diff revision 2)
> -  }
>  
>    static already_AddRefed<SharedJSAllocatedData>
> -  AllocateForExternalData(size_t aDataLength)
> +  CreateFromExternalData(const char* aData, size_t aDataLength)
>    {
> -    uint64_t* data = Allocate64bitSafely(aDataLength);
> +    JSStructuredCloneData buf(0, 4096, 4096);

It seems that these numbers are the default.
What about if you can just do:

JSStructuredCloneData buf;

and the CTOR of this object sets these values by default?

::: dom/ipc/StructuredCloneData.h:73
(Diff revision 2)
>  public:
>    StructuredCloneData()
>      : StructuredCloneHolder(StructuredCloneHolder::CloningSupported,
>                              StructuredCloneHolder::TransferringSupported,
>                              StructuredCloneHolder::DifferentProcess)
> -    , mExternalData(nullptr)
> +    , mExternalData(0, 0, 0)

oh... here are 0s?
Add a comment.

::: dom/ipc/StructuredCloneData.cpp:113
(Diff revision 2)
>  
>  bool
>  StructuredCloneData::ReadIPCParams(const IPC::Message* aMsg,
>                                     PickleIterator* aIter)
>  {
> -  MOZ_ASSERT(!Data());
> +  // MOZ_ASSERT(!Data());

Why this MOZ_ASSERT is comment out? I'll check the other patches to see.

::: ipc/glue/IPCMessageUtils.h:88
(Diff revision 2)
> +  }
> +
>    bool
>    operator==(const SerializedStructuredCloneBuffer& aOther) const
>    {
> -    return this->data == aOther.data &&
> +    return false;

Tell me more about this.
Attachment #8748550 - Flags: review?(amarchesini) → review+
https://reviewboard.mozilla.org/r/50397/#review55858

> Don't we have to deallocate 'data' ?

No, we don't have to deallocate 'data' because it's a const pointer pointing to the Data() interal. The buffer is owned by Data().

> What are these numbers? We must write a comment. Or we should add a typedef/helper/something to 'hide' them and centralize them in 1 single place. These numbers are everywhere in this patch.

I added a default constructor in patch part 6. Sorry I didn't comment in code.

> same here.

same here.

> have you filed this bug? Write the ID here before landing the patch.

I fixed this in the patch part 7.

> It seems that these numbers are the default.
> What about if you can just do:
> 
> JSStructuredCloneData buf;
> 
> and the CTOR of this object sets these values by default?

Same here.

> oh... here are 0s?
> Add a comment.

Same here.

> Why this MOZ_ASSERT is comment out? I'll check the other patches to see.

Because the original MOZ_ASSERT was checking if we already have data in the buffer. I think in later patches I changed this to a boolean variable.

> Tell me more about this.

The copy assignment operator and the equality operator are needed by the IPDL generated code. We relied on the copy assignment operator at some places but we never use the equality operator. To implement this properly we need to do memcmp for all the buffers. I tend to put a NS_NOTREACHED here and simply return false since we aren't using it.
Whiteboard: btpp-active → btpp-active, e10st?
Attachment #8748549 - Flags: review?(jorendorff)
Comment on attachment 8748549 [details]
Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.

https://reviewboard.mozilla.org/r/50395/#review56914

Clearing the review flag for now.

::: js/public/StructuredClone.h:209
(Diff revision 2)
>  
>      ~JSAutoStructuredCloneBuffer() { clear(); }
>  
>      uint64_t* data() const { return data_; }
>      size_t nbytes() const { return nbytes_; }
> +    bool empty() const { return !data_ || !nbytes_; }

I don't think I understand the patch. If `nbytes_` is zero, then `data_` must be null, right?
I agree with froydnj on part 3. The footgun is inherent in nsTArray, not Vector.

I'm OK with marking Vector<> non-memmovable for all instances, just to be on the safe side. (Some instances are memmovable, but maybe we don't ever actually need to memmove them; or maybe we can fix that using a partial specialization of Vector.)
(In reply to Jason Orendorff [:jorendorff] from comment #38)
> I agree with froydnj on part 3. The footgun is inherent in nsTArray, not
> Vector.

FWIW, I have patches that fix the footgun in nsTArray and should be posting them later today or tomorrow.
Attachment #8748550 - Flags: review?(jorendorff) → review+
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review56916

Long review, but this is great work. Thank you. r=me with the comments below addressed.

::: js/public/StructuredClone.h:160
(Diff revision 2)
>                         JS::MutableHandleValue vp,
>                         const JSStructuredCloneCallbacks* optionalCallbacks, void* closure);
>  
>  /**
>   * Note: On success, the caller is responsible for calling
> - * JS_ClearStructuredClone(*datap, nbytes, optionalCallbacks, closure).
> + * JS_ClearStructuredClone(*datap, optionalCallbacks, closure).

The comment is inaccurate - the caller no longer has to do this. Please remove the comment.

::: js/public/StructuredClone.h:163
(Diff revision 2)
>  /**
>   * Note: On success, the caller is responsible for calling
> - * JS_ClearStructuredClone(*datap, nbytes, optionalCallbacks, closure).
> + * JS_ClearStructuredClone(*datap, optionalCallbacks, closure).
>   */
>  JS_PUBLIC_API(bool)
> -JS_WriteStructuredClone(JSContext* cx, JS::HandleValue v, uint64_t** datap, size_t* nbytesp,
> +JS_WriteStructuredClone(JSContext* cx, JS::HandleValue v, JSStructuredCloneData&& data,

This JS_WriteStructuredClone() signature uses a move reference, but then the implementation uses a plain reference. I think the plain reference is correct. (But I'm a little surprised it links and runs with this mismatch!)

::: js/public/StructuredClone.h:170
(Diff revision 2)
>                          void* closure, JS::HandleValue transferable);
>  
>  JS_PUBLIC_API(bool)
> -JS_ClearStructuredClone(uint64_t* data, size_t nbytes,
> +JS_ClearStructuredClone(JSStructuredCloneData& data,
>                          const JSStructuredCloneCallbacks* optionalCallbacks,
>                          void *closure, bool freeData = true);

Please remove this API `JS_ClearStructuredClone` if possible. The destructor now does this for us, and there's also a `Clear()` method on `JSStructuredCloneData`.

::: js/public/StructuredClone.h:230
(Diff revision 2)
>                 const JSStructuredCloneCallbacks* callbacks=nullptr, void* closure=nullptr);
>  
>      /**
>       * Release the buffer and transfer ownership to the caller. The caller is
>       * responsible for calling JS_ClearStructuredClone or feeding the memory
>       * back to JSAutoStructuredCloneBuffer::adopt.

Please remove the last sentence of the comment on `steal()`.

::: js/public/StructuredClone.h:232
(Diff revision 2)
>      /**
>       * Release the buffer and transfer ownership to the caller. The caller is
>       * responsible for calling JS_ClearStructuredClone or feeding the memory
>       * back to JSAutoStructuredCloneBuffer::adopt.
>       */
> -    void steal(uint64_t** datap, size_t* nbytesp, uint32_t* versionp=nullptr,
> +    void steal(JSStructuredCloneData& data, uint32_t* versionp=nullptr,

Style micro-nit: Please stick with the prevailing style of using pointers for all out-parameters here:  `JSStructuredCloneData* data`.

::: js/src/jsalloc.h:83
(Diff revision 2)
>              return nullptr;
>          return static_cast<T*>(onOutOfMemory(allocFunc, bytes, reallocPtr));
>      }
>  
>    public:
> -    MOZ_IMPLICIT TempAllocPolicy(JSContext* cx) : cx_((ContextFriendFields*) cx) {} // :(
> +    MOZ_IMPLICIT TempAllocPolicy(JSContext* cx = nullptr) : cx_((ContextFriendFields*) cx) {} // :(

Please revert this change.

Alloc policies are (among other things) a way of using the type system to communicate among engineers what error-handling protocol an object uses. When you see a Vector<..., TempAllocPolicy>, you know that vector's append() method reports OOM on the current thread's context.

Error-handling is easy to get wrong. Weakening the signals that we can send one another using types will make it that much easier. :(

I think all the APIs around structured cloning require a cx anyway, so it looks like this was just a convenience and can be reverted without too much trouble.

::: js/src/jsalloc.h:132
(Diff revision 2)
>      }
>  
>      JS_FRIEND_API(void) reportAllocOverflow() const;
>  
>      bool checkSimulatedOOM() const {
> -        if (js::oom::ShouldFailWithOOM()) {
> +        if (js::oom::ShouldFailWithOOM() && cx_) {

and here

::: js/src/jsalloc.cpp:18
(Diff revision 2)
>  void*
>  TempAllocPolicy::onOutOfMemory(AllocFunction allocFunc, size_t nbytes, void* reallocPtr)
>  {
> +    if (cx_)
> -    return static_cast<ExclusiveContext*>(cx_)->onOutOfMemory(allocFunc, nbytes, reallocPtr);
> +        return static_cast<ExclusiveContext*>(cx_)->onOutOfMemory(allocFunc, nbytes, reallocPtr);
> +    return nullptr;

and here (and below in reportAllocOverflow)

::: js/src/vm/StructuredClone.cpp:160
(Diff revision 2)
> +struct BufferIterator {
> +    struct End {};
> +    explicit BufferIterator(JSStructuredCloneData& aBuffer)
> +        : mBuffer(aBuffer)
> +        , mIter(aBuffer.Iter())
> +    {}

Style nits: SpiderMonkey style is a bit different from the rest of Gecko. `mBuffer` and `mIter` can stay, but please change `aBuffer` to plain `buffer` (and the same for other method arguments) and change the method names to lowercase. Thanks!

(We're interested in adopting a style that's more like Gecko, but not one class at a time!)

::: js/src/vm/StructuredClone.cpp
(Diff revision 2)
> -    : cx(cx), point(data), bufEnd(data + nbytes / 8)
> +    : cx(cx), point(data)
>  {
> -    // On 32-bit, we sometimes construct an SCInput from an SCOutput buffer,
> -    // which is not guaranteed to be 8-byte aligned
> -    MOZ_ASSERT((uintptr_t(data) & (sizeof(int) - 1)) == 0);
> -    MOZ_ASSERT((nbytes & 7) == 0);

The comment is no longer true, so thanks for deleting that; but both assertions can be retained:

    static_assert(JSStructuredCloneData::kSegmentAlignment % 8 == 0, "structured clone buffer reads should be aligned");
    MOZ_ASSERT(data.Size() % 8 == 0);

::: js/src/vm/StructuredClone.cpp:638
(Diff revision 2)
> +    if (!buffer) {
> +        ReportOutOfMemory(reinterpret_cast<ExclusiveContext*>(cx));
> +        return false;
> +    }
> +    if (!point.ReadBytes(buffer, size))
> +        return false;

Missing free here! But fortunately I don't think you need to malloc here anyway. You can `ReadBytes` directly into `p` and then use `swapFromLittleEndianInPlace` instead of `copyAndSwap`.

::: js/src/vm/StructuredClone.cpp:754
(Diff revision 2)
>      if (nelems + sizeof(uint64_t) / sizeof(T) - 1 < nelems) {
>          ReportAllocationOverflow(context());
>          return false;
>      }
>      size_t nwords = JS_HOWMANY(nelems, sizeof(uint64_t) / sizeof(T));
> -    size_t start = buf.length();
> +    uint64_t* buffer = static_cast<uint64_t*>(js_malloc(sizeof(uint64_t) * nwords));

I don't think the call to copyAndSwapToLittleEndian here would be correct on big-endian platforms: it's byteswapping 8-byte chunks, but it should be swapping T-sized chunks.

Dumber code is possible: maybe we should write

    for (size_t i = 0; i < nelems; i++) {
        T value = swapToLittleEndian(p[i]);
        if (!buf.WriteBytes(&value, sizeof(value)))
            return false;
    }

More bounds checks, but it eliminates a copy and the malloc() call. And it seems more obviously-correct. Your call.
(To be clear, I reviewed only the changes under /js.)
Attachment #8760668 - Flags: review?(jorendorff) → review+
Comment on attachment 8760668 [details]
Bug 1264642 - Part 6. Add default constructor for JSStructuredCloneData

https://reviewboard.mozilla.org/r/51607/#review56944

r=me with a fairly serious reservation: Comment 40 shot down the `TempAllocPolicy` default constructor, so you'll have to pass a `JSContext*` to the constructor at each call site. Apart from that, this is fine.
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review56982

I'm going to r- because ParamTraits<SerializedStructuredCloneBuffer>::Read is copying rather than borrowing. It's really important that we borrow there since these messages tend to be very large. There are a few other places where we can eliminate copies. Otherwise the patch looks good.

::: dom/base/nsStructuredCloneContainer.cpp:140
(Diff revision 2)
> -  nsAutoCString binaryData(reinterpret_cast<char*>(Data()), DataLength());
> +  const char* data;
> +  auto iter = Data().Iter();
> +  size_t size = Data().Size();
> +  Data().FlattenBytes(iter, &data, size);
> +  nsAutoCString binaryData(data, size);

I would really like to remove FlattenBytes, so I'd rather you not use it. Instead, you can just make an empty nsCString, call SetLength on it, and then use ReadBytesInto. Like this:
http://searchfox.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#353-355

::: dom/ipc/StructuredCloneData.cpp:103
(Diff revision 2)
>  {
> -  WriteParam(aMsg, DataLength());
> +  size_t length = DataLength() / sizeof(uint64_t);
> +  WriteParam(aMsg, length);
>  
> -  if (DataLength()) {
> -    aMsg->WriteBytes(Data(), DataLength());
> +  if (length) {
> +    for (auto iter = Data().Iter(); !iter.Done(); iter.Advance(Data(), sizeof(uint64_t))) {

It would be more efficient to copy whole segments at a time here using RemainingInSegment().

::: ipc/glue/IPCMessageUtils.h:749
(Diff revision 2)
> -      const_cast<const char**>(reinterpret_cast<char**>(&aResult->data));
> +          if (!aResult->data.WriteBytes(reinterpret_cast<char*>(&cur),
> -    // Structured clone data must be 64-bit aligned.
> -    if (!const_cast<Message*>(aMsg)->FlattenBytes(aIter, buffer, aResult->dataLength,
> -                                                  sizeof(uint64_t))) {
> +                                       sizeof(uint64_t))) {

This is going to copy. We need to borrow instead. I remember you mentioned a problem to me in London, but I forgot what it was. Was it related to this?

::: js/public/StructuredClone.h:163
(Diff revision 2)
>  /**
>   * Note: On success, the caller is responsible for calling
> - * JS_ClearStructuredClone(*datap, nbytes, optionalCallbacks, closure).
> + * JS_ClearStructuredClone(*datap, optionalCallbacks, closure).
>   */
>  JS_PUBLIC_API(bool)
> -JS_WriteStructuredClone(JSContext* cx, JS::HandleValue v, uint64_t** datap, size_t* nbytesp,
> +JS_WriteStructuredClone(JSContext* cx, JS::HandleValue v, JSStructuredCloneData&& data,

Also, since this is an outparam, I think it would be better to make it a pointer.

::: js/src/vm/StructuredClone.cpp:185
(Diff revision 2)
> +    bool ReadBytes(char* aOutData, size_t aSize) {
> +        return mBuffer.ReadBytes(mIter, aOutData, aSize);
> +    }
> +
> +    operator T*() const {
> +        return reinterpret_cast<T*>(mIter.Data());

This is not safe unless you can somehow guarantee that there are sizeof(T) bytes left in the current segment. You should at least assert that's true.

::: js/src/vm/StructuredClone.cpp:188
(Diff revision 2)
> +
> +    operator T*() const {
> +        return reinterpret_cast<T*>(mIter.Data());
> +    }
> +
> +    End end;

What's this for?

::: js/src/vm/StructuredClone.cpp:210
(Diff revision 2)
>      bool writePtr(const void*);
>  
>      template <class T>
>      bool writeArray(const T* p, size_t nbytes);
>  
> -    bool extractBuffer(uint64_t** datap, size_t* sizep);
> +    bool extractBuffer(JSStructuredCloneData& data);

Since this is an outparam, I think it should be a pointer.

::: js/src/vm/StructuredClone.cpp:331
(Diff revision 2)
>  
>      bool write(HandleValue v);
>  
>      SCOutput& output() { return out; }
>  
> -    bool extractBuffer(uint64_t** datap, size_t* sizep) {
> +    bool extractBuffer(JSStructuredCloneData& data) {

Same about using a pointer.

::: js/src/vm/StructuredClone.cpp:451
(Diff revision 2)
>                       const JSStructuredCloneCallbacks* cb, void* cbClosure)
>  {
> -    MOZ_ASSERT(nbytes % sizeof(uint64_t) == 0);
> -    uint64_t* end = buffer + nbytes / sizeof(uint64_t);
> +    auto point = BufferIterator<uint64_t>(buffer);
> +    auto end = point.end;
> -    uint64_t* point = buffer;
>      if (point == end)

I guess I see what you're using this for. Would it be that hard to just use .Done()?

::: js/src/vm/StructuredClone.cpp:631
(Diff revision 2)
> -    copyAndSwapFromLittleEndian(p, point, nelems);
> -    point += nwords;
> +    size_t size = sizeof(uint64_t) * nwords;
> +    char* buffer = static_cast<char*>(js_malloc(size));
> +    if (!buffer) {
> +        ReportOutOfMemory(reinterpret_cast<ExclusiveContext*>(cx));
> +        return false;
> +    }
> +    if (!point.ReadBytes(buffer, size))
> +        return false;
> +
> +    copyAndSwapFromLittleEndian(p, buffer, nelems);
> +    js_free(buffer);

This is pretty inefficient. You should read and copy the data in each segment separately using the iterator. That way you don't have to create |buffer|--you can just call copyAndSwapFromLittleEndian on the iterator's Data().

::: js/src/vm/StructuredClone.cpp:754
(Diff revision 2)
>      if (nelems + sizeof(uint64_t) / sizeof(T) - 1 < nelems) {
>          ReportAllocationOverflow(context());
>          return false;
>      }
>      size_t nwords = JS_HOWMANY(nelems, sizeof(uint64_t) / sizeof(T));
> -    size_t start = buf.length();
> +    uint64_t* buffer = static_cast<uint64_t*>(js_malloc(sizeof(uint64_t) * nwords));

I think the endian swapping here is probably correct since you're still passing the correct type T to copyAndSwapToLittleEndian. But I agree that we need to get rid of this copy, so I think we should use Jason's code.

::: js/src/vm/StructuredClone.cpp:2284
(Diff revision 2)
>  
>      return buf.read(cx, vp, callbacks, closure);
>  }
>  
>  JSAutoStructuredCloneBuffer::JSAutoStructuredCloneBuffer(JSAutoStructuredCloneBuffer&& other)
> +    : data_(0, 0, 0)

Does it work to set the standard capacity to 0? I don't see that being copied in during the assignment operator. Maybe we should do that.
Attachment #8748550 - Flags: review?(wmccloskey) → review-
Comment on attachment 8760667 [details]
Bug 1264642 - Part 5. Define ParamTraits<JSStructuredCloneData> and use it.

https://reviewboard.mozilla.org/r/51605/#review56992

r- because of the same borrowing issue as the other patch.
Attachment #8760667 - Flags: review?(wmccloskey) → review-
https://reviewboard.mozilla.org/r/50395/#review56914

> I don't think I understand the patch. If `nbytes_` is zero, then `data_` must be null, right?

It was added to help migrating away `data_`. I wasn't sure if the case where `data_` is allocated but `nbytes_` is zero is possible. In later patch this method only checks length.
Comment on attachment 8760669 [details]
Bug 1264642 - Part 5. Make SerializedStructuredClone{Read,Write}Info use SerializedStructuredCloneBuffer.

https://reviewboard.mozilla.org/r/58184/#review59702
Attachment #8760669 - Flags: review?(amarchesini) → review+
https://reviewboard.mozilla.org/r/50397/#review56916

> Please remove this API `JS_ClearStructuredClone` if possible. The destructor now does this for us, and there's also a `Clear()` method on `JSStructuredCloneData`.

I think I can remove JS_ClearStructuredClone since it's only used in TestingFunctions.cpp. However note that JSAutoStructuredCloneBuffer's destructor calls DiscardTransferables but JSStructuredCloneData's does not. Should we change JS_ClearStructuredClone to JS_DiscardTransferablesStructuredClone?

> Please remove the last sentence of the comment on `steal()`.

Same as JS_ClearStructuredClone. Feeding data back to a JSAutoStructuredCloneBuffer is needed to properly discard transerables. Do you think we should build that into JSStructuredCloneData's destructor? That means we have to make JSStructuredCloneData inherit from BufferList or embed a BufferList. I think JSAutoStructuredCloneBuffer is exactly that and JSStructuredCloneData is just a dumb type holding buffers.

> Please revert this change.
> 
> Alloc policies are (among other things) a way of using the type system to communicate among engineers what error-handling protocol an object uses. When you see a Vector<..., TempAllocPolicy>, you know that vector's append() method reports OOM on the current thread's context.
> 
> Error-handling is easy to get wrong. Weakening the signals that we can send one another using types will make it that much easier. :(
> 
> I think all the APIs around structured cloning require a cx anyway, so it looks like this was just a convenience and can be reverted without too much trouble.

This is needed because JSStructuredCloneData can be used outside of JS and without a cx. Actually it's harder than I thought to revert this change. I want to use the BufferList type in both JSAutoStructuredCloneBuffer and SCOutput but avoiding copy when moving data from SCOutput to JSAutoStructuredCloneBuffer. It doesn't feel right that a JSStructuredCloneData stealed from a JSAutoStructuredCloneBuffer can contain possibly invalid cx. That's why I think the cx should be nullable.
Flags: needinfo?(jorendorff)
Blocks: 1274075
I found a way to convert between two BufferList types.
Flags: needinfo?(jorendorff)
https://reviewboard.mozilla.org/r/50397/#review56916

> I think I can remove JS_ClearStructuredClone since it's only used in TestingFunctions.cpp. However note that JSAutoStructuredCloneBuffer's destructor calls DiscardTransferables but JSStructuredCloneData's does not. Should we change JS_ClearStructuredClone to JS_DiscardTransferablesStructuredClone?

I forgot that I already defined a new class for JSStructuredCloneData! Let's do this.
https://reviewboard.mozilla.org/r/50397/#review56982

> It would be more efficient to copy whole segments at a time here using RemainingInSegment().

Fixed in later patch.

> I guess I see what you're using this for. Would it be that hard to just use .Done()?

I try not to touch the original structured clone algorithm & style. I understand that using a End type is a bit over so I'll make it .Done() ;)
Comment on attachment 8748548 [details]
Bug 1264642 - Part 1. Remove unused methods from IDBObjectStore::StructuredCloneWriteInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50393/diff/2-3/
Attachment #8748549 - Attachment description: Bug 1264642 - Part 2. Add empty() method to JSAutoStructuredCloneBuffer and use it. → Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.
Attachment #8760666 - Attachment description: Bug 1264642 - Part 3. Make mfbt/Vector safe to use in nsTArray → Bug 1264642 - Part 3. Add BufferList::MoveFallible.
Attachment #8760669 - Attachment description: Bug 1264642 - Part 7. Make SerializedStructuredClone{Read,Write}Info use SerializedStructuredCloneBuffer → Bug 1264642 - Part 5. Make SerializedStructuredClone{Read,Write}Info use SerializedStructuredCloneBuffer.
Attachment #8748549 - Flags: review?(wmccloskey)
Attachment #8760666 - Flags: review?(wmccloskey)
Attachment #8748550 - Flags: review- → review?(wmccloskey)
Comment on attachment 8748549 [details]
Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50395/diff/2-3/
Comment on attachment 8760666 [details]
Bug 1264642 - Part 3. Add BufferList::MoveFallible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51603/diff/1-2/
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50397/diff/2-3/
Comment on attachment 8760669 [details]
Bug 1264642 - Part 5. Make SerializedStructuredClone{Read,Write}Info use SerializedStructuredCloneBuffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58184/diff/1-2/
Attachment #8760667 - Attachment is obsolete: true
Attachment #8760668 - Attachment is obsolete: true
review ping
Flags: needinfo?(wmccloskey)
Sorry, I was sick all last week and into this week. I started looking it over today but it will take some time. I should be able to finish tomorrow.
Flags: needinfo?(wmccloskey)
Comment on attachment 8760666 [details]
Bug 1264642 - Part 3. Add BufferList::MoveFallible.

https://reviewboard.mozilla.org/r/51603/#review66880

::: mfbt/BufferList.h:459
(Diff revision 2)
> +BufferList<OtherAllocPolicy>
> +BufferList<AllocPolicy>::MoveFallible(bool* aSuccess, OtherAllocPolicy aAP)
> +{
> +  BufferList<OtherAllocPolicy> result(0, 0, mStandardCapacity, aAP);
> +
> +  IterImpl iter =  Iter();

Two spaces before Iter()
Attachment #8760666 - Flags: review?(wmccloskey) → review+
Comment on attachment 8748549 [details]
Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.

https://reviewboard.mozilla.org/r/50395/#review66920

::: mfbt/BufferList.h:454
(Diff revision 3)
> +    size_t toMove = aIter.RemainingInSegment() - size;
> +    memmove(aIter.mData, aIter.mData + size, toMove);
> +    aIter.mDataEnd = aIter.mData + toMove;
> +    mSegments[aIter.mSegment].mSize -= size;
> +    *aSuccess = true;

This can violate the invariant that every buffer except the last one is full.

I'm wondering if we could simplify this function a lot by not removing all the data. We would only remove full segments. That makes it kind of a weird function, but we can just say that the contents of the buffer before aIter+aSize is left undefined.
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review66536

::: dom/indexedDB/IDBObjectStore.cpp:1071
(Diff revision 3)
> +  JSStructuredCloneData buf;
> +  if (!buf.WriteBytes(data, dataLen)) {
> +    return false;
> +  }

Hmm, the extra copying here seems like a problem. It would be nice if we could create a non-owning BufferList that just contained one buffer (data in this case). Maybe add an extra API function?

::: dom/indexedDB/IDBObjectStore.cpp:1312
(Diff revision 3)
> -  // XXX Remove this
> -  memcpy(cloneData.Elements(), cloneWriteInfo.mCloneBuffer.data(),
> -         cloneWriteInfo.mCloneBuffer.nbytes());
> +  const char* buf;
> +  auto iter = cloneWriteInfo.mCloneBuffer.data().Iter();
> +  cloneWriteInfo.mCloneBuffer.data().FlattenBytes(iter, &buf, size);
> +
> +  // FIXME Bug XXXXXX Change SerializedStructuredCloneReadInfo and
> +  // SerializedStructuredCloneWriteInfo to use JSStructuredCloneData
> +  // instead of raw buffer.
> +  memcpy(cloneData.Elements(), buf, size);

Why not just ReadBytes() from the buffer into cloneData.Element()? We do that elsewhere.

::: ipc/glue/IPCMessageUtils.h:72
(Diff revision 3)
> +    data.Clear();
> +    auto iter = aOther.data.Iter();
> +    while (!iter.Done()) {
> +      data.WriteBytes(iter.Data(), iter.RemainingInSegment());
> +      iter.Advance(aOther.data, iter.RemainingInSegment());
> +    }
> +    return *this;

Is this ever used? What for? We should avoid ways of accidentally introducing extra copies. Can we just delete this operator?

::: ipc/glue/IPCMessageUtils.h:742
(Diff revision 3)
> -      return true;
> +    if (length && !aMsg->ExtractBuffers(aIter, length, &buffers)) {
> +      return false;
> +    }

I don't understand why you're doing this here. This is what Borrow is supposed to be used for. There's no need to remove the data from the original message. Why not just leave it there and return a borrowed buffer?

::: js/public/StructuredClone.h:202
(Diff revision 3)
> +    using BufferList::BufferList;
> +};
> +
>  /** Note: if the *data contains transferable objects, it can be read only once. */
>  JS_PUBLIC_API(bool)
> -JS_ReadStructuredClone(JSContext* cx, uint64_t* data, size_t nbytes, uint32_t version,
> +JS_ReadStructuredClone(JSContext* cx, JSStructuredCloneData& data, uint32_t version,

Could data be const?

::: js/public/StructuredClone.h:222
(Diff revision 3)
>  /** RAII sugar for JS_WriteStructuredClone. */
>  class JS_PUBLIC_API(JSAutoStructuredCloneBuffer) {
> -    uint64_t* data_;
> +    JSStructuredCloneData data_;
> -    size_t nbytes_;
>      uint32_t version_;
> -    enum {
> +    OwnTransferablePolicy ownTransferables_;

Why do we need two of these (one here and one in data_)?

::: js/public/StructuredClone.h:224
(Diff revision 3)
>      const JSStructuredCloneCallbacks* callbacks_;
>      void* closure_;

Same here.

::: js/public/StructuredClone.h:251
(Diff revision 3)
> -    size_t nbytes() const { return nbytes_; }
> +    bool empty() const { return !data_.Size(); }
>  
>      void clear(const JSStructuredCloneCallbacks* optionalCallbacks=nullptr, void* closure=nullptr);
>  
>      /** Copy some memory. It will be automatically freed by the destructor. */
> -    bool copy(const uint64_t* data, size_t nbytes, uint32_t version=JS_STRUCTURED_CLONE_VERSION,
> +    bool copy(JSStructuredCloneData& data, uint32_t version=JS_STRUCTURED_CLONE_VERSION,

Can data be const?

::: js/src/vm/StructuredClone.cpp:200
(Diff revision 3)
> +            size_t offset = iter.RemainingInSegment();
> +            memcpy(iter.Data(), buf, offset);
> +            iter.Advance(mBuffer, offset);
> +            memcpy(iter.Data(), buf+offset, sizeof(T) - offset);

This isn't needed. BufferList segments are always a multiple of 8 bytes.

::: js/src/vm/StructuredClone.cpp:208
(Diff revision 3)
> +        if (mIter.HasRoomFor(sizeof(T))) {
> +            return *reinterpret_cast<T*>(mIter.Data());
> +        } else {
> +            typename BufferList::IterImpl iter = mIter;
> +            T result;
> +            mBuffer.ReadBytes(iter, reinterpret_cast<char*>(&result), sizeof(T));
> +            return result;
> +        }

Same here. This can be simplified because of the 8 byte segment alignment.
Attachment #8748550 - Flags: review?(wmccloskey) → review+
Comment on attachment 8748549 [details]
Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.

https://reviewboard.mozilla.org/r/50395/#review66920

> This can violate the invariant that every buffer except the last one is full.
> 
> I'm wondering if we could simplify this function a lot by not removing all the data. We would only remove full segments. That makes it kind of a weird function, but we can just say that the contents of the buffer before aIter+aSize is left undefined.

You mean we change segment.mData? Yes it would preserve the invariant. We will need to remember the original mData in order to free it properly.
Comment on attachment 8748549 [details]
Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.

https://reviewboard.mozilla.org/r/50395/#review66920

> You mean we change segment.mData? Yes it would preserve the invariant. We will need to remember the original mData in order to free it properly.

On a second thought. This won't work because the range we want to extract could be in the middle of the buffer. We can't leave a hole in the buffer.. unless we split the segment into two segments. What do you think, Bill?
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review66536

> Why not just ReadBytes() from the buffer into cloneData.Element()? We do that elsewhere.

Sorry I didn't update the comment. This is fixed in Part 5. where we use move and don't need to copy at all.

> Is this ever used? What for? We should avoid ways of accidentally introducing extra copies. Can we just delete this operator?

I believe it's not used. It's required by the IPDL generated code so I can't delete it. I can add a NS_WARNING here to avoid abuse.

> I don't understand why you're doing this here. This is what Borrow is supposed to be used for. There's no need to remove the data from the original message. Why not just leave it there and return a borrowed buffer?

I write it in the ExtractBuffer comment, I should have also put it here:

Borrowing is not suitable to use for IPC to hand out data because we often want to store the data somewhere for processing after IPC has released the underlying buffers. One case is PContentChild::SendGetXPCOMProcessAttributes. We can't return a borrowed buffer because the out param outlives the IPDL callback.

> Could data be const?

No, because JS_ReadStructuredClone modifies data in JSStructuredCloneReader::readTransferMap()

> Why do we need two of these (one here and one in data_)?

I think I can reduce it to only one in data_.

> Can data be const?

Yes, I think I can make it const.

> This isn't needed. BufferList segments are always a multiple of 8 bytes.

I'm afraid we lost this attribute after I extract buffers from IPDL BufferList.
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review66536

> Hmm, the extra copying here seems like a problem. It would be nice if we could create a non-owning BufferList that just contained one buffer (data in this case). Maybe add an extra API function?

Also fixed in Part 5.
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #61)
> Comment on attachment 8748549 [details]
> Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.
> 
> https://reviewboard.mozilla.org/r/50395/#review66920
> 
> > This can violate the invariant that every buffer except the last one is full.
> > 
> > I'm wondering if we could simplify this function a lot by not removing all the data. We would only remove full segments. That makes it kind of a weird function, but we can just say that the contents of the buffer before aIter+aSize is left undefined.
> 
> You mean we change segment.mData? Yes it would preserve the invariant. We
> will need to remember the original mData in order to free it properly.

No, what I mean is this: say you have a BufferList with three segments, arranged like this:

[123] [4567] [89]

Let's say that aIter points to 2 and you want to extract 7 bytes. With your current version, we would end up with this BufferList after the extraction:

[1] [9]
     ^

I'm proposing that instead we end up with this:

[123] [89]
        ^

So we would remove the segments that are being transferred to the other BufferList, but we would leave along the segments that we copy from. This way, the rules about alignment will be preserved and the code will be a lot simpler.

> > This isn't needed. BufferList segments are always a multiple of 8 bytes.
> 
> I'm afraid we lost this attribute after I extract buffers from IPDL BufferList.

I'm pretty sure there's code in pickle.cc that depends on this (at least on ARM). We can't break it.
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review66536

> I'm afraid we lost this attribute after I extract buffers from IPDL BufferList.

I fixed this by forcing StructuredClone segments to be always 8 bytes aligned. We did that in the origin code but I removed it accidiently.
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review66536

> I believe it's not used. It's required by the IPDL generated code so I can't delete it. I can add a NS_WARNING here to avoid abuse.

Copy is used in BroadcastChannel and MessageManagers to send messages to multiple endpoints. I think in some cases we could eliminate the copy by combine shared structured clone data and per channel data. I'll create a bug to track this.
(In reply to Bill McCloskey (:billm) from comment #65)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #61)
> > Comment on attachment 8748549 [details]
> > Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.
> > 
> > https://reviewboard.mozilla.org/r/50395/#review66920
> > 
> > > This can violate the invariant that every buffer except the last one is full.
> > > 
> > > I'm wondering if we could simplify this function a lot by not removing all the data. We would only remove full segments. That makes it kind of a weird function, but we can just say that the contents of the buffer before aIter+aSize is left undefined.
> > 
> > You mean we change segment.mData? Yes it would preserve the invariant. We
> > will need to remember the original mData in order to free it properly.
> 
> No, what I mean is this: say you have a BufferList with three segments,
> arranged like this:
> 
> [123] [4567] [89]
> 
> Let's say that aIter points to 2 and you want to extract 7 bytes. With your
> current version, we would end up with this BufferList after the extraction:
> 
> [1] [9]
>      ^
> 
> I'm proposing that instead we end up with this:
> 
> [123] [89]
>         ^
> 
> So we would remove the segments that are being transferred to the other
> BufferList, but we would leave along the segments that we copy from. This
> way, the rules about alignment will be preserved and the code will be a lot
> simpler.

Thanks for the explanation. Now I understand what you mean by left contents of the buffer before aIter+aSize undefined. It seems OK for our purpose since we will never re-read the buffer in IPDL.
Comment on attachment 8780632 [details]
Bug 1264642 - Part 6. Mark JSStructuredCloneData as MOZ_NON_MEMMOVABLE and add specializations in nsTArray.h.

https://reviewboard.mozilla.org/r/71174/#review68800

Looks good.  Would love to hear a positive answer to the question below.

::: xpcom/glue/nsTArray.h:719
(Diff revision 1)
> -  typedef nsTArray_CopyWithConstructors<nsRegion> Type;
> -};
> -
> -template<>
> -struct nsTArray_CopyChooser<nsIntRegion>
> -{
> -  typedef nsTArray_CopyWithConstructors<nsIntRegion> Type;
> -};
> +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::SerializedStructuredCloneBuffer)
> +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::ipc::StructuredCloneData)
> +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::ClonedMessageData)
> +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::indexedDB::StructuredCloneReadInfo);
> +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::indexedDB::ObjectStoreCursorResponse)
> +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::indexedDB::SerializedStructuredCloneReadInfo);
> +DECLARE_USE_COPY_CONSTRUCTORS(JSStructuredCloneData)
> +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::MessagePortMessage)

Were all of these were added because of the `MOZ_NON_MEMMOVABLE` annotation added to `JSStructuredCloneData`?  If so, that would be most excellent!
Attachment #8780632 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #75)
> Comment on attachment 8780632 [details]
> Bug 1264642 - Part 6. Mark JSStructuredCloneData as MOZ_NON_MEMMOVABLE and
> add specializations in nsTArray.h.
> 
> https://reviewboard.mozilla.org/r/71174/#review68800
> 
> Looks good.  Would love to hear a positive answer to the question below.
> 
> ::: xpcom/glue/nsTArray.h:719
> (Diff revision 1)
> > -  typedef nsTArray_CopyWithConstructors<nsRegion> Type;
> > -};
> > -
> > -template<>
> > -struct nsTArray_CopyChooser<nsIntRegion>
> > -{
> > -  typedef nsTArray_CopyWithConstructors<nsIntRegion> Type;
> > -};
> > +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::SerializedStructuredCloneBuffer)
> > +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::ipc::StructuredCloneData)
> > +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::ClonedMessageData)
> > +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::indexedDB::StructuredCloneReadInfo);
> > +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::indexedDB::ObjectStoreCursorResponse)
> > +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::indexedDB::SerializedStructuredCloneReadInfo);
> > +DECLARE_USE_COPY_CONSTRUCTORS(JSStructuredCloneData)
> > +DECLARE_USE_COPY_CONSTRUCTORS(mozilla::dom::MessagePortMessage)
> 
> Were all of these were added because of the `MOZ_NON_MEMMOVABLE` annotation
> added to `JSStructuredCloneData`?  If so, that would be most excellent!

Yes! :)
Comment on attachment 8748549 [details]
Bug 1264642 - Part 2. Add BufferList::Extract and Pickle::ExtractBuffers.

https://reviewboard.mozilla.org/r/50395/#review69220

::: mfbt/BufferList.h:265
(Diff revision 4)
>  
> +  // Return a new BufferList that adopts the byte range starting at Iter so that
> +  // range [aIter, aIter + aSize) is transplanted to the returned BufferList.
> +  // Contents of the buffer before aIter + aSize is left undefined.
> +  // Extract can fail, in which case *aSuccess will be false upon return. The
> +  // moved buffers is erased from the original BufferList. In case of extract

moved buffers *are* erased

::: mfbt/BufferList.h:266
(Diff revision 4)
> +  // fails, the original BufferList is intact.  All other iterator except aIter
> +  // is invalidated.

All other *iterators* except aIter *are* invalidated.
Attachment #8748549 - Flags: review?(wmccloskey) → review+
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/162098402acc
Part 1. Remove unused methods from IDBObjectStore::StructuredCloneWriteInfo. r=baku
https://hg.mozilla.org/integration/autoland/rev/06fc278fcedf
Part 2. Add BufferList::Extract and Pickle::ExtractBuffers. r=billm
https://hg.mozilla.org/integration/autoland/rev/9f434697ef2e
Part 3. Add BufferList::MoveFallible. r=billm
https://hg.mozilla.org/integration/autoland/rev/7c60fc4144fb
Part 4. Use BufferList to replace raw buffers in StructuredClone. r=baku,billm,jorendorff
https://hg.mozilla.org/integration/autoland/rev/078e5c447f21
Part 5. Make SerializedStructuredClone{Read,Write}Info use SerializedStructuredCloneBuffer. r=baku
https://hg.mozilla.org/integration/autoland/rev/f0067001c059
Part 6. Mark JSStructuredCloneData as MOZ_NON_MEMMOVABLE and add specializations in nsTArray.h. r=froydnj
Since this has been landed we have a new nearly perma orange Marionette test failure. Maybe that is related? I filed it as bug 1296003.
Comment on attachment 8748550 [details]
Bug 1264642 - Part 4. Use BufferList to replace raw buffers in StructuredClone.

https://reviewboard.mozilla.org/r/50397/#review71016

::: dom/messagechannel/SharedMessagePortMessage.cpp:72
(Diff revision 8)
>  
>    for (auto& message : aArray) {
>      RefPtr<SharedMessagePortMessage> data = new SharedMessagePortMessage();
>  
> -    data->mData.SwapElements(message.data());
> +    data->mBuffer = MakeUnique<JSAutoStructuredCloneBuffer>(
> +      JS::StructuredCloneScope::SameProcessSameThread, nullptr, nullptr);

You are right, it doesn't change too much, but better to write DifferentProcess for consistency.

In theory we could have different de/serialization based on the scope.

::: dom/messagechannel/SharedMessagePortMessage.cpp:150
(Diff revision 8)
>  
>    for (auto& message : aArray) {
>      RefPtr<SharedMessagePortMessage> data = new SharedMessagePortMessage();
>  
> -    data->mData.SwapElements(message.data());
> +    data->mBuffer = MakeUnique<JSAutoStructuredCloneBuffer>(
> +      JS::StructuredCloneScope::SameProcessSameThread, nullptr, nullptr);

same here.
Attachment #8748550 - Flags: review+
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed06c7a26d4
Part 1. Remove unused methods from IDBObjectStore::StructuredCloneWriteInfo. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9ad46a04ee
Part 2. Add BufferList::Extract and Pickle::ExtractBuffers. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bd09e50989
Part 3. Add BufferList::MoveFallible. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c07eaec94c4
Part 4. Use BufferList to replace raw buffers in StructuredClone. r=baku r=billm r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/84cc65140114
Part 5. Make SerializedStructuredClone{Read,Write}Info use SerializedStructuredCloneBuffer. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f16afa01319
Part 6. Mark JSStructuredCloneData as MOZ_NON_MEMMOVABLE and add specializations in nsTArray.h. r=froydnj
Flags: needinfo?(kchen)
Looks like this should stay with 51 but let us know if you are thinking of uplift to 50. Too late for 49 for sure.
status-firefox49: affected → wontfix
status-firefox50: affected → fix-optional
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #110)
> Looks like this should stay with 51 but let us know if you are thinking of
> uplift to 50. Too late for 49 for sure.
Flags: needinfo?(kchen)
IMO the changes are too massive so not qualified for uplift.
Flags: needinfo?(kchen)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: