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.

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.

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

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.

Splitting blob and file handle support into separate bugs. File handle is bug 771288.

Also blocks MMS DOM API

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.

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

I fixed the pickle stuff so that it asserts our marker now.

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.

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

For the record, unless doing sync stuff is trivial, I don't think we should block on it here.

It should be trivial.

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.

looking at the patch ...

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; >+} >+

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

The IndexedDB patch looks great otherwise.

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.

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

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.

Fixes the case when sc is empty.

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.

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

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

Comment on attachment 647327 [details] [diff] [review] Patch So implicit r+ within my patches

Comment on attachment 647934 [details] [diff] [review] v7 These changes look good.

ben, could you upload the patch which fixes IDB tests (the one which passed in ipc-mode, but not in non-ipc-mode)

This is the final patch. Incorporates all previous patches and review comments from everyone.

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.

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.