1311466 - Enable storing WebAssembly modules in IndexedDB

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

[Jan Varga [:janv]]

Attachment: Part 1: Define only one file type; r=asuth

Comment 2

[Jan Varga [:janv]]

Attachment: Part 2: Refactor IPDL structs to use file types too; r=asuth

Comment 3

[Jan Varga [:janv]]

Attachment: Part 3: Expose serialization into a buffer; r=luke

Comment 4

[Jan Varga [:janv]]

Attachment: Part 4: Core changes for WebAssembly module serialization including a test; r=asuth

Comment 5

[Jan Varga [:janv]]

Attachment: Part 5: Expose file stream internal file descriptor through nsIFileMetadata interface; r=froydnj

Comment 6

[Jan Varga [:janv]]

Attachment: Part 6: Fix remote blobs to support nsIFileMetadata interface; r=baku

Comment 7

[Jan Varga [:janv]]

Attachment: Part 7: Add a new method for creating wasm module objects; r=luke

Comment 8

[Jan Varga [:janv]]

Attachment: Part 8: Core changes for WebAssembly module deserialization (works only in xpcshell); r=asuth

Comment 9

[Jan Varga [:janv]]

Attachment: Part 9: Try to unwrap WebAssembly modules passed to WasmExtractCode; r=luke

Comment 10

[Jan Varga [:janv]]

Attachment: Part 10: Implement basic functionality for preprocessing results before they are sent for synchronous deserialization; r=asuth

Comment 11

[Jan Varga [:janv]]

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91c0f43080365cedc419c922ba65ba825838abd2

Comment 12

[Jan Varga [:janv]]

Needed before initial landing:
- SpiderMonkey changes for serializing to/from two separate files (bytecode and machinecode)

TODO:
- implement preprocessing for objectStore.getAll(), index.get(), index.getAll() and cursors
- support for DOM workers
- sharing of WebAssembly modules (like sharing of DOM blobs)
- automatic recompilation of machinecode when cpu or build id doesn't match

Anyway, the additional SpiderMonkey changes plus those 10 patches should be enough for initial landing, we can iterate later. This stuff is still behind a pref.

Comment 13

[Jan Varga [:janv]]

Attachment: Part 7: Add a new method for creating wasm module objects; r=luke

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8802827 [details] [diff] [review]
Part 5: Expose file stream internal file descriptor through nsIFileMetadata interface; r=froydnj

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

I think this is OK, though the commit message should be modified, as it bears little correspondence with what the patch actually does.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8802825 [details] [diff] [review]
Part 3: Expose serialization into a buffer; r=luke

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

It's the right change; this should be subsumed by the new patch set in bug 1276029.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8802933 [details] [diff] [review]
Part 7: Add a new method for creating wasm module objects; r=luke

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

Similarly, this is the right change, but should be subsumed by the updated patch in bug 1276029.

Comment 17

[Jan Varga [:janv]]

Attachment: Part 3: Core changes for WebAssembly module serialization including a test; r=asuth

Comment 18

[Jan Varga [:janv]]

Attachment: Part 4: Add a C++ only getFileDescriptor() method to nsIFileMetadata interface...

Part 4: Add a C++ only getFileDescriptor() method to nsIFileMetadata interface. All file stream implementations (nsFileInputStream, nsPartialFileInputStream, nsFileOutputStream, nsAtomicFileOutputStream, nsSafeFileOutputStream and nsFileStream) support nsIFileMetadata interface so the new method is available on all these streams. The returned file descriptor can be used for memory mapping of the underlying file. The new method exposes stream's internal member variable, so it should be used carefully; r=froydnj

Comment 19

[Jan Varga [:janv]]

Attachment: Part 5: Fix remote blobs to support nsIFileMetadata interface; r=baku

Comment 20

[Jan Varga [:janv]]

Attachment: Part 6: Core changes for WebAssembly module deserialization (works only in xpcshell); r=asuth

Comment 21

[Jan Varga [:janv]]

Attachment: Part 7: Try to unwrap WebAssembly modules passed to WasmExtractCode; r=luke

Comment 22

[Jan Varga [:janv]]

Attachment: Part 8: Implement basic functionality for preprocessing results before they are sent for synchronous deserialization; r=asuth

Comment 23

[Jan Varga [:janv]]

Attachment: Part 9: Extract common code for sending preprocess info; r=asuth

Comment 24

[Jan Varga [:janv]]

Attachment: Part 10: Implement functionality for preprocessing multiple results, fixed objectStore.getAll() to use it including a new test; r=asuth

Comment 25

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ec54582e43375f068634af311407ebd9aba3a31

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8803404 [details] [diff] [review]
Part 6: Core changes for WebAssembly module deserialization (works only in xpcshell); r=asuth

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

::: dom/indexedDB/ActorsParent.cpp
@@ +124,5 @@
>    {0x6b505c84, 0x2c60, 0x4ffb, {0x8b, 0x91, 0xfe, 0x22, 0xb1, 0xec, 0x75, 0xe2}}
>  
>  namespace mozilla {
> +
> +MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPRFileDesc,

Scoped.h says it's deprecated in favor of UniquePtr: http://searchfox.org/mozilla-central/source/mfbt/Scoped.h#13

Unfortunately, the correct fixes are slightly more boilerplatey.  Not sure what to do about that, although since at least one wasm patch included the same code, I suspect you're just following an existing wasm idiom.  Maybe just add a comment referencing bug 1037100 or a more specific bug you file?
* NSS has one at http://searchfox.org/mozilla-central/source/security/manager/ssl/ScopedNSSTypes.h#386 that uses a MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE macro it introduces at http://searchfox.org/mozilla-central/source/security/manager/ssl/ScopedNSSTypes.h#99
* nsTerminator does UniquePtr<PRFileDesc, PR_CloseDelete> where it introduced a manual helper PR_CloseDelete at http://searchfox.org/mozilla-central/source/toolkit/components/terminator/nsTerminator.cpp#173 which usage at http://searchfox.org/mozilla-central/source/toolkit/components/terminator/nsTerminator.cpp#269

::: dom/indexedDB/IndexedDatabase.h
@@ +37,5 @@
>    RefPtr<Blob> mBlob;
>    RefPtr<IDBMutableFile> mMutableFile;
>    RefPtr<FileInfo> mFileInfo;
>    FileType mType;
> +  bool mValid;

nit: A comment that this is specific to wasm validity at this time or renaming to mValidWasm to otherwise clarify would be useful I think.

Comment 27

[Jan Varga [:janv]]

This is almost ready, just awaiting reviews for part 5 and part 8.

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8803403 [details] [diff] [review]
Part 5: Fix remote blobs to support nsIFileMetadata interface; r=baku

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

While you're touching RemoteInputStream, you might want to fix RemoteInputStream::Available's IsOnOwningThread case.  Specifically, this code looks like the last `NS_ENSURE_SUCCESS(rv, rv);` really wants to be `return rv;` instead of falling through.  The only real harm would seem to be the potential for the DEBUG main-thread NS_WARNING to fire, of course.

  if (!IsOnOwningThread()) {
    nsresult rv = BlockAndWaitForStream();
    NS_ENSURE_SUCCESS(rv, rv);

    rv = mStream->Available(aAvailable);
    NS_ENSURE_SUCCESS(rv, rv);
  }

Comment 29

[Andrea Marchesini [:baku]]

Comment on attachment 8803403 [details] [diff] [review]
Part 5: Fix remote blobs to support nsIFileMetadata interface; r=baku

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

::: dom/ipc/Blob.cpp
@@ +32,5 @@
>  #include "mozilla/ipc/PFileDescriptorSetParent.h"
>  #include "MultipartBlobImpl.h"
>  #include "nsDataHashtable.h"
>  #include "nsHashKeys.h"
> +#include "nsIFileStreams.h"

alphabetic order... ? or a kind of...

@@ +1355,5 @@
>    MOZ_CRASH("RemoteInputStream should never be deserialized");
>  }
>  
> +NS_IMETHODIMP
> +RemoteInputStream::GetSize(int64_t* _retval)

aRetval or better aSize

@@ +1376,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +RemoteInputStream::GetLastModified(int64_t* _retval)

same.

@@ +1397,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +RemoteInputStream::GetFileDescriptor(PRFileDesc** _retval)

same

Comment 30

[Jan Varga [:janv]]

Comment on attachment 8803400 [details] [diff] [review]
Part 3: Core changes for WebAssembly module serialization including a test; r=asuth

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

::: dom/base/StructuredCloneTags.h
@@ +56,5 @@
>  
>    // This tag is used by both main thread and workers.
>    SCTAG_DOM_URLSEARCHPARAMS,
>  
> +  SCTAG_DOM_WASM,

Btw, I moved this right after SCTAG_DOM_FILE.

I was doing a rebase and I noticed that someone removed an entry from the list, bug 1310859. So the value of SCTAG_DOM_WASM changed and our static assert fired during a rebuild. If I didn't move the new tag then in future removing of a tag would be more difficult I think. It seems we should keep all the tags we support in IDB in front of the list.

Comment 31

[Jan Varga [:janv]]

I added a comment for that.

diff --git a/dom/base/StructuredCloneTags.h b/dom/base/StructuredCloneTags.h
--- a/dom/base/StructuredCloneTags.h
+++ b/dom/base/StructuredCloneTags.h
@@ -24,16 +24,18 @@ enum StructuredCloneTags {
   SCTAG_DOM_FILE_WITHOUT_LASTMODIFIEDDATE,
 
   SCTAG_DOM_FILELIST,
   SCTAG_DOM_MUTABLEFILE,
   SCTAG_DOM_FILE,
 
   SCTAG_DOM_WASM,
 
+  // New IDB tags go here!
+
   // These tags are used for both main thread and workers.
   SCTAG_DOM_IMAGEDATA,
   SCTAG_DOM_MAP_MESSAGEPORT,
 
   SCTAG_DOM_FUNCTION,
 
   // This tag is for WebCrypto keys
   SCTAG_DOM_WEBCRYPTO_KEY,
@@ -53,15 +55,21 @@ enum StructuredCloneTags {
 
   SCTAG_DOM_EXPANDED_PRINCIPAL,
 
   SCTAG_DOM_DIRECTORY,
 
   // This tag is used by both main thread and workers.
   SCTAG_DOM_URLSEARCHPARAMS,
 
+  // When adding a new tag for IDB, please don't add it to the end of the list!
+  // Tags that are supported by IDB must not ever change. See the static assert
+  // in IDBObjectStore.cpp, method CommonStructuredCloneReadCallback.
+  // Adding to the end of the list would make removing of other tags harder in
+  // future.
+
   SCTAG_DOM_MAX
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // StructuredCloneTags_h__

Comment 32

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8803449 [details] [diff] [review]
Part 8: Implement basic functionality for preprocessing results before they are sent for synchronous deserialization; r=asuth

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

::: dom/indexedDB/ActorsChild.cpp
@@ +735,5 @@
> +
> +            file->mType = serializedFile.type();
> +
> +            MOZ_ASSERT(moduleIndex < aModules->Length());
> +            file->mWasmModule = aModules->ElementAt(moduleIndex++);

Given that moduleIndex++ is incremented on eWasmCompiled and therefore the ordering of the [eWasmBytecode, eWasmCompiled] pair is now even more important, I'd suggest adding a comment to StructuredCloneWriteCallback in IDBObjectStore.cpp to make it clear that the ordering is significant and must never be changed (without deep contemplation).

@@ +2889,5 @@
> +  AssertIsOnOwningThread();
> +
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsIEventTarget> target =
> +      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);

I think the rationale here is that the stream transport service exists to make synchronous things async, plus it has a threadpool with a reasonably high cap of 25 threads.  This seems sufficiently sound, but I think it would be helpful to put in a comment.

@@ +2897,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +  } else {
> +    MOZ_ASSERT(false, "Fix me!");

I know in comment 12 you mentioned that support for DOM workers was still to be a todo, but I don't understand why the stream transport service dispatch is gated behind NS_IsMainThread()?  Both nsStreamTransportService and its used nsThreadPool use threadsafe isupports.  (Also nsStreamTransportService::Dispatch uses a MutexAutoLock and so does nsThreadPool::PutEvent which is where the calls end up.)

Please add an explanatory comment with bug number for follow-up if this is something more than concern about STS's thread-safety and instead just a weird place to enforce that workers can't have this feature.

::: dom/indexedDB/ActorsParent.cpp
@@ +6053,5 @@
>  
>  class TransactionDatabaseOperationBase
>    : public DatabaseOperationBase
>  {
> +  enum class InternalState

This is a great cleanup and I really appreciate the comprehensive asserts that make it easy to understand and be confident in the progression of the state machine.

::: dom/indexedDB/PBackgroundIDBRequest.ipdl
@@ +126,5 @@
> +};
> +
> +union PreprocessResponse
> +{
> +  nsresult;

It's probably worth adding a brief comment here that the nsresult is used if an error occurs for any preprocess request type, but the specific response types are sent on success.

@@ +141,4 @@
>  child:
>    async __delete__(RequestResponse response);
> +
> +  async Preprocess(PreprocessParams params);

I think a comment here or elsewhere elaborating on why Preprocess needs to exist would be really useful.  For example:

Preprocess is used in cases where response processing needs to do something asynchronous off of the child actor's thread before returning the actual result to user code.  This is necessary because RequestResponse processing occurs in __delete__ and the PBackgroundIDBRequest implementations' life-cycles are controlled by IPC and are not otherwise reference counted.  By introducing the (optional) Preprocess/Continue steps reference counting or the introduction of additional runnables are avoided.

::: dom/indexedDB/test/file.js
@@ +209,5 @@
>    let exp1 = wasmExtractCode(module1);
>    let exp2 = wasmExtractCode(module2);
>    let code1 = exp1.code;
>    let code2 = exp2.code;
> +  todo(code1 instanceof Uint8Array, "Instance of Uint8Array");

Can this `todo` become an `ok` again?  I don't see any fixes in the other patches.  If you can't make it `ok`, I think this needs a comment referencing the (possibly freshly filed) bug where the problem will be addressed.

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8803729 [details] [diff] [review]
Part 10: Implement functionality for preprocessing multiple results, fixed objectStore.getAll() to use it including a new test; r=asuth

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

::: dom/indexedDB/ActorsChild.cpp
@@ +2785,5 @@
> +                                    files);
> +
> +
> +    RefPtr<PreprocessHelper>& preprocessHelper = mPreprocessHelpers[index];
> +    preprocessHelper = new PreprocessHelper(index, this);

Spawning all of these in parallel to perform synchronous I/O on the stream transport service's shared-by-everyone's max=25 threads has the potential to cause some annoying browser hiccups.  Since these are reads coming off disk and that means the quota manager already gets to limit/bound the I/O, I don't think it's likely to be a huge problem.  That said, I think I'd still feel better if there was a comment here making the potential implications more clear.

(From an I/O perspective, it may be worth considering using a single thread or a very small threadpool owned by the BackgroundRequestChild so spinning disks are seeing bulk reads more than a bunch random reads.  But I expect the normal wasm use-cases wouldn't benefit, so why add the complexity.)

::: dom/indexedDB/ActorsParent.cpp
@@ -5924,5 @@
>                                            uint32_t aDataIndex,
>                                            uint32_t aFileIdsIndex,
>                                            FileManager* aFileManager,
> -                                          StructuredCloneReadInfo* aInfo,
> -                                          bool* aHasWasm)

It's great to see aHasWasm be removed!

::: dom/indexedDB/test/unit/test_wasm_getAll.js
@@ +13,5 @@
> +    this.window ? window.location.pathname : "test_wasm_getAll.js";
> +
> +  const objectStoreInfo = { name: "Wasm", options: { autoIncrement: true } };
> +
> +  const values = [ 42, null ];

It's a little confusing that you initialize this like so and then clobber index 1 (the null) and append index 2 below with the wasm arrays.  Did you mean for this to be const values = [[42, null]]?  (My concern is mainly it's not clear if these specific values have importance to the test, so assuming they do, clobbering null seems wrong.)

Comment 34

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #28)
> While you're touching RemoteInputStream, you might want to fix
> RemoteInputStream::Available's IsOnOwningThread case.  Specifically, this
> code looks like the last `NS_ENSURE_SUCCESS(rv, rv);` really wants to be
> `return rv;` instead of falling through.  The only real harm would seem to
> be the potential for the DEBUG main-thread NS_WARNING to fire, of course.
> 
>   if (!IsOnOwningThread()) {
>     nsresult rv = BlockAndWaitForStream();
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     rv = mStream->Available(aAvailable);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }

It seems like the right thing to do. I added "return NS_OK" there.

Comment 35

[Jan Varga [:janv]]

(In reply to Andrea Marchesini [:baku] from comment #29)
> @@ +32,5 @@
> >  #include "mozilla/ipc/PFileDescriptorSetParent.h"
> >  #include "MultipartBlobImpl.h"
> >  #include "nsDataHashtable.h"
> >  #include "nsHashKeys.h"
> > +#include "nsIFileStreams.h"
> 
> alphabetic order... ? or a kind of...

Ok, fixed.

> 
> @@ +1355,5 @@
> >    MOZ_CRASH("RemoteInputStream should never be deserialized");
> >  }
> >  
> > +NS_IMETHODIMP
> > +RemoteInputStream::GetSize(int64_t* _retval)
> 
> aRetval or better aSize

Fixed here and elsewhere too.

Comment 36

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #32)
> @@ +735,5 @@
> > +
> > +            file->mType = serializedFile.type();
> > +
> > +            MOZ_ASSERT(moduleIndex < aModules->Length());
> > +            file->mWasmModule = aModules->ElementAt(moduleIndex++);
> 
> Given that moduleIndex++ is incremented on eWasmCompiled and therefore the
> ordering of the [eWasmBytecode, eWasmCompiled] pair is now even more
> important, I'd suggest adding a comment to StructuredCloneWriteCallback in
> IDBObjectStore.cpp to make it clear that the ordering is significant and
> must never be changed (without deep contemplation).

Ok, I added this comment:
// The ordering of the bytecode and compiled file is significant and must
// never be changed. These two files must always form a pair
// [eWasmBytecode, eWasmCompiled]. Everything else depends on it!

> @@ +2889,5 @@
> > +  AssertIsOnOwningThread();
> > +
> > +  if (NS_IsMainThread()) {
> > +    nsCOMPtr<nsIEventTarget> target =
> > +      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
> 
> I think the rationale here is that the stream transport service exists to
> make synchronous things async, plus it has a threadpool with a reasonably
> high cap of 25 threads.  This seems sufficiently sound, but I think it would
> be helpful to put in a comment.

Ok, added.

> @@ +2897,5 @@
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      return rv;
> > +    }
> > +  } else {
> > +    MOZ_ASSERT(false, "Fix me!");
> 
> I know in comment 12 you mentioned that support for DOM workers was still to
> be a todo, but I don't understand why the stream transport service dispatch
> is gated behind NS_IsMainThread()?  Both nsStreamTransportService and its
> used nsThreadPool use threadsafe isupports.  (Also
> nsStreamTransportService::Dispatch uses a MutexAutoLock and so does
> nsThreadPool::PutEvent which is where the calls end up.)

Oh cool, it works! I didn't know that.
I tweaked JS helpers a bit and I'm now able to run the test on workers too!

> @@ +126,5 @@
> > +};
> > +
> > +union PreprocessResponse
> > +{
> > +  nsresult;
> 
> It's probably worth adding a brief comment here that the nsresult is used if
> an error occurs for any preprocess request type, but the specific response
> types are sent on success.

Ok, added.

> 
> @@ +141,4 @@
> >  child:
> >    async __delete__(RequestResponse response);
> > +
> > +  async Preprocess(PreprocessParams params);
> 
> I think a comment here or elsewhere elaborating on why Preprocess needs to
> exist would be really useful.  For example:
> 
> Preprocess is used in cases where response processing needs to do something
> asynchronous off of the child actor's thread before returning the actual
> result to user code.  This is necessary because RequestResponse processing
> occurs in __delete__ and the PBackgroundIDBRequest implementations'
> life-cycles are controlled by IPC and are not otherwise reference counted. 
> By introducing the (optional) Preprocess/Continue steps reference counting
> or the introduction of additional runnables are avoided.

I really like this comment, ok, added.

> ::: dom/indexedDB/test/file.js
> @@ +209,5 @@
> >    let exp1 = wasmExtractCode(module1);
> >    let exp2 = wasmExtractCode(module2);
> >    let code1 = exp1.code;
> >    let code2 = exp2.code;
> > +  todo(code1 instanceof Uint8Array, "Instance of Uint8Array");
> 
> Can this `todo` become an `ok` again?  I don't see any fixes in the other
> patches.  If you can't make it `ok`, I think this needs a comment
> referencing the (possibly freshly filed) bug where the problem will be
> addressed.

I'm going to ask luke if he can investigate this.

Comment 37

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #36)
> > > +  todo(code1 instanceof Uint8Array, "Instance of Uint8Array");
> > 
> > Can this `todo` become an `ok` again?  I don't see any fixes in the other
> > patches.  If you can't make it `ok`, I think this needs a comment
> > referencing the (possibly freshly filed) bug where the problem will be
> > addressed.
> 
> I'm going to ask luke if he can investigate this.

I think the issue is that SpecialPowers.Cu.getJSTestingFunctions().x produces a JS-scripted-proxy-wrapped function (so not the builtin proxies that tend to be handled well by instanceof).  Does it work if you call unwrap() like I did in the WebAssembly.compile promise tests?
  http://hg.mozilla.org/mozilla-central/file/tip/dom/promise/tests/test_webassembly_compile.html#l13

Comment 38

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #37)
> (In reply to Jan Varga [:janv] from comment #36)
> > > > +  todo(code1 instanceof Uint8Array, "Instance of Uint8Array");
> > > 
> > > Can this `todo` become an `ok` again?  I don't see any fixes in the other
> > > patches.  If you can't make it `ok`, I think this needs a comment
> > > referencing the (possibly freshly filed) bug where the problem will be
> > > addressed.
> > 
> > I'm going to ask luke if he can investigate this.
> 
> I think the issue is that SpecialPowers.Cu.getJSTestingFunctions().x
> produces a JS-scripted-proxy-wrapped function (so not the builtin proxies
> that tend to be handled well by instanceof).  Does it work if you call
> unwrap() like I did in the WebAssembly.compile promise tests?
>  
> http://hg.mozilla.org/mozilla-central/file/tip/dom/promise/tests/
> test_webassembly_compile.html#l13

I tried that yeah, maybe incorrectly. I tried other things and spent non trivial time with it and finally gave up :)

Comment 39

[Luke Wagner [:luke]]

Righto, I'll try to fix up after landing then :)

Comment 40

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #26)
> @@ +124,5 @@
> >    {0x6b505c84, 0x2c60, 0x4ffb, {0x8b, 0x91, 0xfe, 0x22, 0xb1, 0xec, 0x75, 0xe2}}
> >  
> >  namespace mozilla {
> > +
> > +MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPRFileDesc,
> 
> Scoped.h says it's deprecated in favor of UniquePtr:
> http://searchfox.org/mozilla-central/source/mfbt/Scoped.h#13
> 
> Unfortunately, the correct fixes are slightly more boilerplatey.  Not sure
> what to do about that, although since at least one wasm patch included the
> same code, I suspect you're just following an existing wasm idiom.  Maybe
> just add a comment referencing bug 1037100 or a more specific bug you file?
> * NSS has one at
> http://searchfox.org/mozilla-central/source/security/manager/ssl/
> ScopedNSSTypes.h#386 that uses a MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE macro
> it introduces at
> http://searchfox.org/mozilla-central/source/security/manager/ssl/
> ScopedNSSTypes.h#99
> * nsTerminator does UniquePtr<PRFileDesc, PR_CloseDelete> where it
> introduced a manual helper PR_CloseDelete at
> http://searchfox.org/mozilla-central/source/toolkit/components/terminator/
> nsTerminator.cpp#173 which usage at
> http://searchfox.org/mozilla-central/source/toolkit/components/terminator/
> nsTerminator.cpp#269

Sorry I gave up on this. I really tried, but it's just too hard to use it currently.
I also couldn't find fileDesc.rwget() equivalent. The current consumers of the UniquePtr stuff pass PR_Open() directly to its constructor, e.g. fileDesc(PR_Open(...))
But I need to do it with a reference, e.g. OpenNSPRFileDesc(&fileDesc)

> @@ +37,5 @@
> >    RefPtr<Blob> mBlob;
> >    RefPtr<IDBMutableFile> mMutableFile;
> >    RefPtr<FileInfo> mFileInfo;
> >    FileType mType;
> > +  bool mValid;
> 
> nit: A comment that this is specific to wasm validity at this time or
> renaming to mValidWasm to otherwise clarify would be useful I think.

Ok, added a comment.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd47b2b0807
Part 1: Define only one file type; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/134a73388a64
Part 2: Refactor IPDL structs to use file types too; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/4738f002dec6
Part 3: Core changes for WebAssembly module serialization including a test; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c24c16f58e1
Part 4: Add a C++ only getFileDescriptor() method to nsIFileMetadata interface. All file stream implementations (nsFileInputStream, nsPartialFileInputStream, nsFileOutputStream, nsAtomicFileOutputStream, nsSafeFileOutputStream and nsFileStream) support nsIFileMetadata interface so the new method is available on all these streams. The returned file descriptor can be used for memory mapping of the underlying file. The new method exposes stream's internal member variable, so it should be used carefully; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d12aaa1f3f45
Part 5: Fix remote blobs to support nsIFileMetadata interface; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/efee09f2e332
Part 6: Core changes for WebAssembly module deserialization (works only in xpcshell); r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ddfb4a361d9
Part 7: Try to unwrap WebAssembly modules passed to WasmExtractCode; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3eced266e0
Part 8: Implement basic functionality for preprocessing results before they are sent for synchronous deserialization; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0751b15d579
Part 9: Extract common code for sending preprocess info; r=asuth

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9476f6056e4b
A follow-up fix. Fix a bad implicit conversion constructor; r=me CLOSED TREE

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abd027328a92
A follow-up fix. Fix Cancel() return type to make windows happy; r=me

Comment 44

[Jan Varga [:janv]]

This was tested on try several times, but I made some recent changes to address review comments and wanted to finish/land this today, so other people will have some time for testing before they publish it on Friday.

Comment 45

[Luke Wagner [:luke]]

\o/

Comment 46

[Luke Wagner [:luke]]

Attachment: fix-verify-wasm-module

Oh hah, right: wasmExtractCode() executes in some SpecialPowers compartment/global and so we to pass instanceof the constructors of *that* global, not content.

Comment 47

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #33)
> @@ +2785,5 @@
> > +                                    files);
> > +
> > +
> > +    RefPtr<PreprocessHelper>& preprocessHelper = mPreprocessHelpers[index];
> > +    preprocessHelper = new PreprocessHelper(index, this);
> 
> Spawning all of these in parallel to perform synchronous I/O on the stream
> transport service's shared-by-everyone's max=25 threads has the potential to
> cause some annoying browser hiccups.  Since these are reads coming off disk
> and that means the quota manager already gets to limit/bound the I/O, I
> don't think it's likely to be a huge problem.  That said, I think I'd still
> feel better if there was a comment here making the potential implications
> more clear.
> 
> (From an I/O perspective, it may be worth considering using a single thread
> or a very small threadpool owned by the BackgroundRequestChild so spinning
> disks are seeing bulk reads more than a bunch random reads.  But I expect
> the normal wasm use-cases wouldn't benefit, so why add the complexity.)

Ok I added a comment, I agree we need an own thread/threadpool, but we had to use the stream transport service due to time constraints.

> @@ +13,5 @@
> > +    this.window ? window.location.pathname : "test_wasm_getAll.js";
> > +
> > +  const objectStoreInfo = { name: "Wasm", options: { autoIncrement: true } };
> > +
> > +  const values = [ 42, null ];
> 
> It's a little confusing that you initialize this like so and then clobber
> index 1 (the null) and append index 2 below with the wasm arrays.  Did you
> mean for this to be const values = [[42, null]]?  (My concern is mainly it's
> not clear if these specific values have importance to the test, so assuming
> they do, clobbering null seems wrong.)

Fixed

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e2a14f1911
Part 10: Implement functionality for preprocessing multiple results, fixed objectStore.getAll() to use it including a new test; r=asuth

Comment 49

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #47)
> > (From an I/O perspective, it may be worth considering using a single thread
> > or a very small threadpool owned by the BackgroundRequestChild so spinning
> > disks are seeing bulk reads more than a bunch random reads.  But I expect
> > the normal wasm use-cases wouldn't benefit, so why add the complexity.)
> 
> Ok I added a comment, I agree we need an own thread/threadpool, but we had
> to use the stream transport service due to time constraints.

However, I'm not sure if BackgroundRequestChild instance should own it. The same over spawning can happen if JS calls get() multiple times and responses contain wasm modules.

BackgroundFactoryChild could own it, but that can be tricky. I would rather create a new service for it, specific for IDB child processes. It would use the LazyIdleThread class (so just one lazily initialized thread with automatic shutdown on idle).

Comment 50

[Jan Varga [:janv]]

One more idea, the preprocessing doesn't have to be done at all if we communicate with a worker :) Workers can happily do synchronous I/O. However, we should do this optimization only if it doesn't add too much complexity to the code.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c22be6f4205e
A follow-up fix. Fix a todo in verifyWasmModule() by passing constructors of correct global to instanceof; r=janv

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7dd47b2b0807
https://hg.mozilla.org/mozilla-central/rev/134a73388a64
https://hg.mozilla.org/mozilla-central/rev/4738f002dec6
https://hg.mozilla.org/mozilla-central/rev/9c24c16f58e1
https://hg.mozilla.org/mozilla-central/rev/d12aaa1f3f45
https://hg.mozilla.org/mozilla-central/rev/efee09f2e332
https://hg.mozilla.org/mozilla-central/rev/8ddfb4a361d9
https://hg.mozilla.org/mozilla-central/rev/3c3eced266e0
https://hg.mozilla.org/mozilla-central/rev/d0751b15d579
https://hg.mozilla.org/mozilla-central/rev/9476f6056e4b
https://hg.mozilla.org/mozilla-central/rev/abd027328a92
https://hg.mozilla.org/mozilla-central/rev/a1e2a14f1911
https://hg.mozilla.org/mozilla-central/rev/c22be6f4205e

Comment 53

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #50)
> One more idea, the preprocessing doesn't have to be done at all if we
> communicate with a worker :) Workers can happily do synchronous I/O.
> However, we should do this optimization only if it doesn't add too much
> complexity to the code.

But will that synchronous I/O happen on the worker's main thread?  That could cause jank and lose parallelism for apps that primarily run in workers.

Comment 54

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #53)
> (In reply to Jan Varga [:janv] from comment #50)
> > One more idea, the preprocessing doesn't have to be done at all if we
> > communicate with a worker :) Workers can happily do synchronous I/O.
> > However, we should do this optimization only if it doesn't add too much
> > complexity to the code.
> 
> But will that synchronous I/O happen on the worker's main thread?  That
> could cause jank and lose parallelism for apps that primarily run in workers.

There's just one thread AFAIK, there are different queues, the primary one and a control one IIRC.
There are some APIs like FileReaderSync, which reads blobs/files synchronously too.
Anyway, if this is a bad idea, not a problem we have the preprocessing helper to do that on a dedicated I/O thread.

Comment 55

[Luke Wagner [:luke]]

Right, I'm not opposed to sync I/O in general or anything; I'm imagining that if an app is running primarily in a Worker (using OffscreenCanvas to render frames from that worker) that it'd be good to not do any sync deserialization on that Worker thread if we can avoid it using an I/O thread.

Comment 56

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jan Varga [:janv] from comment #49)
> However, I'm not sure if BackgroundRequestChild instance should own it. The
> same over spawning can happen if JS calls get() multiple times and responses
> contain wasm modules.

So... I was assuming that in the multiple get case that the I/O thread scheduling mechanism was going to block any subsequent get requests from the same transaction from running until the request state machine advanced in order to satisfy 2.7.1 Transaction Lifetime's (http://w3c.github.io/IndexedDB/#transaction-lifetime-concept) requirements that: "Unless otherwise defined, requests must be executed in the order in which they were made against the transaction. Likewise, their results must be returned in the order the requests were placed against a specific transaction."

Unfortunately I don't think there is or ended up being a mechanism to maintain this ordering invariant.  My apologies for not stating this as part of the review; I was going a bit faster on some of these WASM pieces and not keeping good review notes and it overflowed my mental stack and disappeared.

The simplest solution might be to have TransactionDatabaseOperationBase::RunOnConnectionThread do something like this at the end instead of what it does now:
```
if (HasPreprocessInfo()) {
  mInternalState = InternalState::SendingPreprocess;
  MOZ_ALWAYS_SUCCEEDS(mOwningThread->Dispatch(this, NS_DISPATCH_NORMAL));
  AcquireAMonitorPseudoCode(this);
  MOZ_ASSERT(mInternalState == InternalState::SendingResults);
} else {
  mInternalState = InternalState::SendingResults;
}

MOZ_ALWAYS_SUCCEEDS(mOwningThread->Dispatch(this, NS_DISPATCH_NORMAL));
```

And have TransactionDatabaseOperationBase::NoteContinueReceived() do something like:
```
MOZ_ASSERT(mInternalState == InternalState::WaitingForContinue);
mInternalState = InternalState::SendingResults;
NotifyMonitorPseudoCode(this);
// Note that we're not doing this->Run() anymore in the name of consistency/simplicity
```

And have NoteActorDestroyed invoke NoteContinueReceived() if (mInternalState == InternalState::WaitingForContinue).  And the on-demand monitor code is left as an exercise to the reader. ;)


By using a monitor to block the I/O thread I think it helps maintain the pre-WASM FIFO ordering simplicity.  Alternately, ConectionPool::Dispatch could keep things in its queue until the pending TransactionDatabaseOperationBase tells it that it is done.  The more complicated case would be desired if we wanted to run transactions more in parallel.  (This would only really make sense if you expect there to be more Preprocess steps in the future and that they can meaningfully run in parallel with IDB I/O thread operations without massive contention of the underlying resources.)


Setting needinfo because the ordering concern is important and I don't think we want this to get lost.  I expect this wants to be a spin-off bug that is addressed in the near future.

Comment 57

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #55)
> Right, I'm not opposed to sync I/O in general or anything; I'm imagining
> that if an app is running primarily in a Worker (using OffscreenCanvas to
> render frames from that worker) that it'd be good to not do any sync
> deserialization on that Worker thread if we can avoid it using an I/O thread.

On second thought, the FileReaderSync is different because the caller knows that it will block.
The deserialization is done internally. So that can be treated as unacceptable slowdown.

Comment 58

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #56)
> (In reply to Jan Varga [:janv] from comment #49)
> > However, I'm not sure if BackgroundRequestChild instance should own it. The
> > same over spawning can happen if JS calls get() multiple times and responses
> > contain wasm modules.
> 
> So... I was assuming that in the multiple get case that the I/O thread
> scheduling mechanism was going to block any subsequent get requests from the
> same transaction from running until the request state machine advanced in
> order to satisfy 2.7.1 Transaction Lifetime's

Hm, you are right, but JS can create separate readonly transactions for each get().
Anyway, we maintain a thread (one and only) per physical database. So these readonly transactions won't be processed in parallel, but the spec doesn't prohibit that.

I can imagine a theoretical case when these get() requests come for separate physical databases. That's not very likely scenario, but still I think the request actor shouldn't own the dedicated thread for preprocessing.

Comment 59

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jan Varga [:janv] from comment #58)
> I can imagine a theoretical case when these get() requests come for separate
> physical databases. That's not very likely scenario, but still I think the
> request actor shouldn't own the dedicated thread for preprocessing.

Agreed, implementing a (per-child) service is an even more thorough way to bound resource usage (and serialize the ordering if the I/O thread itself isn't doing so).  I didn't mention it originally because it seemed like significantly more work and out-of-scope for this bug itself :)

Comment 60

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jan Varga [:janv] from comment #58)
> Hm, you are right, but JS can create separate readonly transactions for each
> get().
> Anyway, we maintain a thread (one and only) per physical database. So these
> readonly transactions won't be processed in parallel, but the spec doesn't
> prohibit that.

Right.  To be clear, there are two separate concerns here:
A) That this change causes IndexedDB to potentially violate the response ordering mandated by the spec whenever a transaction contains anything other than a single WASM get().
B) The resource consumption concerns of lots of WASM files being read and processed simultaneously, especially as it could impact non-IndexedDB code when the StreamTransportService is used.

My primary concern in comment 56 is about "A" and it's mainly a side-effect that it improves the "B" scenario under the current implementation.  (And I thought the "B" scenario was less bad because I had incorrectly assumed "A" was already addressed.)

Comment 61

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #60)
> Right.  To be clear, there are two separate concerns here:
> A) That this change causes IndexedDB to potentially violate the response
> ordering mandated by the spec whenever a transaction contains anything other
> than a single WASM get().

Argh, you are right, good catch!
Yeah, we can end up with wrong ordering of responses :(
We will have to fix that.