Multiprocess blob support

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: overholt, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(1 attachment, 11 obsolete attachments)

221.30 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

5 years ago
Depends on: 726593

Updated

5 years ago
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Summary: IndexedDB: multiprocess blob support → Multiprocess blob and file handle support
This should block b2g v1 because prevents some core gaia apps from running out-of-process, that we absolutely need to have OOP.  At the least: gallery, music, video.
Blocks: 745143
blocking-basecamp: --- → ?

Comment 2

5 years ago
Ben, are we allowed to use sync messages for some specific operations ?
For example, when we need to get a new file info (or just id) during serialization of objects (which contain blobs) in a child process.

Comment 3

5 years ago
You can do sync from child to parent. You cannot/may not do sync from parent to child.
Yeah, what bsmedberg said. Of course, if we can figure out a way to do all things async that might be better, but not a strict requirement.
Blocking internal app development for B2G V1 per Jonas -> blocking+.
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
Blocks: 761930

Comment 6

5 years ago
Talked to Ben, he has time to do this.
I'll resume work on unified quota stuff since we need both pretty soon.
Per comment #6.
Assignee: Jan.Varga → bent.mozilla
(Reporter)

Updated

5 years ago
Blocks: 766203
(Reporter)

Comment 8

5 years ago
Splitting blob and file handle support into separate bugs.  File handle is bug 771288.
Summary: Multiprocess blob and file handle support → Multiprocess blob support
Blocks: 772167
No longer blocks: 772167
Also blocks MMS DOM API
Blocks: 760065

Updated

5 years ago
Blocks: 773798
Created attachment 643036 [details] [diff] [review]
Patch, v1

This patch adds blob/file support to the message manager, and should unblock most gaia apps (I hope!).

It has a basic test but there may be some cases I've missed.
Attachment #643036 - Flags: review?(jonas)
Attachment #643036 - Flags: review?(bugs)
Comment on attachment 643036 [details] [diff] [review]
Patch, v1

cjones, the pickle changes we discussed are in here, can you review those parts? (Or maybe even the whole thing?)
Attachment #643036 - Flags: review?(jones.chris.g)
Created attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1

I fixed the pickle stuff so that it asserts our marker now.
Attachment #643036 - Attachment is obsolete: true
Attachment #643036 - Flags: review?(jones.chris.g)
Attachment #643036 - Flags: review?(jonas)
Attachment #643036 - Flags: review?(bugs)
Attachment #643061 - Flags: review?(jones.chris.g)
Attachment #643061 - Flags: review?(jonas)
Attachment #643061 - Flags: review?(bugs)
Comment on attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1

Could you explain the patch a bit.
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIFrameMessageManager.idl#13 certainly doesn't look valid after the patch.
I mean the comment in http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIFrameMessageManager.idl#16
Well, the comment is already a little crazy, but in so far as it is correct today it will still be correct with this patch :)

> * This is for JS only.

Still true.

> * receiveMessage is called with one parameter, which has the following
> * properties:

Still true.

> *     name:    %message name%,
> *     sync:    %true or false%.

These are unchanged.

> *     json:    %json object or null%,

This was never a JSON object, it's always been just a string. For cloned messages it's now a clone of whatever was sent (so, could be a string, could be an object, etc.).

> *     objects: %array of handles or null, always null if sync is false%

As it says below this has never been implemented and I didn't change it.

> * if the message is synchronous, possible return value is sent back
> * as JSON.

I only added an async message, so this doesn't apply.

> * When the listener is called, 'this' value is the target of the message.

Unchanged.
Comment on attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1


>+[scriptable, builtinclass, uuid(7bf4e325-0fd1-45f9-b973-f64f037072b6)]
> interface nsIFrameMessageManager : nsISupports
> {
>   void addMessageListener(in AString aMessage, in nsIFrameMessageListener aListener);
>   void removeMessageListener(in AString aMessage, in nsIFrameMessageListener aListener);
>+
>   [implicit_jscontext,optional_argc]
>   void sendAsyncMessage([optional] in AString messageName, [optional] in jsval obj);
>+
>+  [implicit_jscontext,optional_argc]
>+  void sendAsyncClonedMessage([optional] in AString messageName,
>+                              [optional] in jsval obj);
>+
What is the reason for this new method?
I was told that structured cloning is a superset of json, so
you could just change the implementation of sendAsyncMessage
(and sendSyncMessage)
Comment on attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1

The correctness of this implementation depends on the minimum
alignment provided by the malloc impl.  It must be >= the
requested write/read alignment.

The argument is, given

 WriteAligned(char* buffer, const char* src, size_t nbytes, size_t alignment):
   (1) assert buffer % alignment == 0

   char* dest = reserveSpace(buffer, nbytes, alignment)
   (2) assert buffer % alignment == 0

   dest += alignment - dest % alignment
   (3) assert dest % alignment == 0

   memcpy(dest, src, nbytes)


 ReadAligned(const char* buffer, char** iter, char** dest, size_t nbytes, size_t alignment):
   (4) assert buffer % alignment == 0

   *dest = *iter
   *iter += computeSpace(buffer, nbytes, alignment)

   *dest += alignment - *dest % alignment
   (5) assert *dest % alignment == 0


We're given (1) by the assumption of the memory allocator.  We're
also given (2) by that --- reserving space may realloc, but we
still must have the same min alignment.

(3) is the postcondition of WriteAligned().  It follows from (1)
and (2) and the skip-bytes math.

We're given (4) by the allocator property.  The amount of space
we skip in read is the same amount we reserve in WriteAligned().
Therefore (5) follows from that and (4).

Our allocators provide a minimum alignment of 8 bytes (for
allocations > 4 bytes, which we always use here).  So we can only
support alignments of 4 or 8 bytes.  The rest of the pickle code
depends on 4-byte alignment so we can only support 4 or 8.

>diff --git a/ipc/chromium/src/base/pickle.cc b/ipc/chromium/src/base/pickle.cc

>-bool Pickle::ReadBytes(void** iter, const char** data, int length) const {
>+bool Pickle::ReadBytes(void** iter, const char** data, int length,
>+                       uint32 alignment) const {
>   DCHECK(iter);
>   DCHECK(data);
>+  DCHECK(alignment % 4 == 0) << "Must be at least 32-bit aligned!";
>+

Check that we're working within the limits of our allocator,

DCHECK(alignment == 4 || alignment == 8);

and property (4)

DCHECK(header_ % alignment == 0)

to satisfy the allocator-alignment property.

>   if (!*iter)
>     *iter = const_cast<char*>(payload());
> 
>+  uint32 offset = intptr_t(*iter) % alignment;

|padding| or |paddingLen| is probably a better name for this
 right?

>-  *data = reinterpret_cast<const char*>(*iter);
>+  *data = static_cast<const char*>(*iter) + offset;
> 

Property (5)

DCHECK(*data % alignment == 0)

>-char* Pickle::BeginWrite(uint32 length) {
>-  // write at a uint32-aligned offset from the beginning of the header
>+char* Pickle::BeginWrite(uint32 length, uint32 alignment) {
>+  DCHECK(alignment % 4 == 0) << "Must be at least 32-bit aligned!";
>+

Property (1) above:

  DCHECK(alignment == 4 || alignment == 8);
  DCHECK(header_ % alignment == 0)


>+  // write at an alignment-aligned offset from the beginning of the header
>   uint32 offset = AlignInt(header_->payload_size, sizeof(uint32));
>-  uint32 new_size = offset + AlignInt(length, sizeof(uint32));
>+  uint32 padding = (header_size_ + offset) %  alignment;
>+  uint32 new_size = offset + padding + AlignInt(length, sizeof(uint32));

This code crosses my eyes a little bit, but if you can prove it
follows the proof above, r=me.

>   if (needed_size > capacity_ && !Resize(std::max(capacity_ * 2, needed_size)))

Property (2) above: 

  DCHECK(header_ % alignment == 0)

>+  if (padding) {
>+    memset(buffer, kBytePaddingMarker, padding);
>+    buffer += padding;
>+  }
>+

Property (3) above, postcondition

  DCHECK(buffer % alignment == 0)

r=me with double-checking the padding computation in Write() and adding assertions.  Making a comment out of the proof might be worthwhile too.
Attachment #643061 - Flags: review?(jones.chris.g) → review+
Comment on attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1

I was trying to compile the patch so that I could have changed the patch
but after fixing failed hunks and other stuff there were still too
many compilation issues... so, a normal review instead.

You should effectively be able to replace the old sendAsyncMessage implementation with
your sendAsyncClonedMessage implementation, but keep the old method name.
Also, supporting sync messaging from child to parent should work the same way.
Currently all the JSObject->serialized_JSON conversion happens in 
nsFrameMessageManager::GetParamsForMessage, so I assume you could have something similar.
It would work for async and sync.


>+++ b/content/base/public/nsIFrameMessageManager.idl
>@@ -3,16 +3,17 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "nsISupports.idl"
> 
> interface nsIDOMWindow;
> interface nsIDocShell;
> interface nsIContent;
>+interface nsIDOMBlob;
Not needed.

> 
> [scriptable, function, uuid(938fcb95-3d63-46be-aa72-94d08fd3b418)]
> interface nsIFrameMessageListener : nsISupports
> {
>   /**
>    * This is for JS only.
>    * receiveMessage is called with one parameter, which has the following
>    * properties:
Update this comment to say that .json is actually structured clone.
(I know, a bit silly, but perhaps too late to change that API)


>+  [implicit_jscontext,optional_argc]
>+  void sendAsyncClonedMessage([optional] in AString messageName,
>+                              [optional] in jsval obj);
Don't add this but reuse the existing method.


The code itself looks ok. If you don't have time to merge sendAsyncClonedMessage implementation to sendAsyncMessage implementation, I could do it,
if there was a patch which compiles ;)
Attachment #643061 - Flags: review?(bugs) → review-
For the record, unless doing sync stuff is trivial, I don't think we should block on it here.
It should be trivial.
Blocks: 774966
Attachment #643061 - Attachment description: Patch, v1.1 → MessageManager patch, v1.1
Created attachment 643824 [details] [diff] [review]
IndexedDB patch (WIP)

This is an implementation for IndexedDB that uses the new PBlob. It only works for object stores at this point, but I figure it's close enough to the full patch to get some feedback. Add/Put/Get/GetAll/OpenCursor are all supported.

Note that this patch doesn't do anything to "share" blobs yet, it copies things unnecessarily. We'll need to do that in a followup.
Attachment #643824 - Flags: feedback?(jonas)
Attachment #643824 - Flags: feedback?(Jan.Varga)

Comment 22

5 years ago
looking at the patch ...

Comment 23

5 years ago
Comment on attachment 643824 [details] [diff] [review]
IndexedDB patch (WIP)

Ben, I'm still looking at the patch, I found this so far ...

>--- a/dom/indexedDB/IDBObjectStore.cpp
>+++ b/dom/indexedDB/IDBObjectStore.cpp
>+nsresult
>+ConvertBlobActors(ContentParent* aContentParent,
>+                  FileManager* aFileManager,
>+                  const nsTArray<StructuredCloneFile>& aFiles,
>+                  InfallibleTArray<PBlobParent*>& aActors)
>+{
>+  NS_ASSERTION(aContentParent, "Null contentParent!");
>+  NS_ASSERTION(aFileManager, "Null file manager!");
>+
>+  if (!aFiles.IsEmpty()) {
>+    PRUint32 fileCount = aFiles.Length();
>+    aActors.SetCapacity(fileCount);
>+
>+    for (PRUint32 index = 0; index < fileCount; index++) {
>+      const StructuredCloneFile& file = aFiles[index];
>+      NS_ASSERTION(file.mFileInfo, "This should never be null!");
>+
>+      nsCOMPtr<nsIFile> directory = aFileManager->GetDirectory();
>+      if (!directory) {
>+        NS_WARNING("Failed to get directory!");
>+        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>+      }

GetDirectory() call can be moved outside of the loop

>+
>+      nsCOMPtr<nsIFile> nativeFile =
>+        aFileManager->GetFileForId(directory, file.mFileInfo->Id());
>+      if (!nativeFile) {
>+        NS_WARNING("Failed to get file!");
>+        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>+      }
>+
>+      nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(nativeFile);

I guess this is just a temp solution. The blob shouldn't be created this way.
Currently, GetOrCreateActorForBlob() will call blob->GetType(), blob->GetName(), size, etc.
and since the blob was initialized using only a native file, it will try to get this info form the os file
So, two problems:
1. main thread I/O, if I'm reading the code correctly, this is called from GetHelper::MaybeSendResponseToChildProcess()
2. file name will be incorrect, content type might be incorrect too, only file size will be ok

>+
>+      nsRefPtr<BlobParent> actor =
>+        aContentParent->GetOrCreateActorForBlob(blob);
>+      NS_ASSERTION(actor, "This should never fail without aborting!");
>+
>+      aActors.AppendElement(actor);
>+    }
>+  }
>+
>+  return NS_OK;
>+}
>+

Comment 24

5 years ago
Well, I think I already mentioned this to Jonas some time ago ...
I'm afraid we would have to deserialize the structured clone in the parent process too, to get this right :(

Comment 25

5 years ago
The IndexedDB patch looks great otherwise.
Created attachment 647116 [details] [diff] [review]
no API change to MM
Created attachment 647117 [details] [diff] [review]
no API changes to MM
Attachment #647116 - Attachment is obsolete: true
Created attachment 647194 [details] [diff] [review]
conservative message data serialization
Attachment #647117 - Attachment is obsolete: true
Created attachment 647327 [details] [diff] [review]
Patch

This is almost totally done. It incorporates all the feedback thus far and smaug's fixes. The only thing I still need to do is add the DCHECK stuff that cjones pointed out.
Attachment #643061 - Attachment is obsolete: true
Attachment #643824 - Attachment is obsolete: true
Attachment #643061 - Flags: review?(jonas)
Attachment #643824 - Flags: feedback?(jonas)
Attachment #643824 - Flags: feedback?(Jan.Varga)
Blocks: 778968
Comment on attachment 647327 [details] [diff] [review]
Patch

r? to:

smaug for message manager stuff
khuey for basic protocol and actor implementation
janv for indexeddb changes
Attachment #647327 - Flags: review?(khuey)
Attachment #647327 - Flags: review?(bugs)
Attachment #647327 - Flags: review?(Jan.Varga)
Comment on attachment 647327 [details] [diff] [review]
Patch

>+run-if.config = ipc

This is not a real thing.
https://tbpl.mozilla.org/?tree=Try&rev=198a04dfee5d shows everything compiling properly.
Created attachment 647666 [details] [diff] [review]
v5 (no asyncSendCloneMessage)

Fixes the case when sc is empty.
Attachment #647194 - Attachment is obsolete: true
Created attachment 647710 [details] [diff] [review]
v5, changes only
Attachment #647327 - Flags: review?(khuey) → review+

Updated

5 years ago
Attachment #647666 - Flags: review?(bent.mozilla)
Comment on attachment 647710 [details] [diff] [review]
v5, changes only

Review of attachment 647710 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsFrameLoader.cpp
@@ +2165,5 @@
> +    if (aData.mDataLength) {
> +      if (!mData.copy(aData.mData, aData.mDataLength)) {
> +        NS_RUNTIMEABORT("OOM");
> +      }
> +      mClosure = aData.mClosure;

Unclear that this is correct... I think it's safer to always assign mClosure even if aData.mDataLength is null.

::: content/base/src/nsFrameMessageManager.cpp
@@ +176,5 @@
>    return true;
>  }
>  
> +static bool
> +GetParamsForMessage(JSContext* aCx,

I really don't like the idea of trying to clone, failing, writing JSON, then parsing JSON, and then cloning the JSON. Is this necessary to pass some test or something? Can we just not do this?

Seems like we should at least print one heck of a warning if this super slow path is reached.

@@ +437,5 @@
>          jsval json = JSVAL_NULL;
> +        if (aCloneData && aCloneData->mDataLength &&
> +            !ReadStructuredClone(ctx, *aCloneData, &json)) {
> +          JS_ClearPendingException(ctx);
> +          json = JSVAL_NULL;

Here you're eating an OOM and you're going to give script a null instead of the data it wanted. Why?

@@ +961,5 @@
> +    if (aData.mDataLength) {
> +      if (!mData.copy(aData.mData, aData.mDataLength)) {
> +        NS_RUNTIMEABORT("OOM");
> +      }
> +      mClosure = aData.mClosure;

Same comment about always setting mClosure

::: content/base/src/nsInProcessTabChildGlobal.cpp
@@ +58,5 @@
> +    if (aData.mDataLength) {
> +      if (!mData.copy(aData.mData, aData.mDataLength)) {
> +        NS_RUNTIMEABORT("OOM");
> +      }
> +      mClosure = aData.mClosure;

Here too.

::: dom/ipc/StructuredCloneUtils.h
@@ +26,5 @@
>  
>  struct
>  StructuredCloneData
>  {
> +  StructuredCloneData() : mData(nullptr), mDataLength(0) {}

Did you need this? I guess it can't hurt, but I'd be surprised if IPDL left it uninitialized somewhere.

Comment 36

5 years ago
>--- a/dom/indexedDB/IDBObjectStore.cpp
>+++ b/dom/indexedDB/IDBObjectStore.cpp
>+nsresult
>+ConvertBlobActors(ContentParent* aContentParent,
>+                  FileManager* aFileManager,
>+                  const nsTArray<StructuredCloneFile>& aFiles,
>+                  InfallibleTArray<PBlobParent*>& aActors)
>+{
>+  NS_ASSERTION(aContentParent, "Null contentParent!");
>+  NS_ASSERTION(aFileManager, "Null file manager!");
>+
>+  if (!aFiles.IsEmpty()) {
>+    PRUint32 fileCount = aFiles.Length();
>+    aActors.SetCapacity(fileCount);
>+
>+    for (PRUint32 index = 0; index < fileCount; index++) {
>+      const StructuredCloneFile& file = aFiles[index];
>+      NS_ASSERTION(file.mFileInfo, "This should never be null!");
>+
>+      nsCOMPtr<nsIFile> directory = aFileManager->GetDirectory();
>+      if (!directory) {
>+        NS_WARNING("Failed to get directory!");
>+        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>+      }

GetDirectory() call can be moved outside of the loop
(In reply to ben turner [:bent] from comment #35)
> Comment on attachment 647710 [details] [diff] [review]
> v5, changes only
> 
> Review of attachment 647710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsFrameLoader.cpp
> @@ +2165,5 @@
> > +    if (aData.mDataLength) {
> > +      if (!mData.copy(aData.mData, aData.mDataLength)) {
> > +        NS_RUNTIMEABORT("OOM");
> > +      }
> > +      mClosure = aData.mClosure;
> 
> Unclear that this is correct... I think it's safer to always assign mClosure
> even if aData.mDataLength is null.
Can aData.mClosure contain something if mDataLength is 0 ?


> > +GetParamsForMessage(JSContext* aCx,
> 
> I really don't like the idea of trying to clone, failing, writing JSON, then
> parsing JSON, and then cloning the JSON. Is this necessary to pass some test
> or something?
Yes, t was some contacts test, IIRC.

> Can we just not do this?
I choose the safest possible way (which is very ugly).



> 
> Seems like we should at least print one heck of a warning if this super slow
> path is reached.
Yes

> @@ +437,5 @@
> >          jsval json = JSVAL_NULL;
> > +        if (aCloneData && aCloneData->mDataLength &&
> > +            !ReadStructuredClone(ctx, *aCloneData, &json)) {
> > +          JS_ClearPendingException(ctx);
> > +          json = JSVAL_NULL;
> 
> Here you're eating an OOM and you're going to give script a null instead of
> the data it wanted. Why?
Well, presumably the data is coming from another process, like content process which is possibly evil.
So if it sends invalid data, no need to throw exceptions in another process.


> >  StructuredCloneData
> >  {
> > +  StructuredCloneData() : mData(nullptr), mDataLength(0) {}
> 
> Did you need this? I guess it can't hurt, but I'd be surprised if IPDL left
> it uninitialized somewhere.
Again, I added this when hunting the problem which ended up being 0 length sc.
But I was somewhat horrified to see uninitialized member variables.
(In reply to Olli Pettay [:smaug] from comment #37)

> > Here you're eating an OOM and you're going to give script a null instead of
> > the data it wanted. Why?
> Well, presumably the data is coming from another process, like content
> process which is possibly evil.
> So if it sends invalid data, no need to throw exceptions in another
But you're right, returning would be the right thing to do.
I think the need to use json is actually a bug in contactsmanager. I'll ask gwagner.
So in contacts there is interface nsIDOMContactFindOptions which is passed as part
of the message. json can somewhat serialize it (if the JS implementation of the interface doesn't
have all the properties, they become "undefined" or "0").

nsIDOMContactFindOptions shouldn't be an interfaces at all, but a dictionary, but I'm not sure
whether implementing methods which take a dictionary as a parameter is actually supported.
Investigating...

But, for now supporting json messages should be ok (even if it is a bit slower)
Created attachment 647934 [details] [diff] [review]
v7
Created attachment 647935 [details] [diff] [review]
v7, changes

this misses the return NS_OK change in ReceiveMessage, but that is in the full patch.

Updated

5 years ago
Attachment #647327 - Flags: review?(Jan.Varga) → review+
Comment on attachment 647327 [details] [diff] [review]
Patch

So implicit r+ within my patches
Attachment #647327 - Flags: review?(bugs) → review+
Attachment #647666 - Flags: review?(bent.mozilla)
Comment on attachment 647934 [details] [diff] [review]
v7

These changes look good.
Attachment #647934 - Flags: review+
ben, could you upload the patch which fixes IDB tests (the one which passed in ipc-mode, but not
in non-ipc-mode)
Created attachment 648148 [details] [diff] [review]
Final patch

This is the final patch. Incorporates all previous patches and review comments from everyone.
Attachment #647327 - Attachment is obsolete: true
Attachment #647666 - Attachment is obsolete: true
Attachment #647710 - Attachment is obsolete: true
Attachment #647934 - Attachment is obsolete: true
Attachment #647935 - Attachment is obsolete: true
Attachment #648148 - Flags: review+
Oh, I will changes nsnull -> nullptr here before I land.
Final patch landed here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4048c64af6de

I did have to make some changes to nsBlobProtocolHandler (simply moved nsBlobURI out into its own file so that it can be used in nsLayoutModule.cpp), those have rs=sicking.
Depends on: 779735
https://hg.mozilla.org/mozilla-central/rev/4048c64af6de
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 648148 [details] [diff] [review]
Final patch

>+  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const jschar*>(PromiseFlatString(json).get()),
>+                              json.Length(), &val), false);
[json is already flat, so no need to PromiseFlatString it. But this is Ms2ger's fault for not using BeginReading in the first place.]
(In reply to neil@parkwaycc.co.uk from comment #50)
> Comment on attachment 648148 [details] [diff] [review]
> Final patch
> 
> >+  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const jschar*>(PromiseFlatString(json).get()),
> >+                              json.Length(), &val), false);
> [json is already flat, so no need to PromiseFlatString it. But this is
> Ms2ger's fault for not using BeginReading in the first place.]

Troll much? The .get() has been there ever since bug 549682 if you feel like you need to assign blame.
Comment on attachment 648148 [details] [diff] [review]
Final patch

Review of attachment 648148 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsIFrameMessageManager.idl
@@ +23,3 @@
>     *     name:    %message name%,
>     *     sync:    %true or false%.
> +   *     json:    %structured clone of the sent message data%,

itym

  data: ...

here?

Sorry, somewhat late to the party. I'm actually in that file so I'll correct that as part of the process.
Duplicate of this bug: 754353
Blocks: 847744
No longer blocks: 760065
You need to log in before you can comment on or make changes to this bug.