1469395 - Remove WebAssembly.Module IndexedDB serialization support

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Luke Wagner [:luke]]

This became controversial right before the wasm MVP release:
  https://github.com/WebAssembly/design/issues/972
and consequently hasn't been included in the official MVP spec nor shipped in Chrome/Safari.

After a lot more discussion, experience and learning how alternatives would work, the proposal is to drop the support entirely:
  https://github.com/WebAssembly/spec/issues/821
and leverage the HTTP Cache + DOM Cache API instead.

Comment 1

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

So the question is what to do about databases that already have this data stored and when.  We need to look at the file_ids column in the object_data table to see the markers for the '/' and '\\' type markers to tell whether we have wasm modules stored in the database.  (We can also deserialize the structured clone payload of course, but that's more costly.)

For "What" our options are:
- Wipe the origin entirely because the contract is that if we're evicting data, we evict all of it.
- Wipe just the database (or pretend like it doesn't exist).
- Just hand out null pointers where previously we would have handed out a WASM module.  Do wipe the underlying blobs.  The rationale would be WASM modules were experimental and in flux and getting a null shouldn't be surprising.

For "When" our options are:
- QuotaManager minor schema upgrade that walks all origins and looks in the databases and does the "what".
- IndexedDB minor schema upgrade that does "what" the next time the database is opened.

:luke, what are your thoughts on the "what"?  I guess the main question for me is whether we expect the IDB databases to be/become abandoned caching artifacts that no one will proactively clean up, or if there was other useful data stored in them.  Wiping the databases (but not the origin) on Firefox upgrade sounds right for the former, and returning null sounds right for the latter.

Comment 2

[Luke Wagner [:luke]]

Given the very low usage and cache-iness of what usage there is, I think the simplest option, which I assume is "wipe the origin entirely" would work just fine in practice.  For default storage, this is as-if the origin was simply evicted.  The permanent storage case I assume is *ultra* rare (based on the unanimous feedback I heard from gamedevs that they don't want to have to prompt for storage), so I think it's ok to wipe in that case too.

Comment 3

[Luke Wagner [:luke]]

This has been accepted by the wasm CG/WG, so I'm ready to fire ze misiles.  I can pretty trivially write patches to delete the two "if (JS::IsWasmModuleObject(aObj)) {...}" in IDBObjectStore.cpp and then delete the IDB tests for these and this will easily achieve standards conformance, but I'm not sure what the right patch is to handle IDB modules that have already been stored (assuming comment 2 origin-clearing semantics are fine).  Please advise?

Comment 4

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

Given that :ttung is back (yay!!) and the wiping is potentially going to be a bit involved, it might be worth considering a simpler stop-gap like you propose.  My knee-jerk concern about deleting those two checks is that it would be weird if you can get() a wasm module out but can't put() it in.  We could make it so that BackgroundRequestChild::HandlePreprocess would mark the database as wasm-supporting so put(wasm) becomes legal and roundtripping works, but that seems silly and confusing.

So, I would say, yeah... maybe a patch deleting those lines is a good first step.  This would leave content in the situation:
- They can get a wasm module out.
- They can delete() the rows that contain wasm modules.
- They can put() overwrite the rows that contain wasm modules.
- They can delete the database.

And if the content deletes the database itself, the origin won't be subject to wiping later on when we implement the spin-off wiping.

Comment 5

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

Some tests would of course need to be updated too.

Comment 6

[Luke Wagner [:luke]]

How hard is the wiping?  Do we have any existing paths where if things look borked, we wipe the origin?

Comment 7

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

(In reply to Luke Wagner [:luke] from comment #6)
> How hard is the wiping?  Do we have any existing paths where if things look
> borked, we wipe the origin?

No, that's on our to-do list to solve QuotaManager breakage.  If we had that, this would be a bit easier.

What I'm envisioning for origin wiping is, implementation-wise:
- Bump the QuotaManager minor schema version.
- At QM initialization time, if the (minor) schema version is not the "wipe" version, trigger a QM upgrade sweep that invokes each QuotaClient for each origin to determine whether we should wipe the origin or not.  If the client says yes, we short circuit processing that origin and wipe it.  Thus far we've added version-specific upgrade methods that are no-op's if the method is not overridden.
- Implement the upgrade method for IDB that opens each IDB database and runs a query against the file_ids column, indicating a wipe should happen if we find any WASM '/' or '\\' markers.
- Bump the minor schema version after the wipe completes.

We'd probably want to pay special attention to error-handling in the event that anti-virus decides to look at the files when we're done with them.  I think we have some directory removal hacks on Windows already that wait a few seconds, but we'd want comments that explain what the expected failures are there and how they manifest.

In particular, I'd want to make sure we don't need to add some type of "tombstone" indicator that expresses "this directory was intended to be wiped, but we ran into problems trying to delete it at the time and you should delete this at next startup".  I'm not sure that's something we'd need long-term, but right now QM and IDB can be a bit brittle at startup in general, and it would suck for us to fail to wipe out a directory entirely and the partial directory deletion then permanently breaking quota manager at startup, etc.  Our longer term strategy is that errors on initializing an origin result in erasure.  It's possible we could just roll that into this change as well if we thought the tombstone might be necessary.


Test-wise we'd want a test that verifies an existing IDB database gets wiped.  It'd also be nice to sanity-check that existing upgrade logic/tests aren't going to have their tested functionality shadowed by this wiping logic.  (That is, if a previous upgrade logic would wipe something, and our logic here would also wipe it, we want to make sure we understand what's happening so we don't accidentally break the upgrade functionality in the future.  Practically speaking, I think that would just mean that any tests that only check for whether the IDB opens and has data might also need to check whether the origin root directory still exists or not.)  The sanity checking could be part of the review.

Comment 8

[Luke Wagner [:luke]]

Interesting, and glad to see this sort of general facility is already on the todo list.  Is there a bug for it?  Maybe we can break this bug into two: one that we can land immediately that prevents new storage of wasm modules, and a second that blocks on the QM-clearing functionality.

Comment 9

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

Agreed on splitting this bug.  I would say do the minimal changes here if you're so inclined and we'll create a spin-off for the follow-up.  Bug 1426119 is the meta-bug for Quota Manager and IndexedDB resiliency.  Expect to see new, specific bugs filed under that meta bug during the next several months in addition to existing bugs being fixed.

Comment 10

[Luke Wagner [:luke]]

Attachment: disable-serialization

This minimal patch disables serialization.  It also disables the deserialization of machine code (always recompiling from bytecode and ignoring the compiled code file).  This will free up the next patch to simplify the SM-side code.

Most of the tests were removed except two token tests to:
 - check that storing a wasm module throws
 - (re-purposing a test that checked for build-id recompilation) check that reading an already-saved module still works

Comment 11

[Luke Wagner [:luke]]

Attachment: disable-serialization

A little more IDB dead-code-removal (found by dead code warnings on try).

Comment 12

[Luke Wagner [:luke]]

Attachment: also-remove-wpts

IIUC, these wpts were added haphazardly a while ago and will be subsumed by the proper tests Ms2ger is writing.  Fine to just remove these Ms2ger?

Comment 13

[Luke Wagner [:luke]]

Attachment: rm-listener-notification

Nice little simplification allowed by removing IDB support.

I started another patch to remove bytecode from wasm::Module, but we still need it in Module::instantiate() for (other than debugging) initSegments().  IIRC, Julian's work in bug 1471500 will add support for saving just the data/elem segment subset of the bytecode which I think could be expanded to use for active segment init too.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8996901 [details] [diff] [review]
also-remove-wpts

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

Makes sense. It would be nice to have a negative test for this as well; let me know if I should write it.

Comment 15

[Lars T Hansen [:lth]]

Comment on attachment 8996892 [details] [diff] [review]
disable-serialization

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

OK.  Apart from the code in js/ I don't feel qualified to comment but it looks all right.

Comment 16

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

Comment on attachment 8996892 [details] [diff] [review]
disable-serialization

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

:janv is going to be back on Monday (which is why he hasn't chimed in yet) and in addition to wanting his sign-off on this, I'm a little backlogged on some other review work, so transferring the review.

I think the one thing that should further change is that CheckWasmModule should be fully removed.  It only existed for bytecode recompilation purposes due to logistical issues surrounding IDB transaction semantics.  If you're removing recompilation and the cached pre-compiled file, then the preprocess mechanism that ensures the module object is already available at structured clone deserialization-time is already sufficient.

Comment 17

[Luke Wagner [:luke]]

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #14)
> Makes sense. It would be nice to have a negative test for this as well; let
> me know if I should write it.

I have an xpc test that does this, but you're right a wpt test should cover this as well, for good measure.  I didn't write it since I assumed it might fight into an organization scheme you already have and since Dan already suggested you write one :) (https://github.com/WebAssembly/spec/pull/711#issuecomment-400510215)

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Oops. In that case, it's on my todo list :)

Comment 19

[Luke Wagner [:luke]]

Attachment: disable-serialization

(Updated with CheckWasmModule() removed, leaving r?janv, carrying forward r+ from asuth and lth.)

Comment 20

[Lars T Hansen [:lth]]

Comment on attachment 8996902 [details] [diff] [review]
rm-listener-notification

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

Seems reasonable enough.

Comment 21

[Jan Varga [:janv]]

Comment on attachment 8997183 [details] [diff] [review]
disable-serialization

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

::: dom/indexedDB/ActorsChild.cpp
@@ -3528,5 @@
>    }
>  
>    RefPtr<JS::WasmModule> module =
>      JS::DeserializeWasmModule(mCurrentBytecodeFileDesc,
> -                              mCurrentCompiledFileDesc,

mCurrentCompiledFileDesc is now unused, actually the parent doesn't need to send the compiled blob at all. Do you plan to fix that in a separate bug ?

SerializeStructuredCloneFiles() in ActorsParent.cpp is the place where an IPC blob is created for stored compiled wasm module.

::: dom/indexedDB/IDBObjectStore.cpp
@@ -646,5 @@
>        return true;
>      }
>    }
>  
> -  if (JS::IsWasmModuleObject(aObj)) {

Should we add a warning for this ?
IDB_WARNING("Storing of WASM modules is not supported anymore!")
Up to you...

IDB_WARNING prints to JS console, so developers can see why they now get the data clone error.

@@ -767,5 @@
>        return true;
>      }
>    }
>  
> -  if (JS::IsWasmModuleObject(aObj)) {

If you decide to add the warning, it should be added here too.

::: dom/indexedDB/test/unit/test_wasm_put_get_values.js
@@ +52,3 @@
>  
> +  // storing a wasm module in IDB should now fail
> +  var failed = false;

Nit: s/var/let

@@ +52,5 @@
>  
> +  // storing a wasm module in IDB should now fail
> +  var failed = false;
> +  try {
> +      objectStore.add(wasmData.value, wasmData.key);

Nit: 2 spaces for indentation

::: dom/indexedDB/test/unit/test_wasm_recompile.js
@@ +98,2 @@
>  
> +  ok(compareBuffers(newCompiledBuffer, compiledBuffer), "Blobs differ");

Nit: "Blobs don't differ" or something like that

Comment 22

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #21)
> > -  if (JS::IsWasmModuleObject(aObj)) {
> 
> Should we add a warning for this ?
> IDB_WARNING("Storing of WASM modules is not supported anymore!")
> Up to you...

Well, it'll throw, and it has always thrown in Chrome, and we'll be updating the MDN page (which is probably the only place it's really documented), so my thinking is it's not worth it.

Comment 23

[Luke Wagner [:luke]]

Attachment: address-review-comments

Regarding your first review comment, is this what you were thinking?  It seems to pass tests.  Ultimately, I think the whole preprocess mechanism could be removed/simplified, but I don't think I'm qualified to do that.  I'm happy to file a follow-up bug that depends on this, though.

Comment 24

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #23)
> Created attachment 8997999 [details] [diff] [review]
> address-review-comments
> 
> Regarding your first review comment, is this what you were thinking?  It
> seems to pass tests.

Yeah, but I only went through the updated patch very quickly, will take a closer look.

> Ultimately, I think the whole preprocess mechanism
> could be removed/simplified, but I don't think I'm qualified to do that. 
> I'm happy to file a follow-up bug that depends on this, though.

The preprocess mechanism must stay for now, since bytecode blobs still need to be preprocessed.

Comment 25

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #24)
> > Ultimately, I think the whole preprocess mechanism
> > could be removed/simplified, but I don't think I'm qualified to do that. 
> > I'm happy to file a follow-up bug that depends on this, though.
> 
> The preprocess mechanism must stay for now, since bytecode blobs still need
> to be preprocessed.

IIRC, it wasn't a problem that bytecode blobs needed to be compiled into JS::WasmModules, with tiering, that operation is a relatively cheap synchronous operation that can happen on any thread.  I thought the problem solved by preprocessing was the ability to re-store modules if they needed to be recompiled.

Comment 26

[Jan Varga [:janv]]

(In reply to Luke Wagner [:luke] from comment #25)
> (In reply to Jan Varga [:janv] from comment #24)
> > > Ultimately, I think the whole preprocess mechanism
> > > could be removed/simplified, but I don't think I'm qualified to do that. 
> > > I'm happy to file a follow-up bug that depends on this, though.
> > 
> > The preprocess mechanism must stay for now, since bytecode blobs still need
> > to be preprocessed.
> 
> IIRC, it wasn't a problem that bytecode blobs needed to be compiled into
> JS::WasmModules, with tiering, that operation is a relatively cheap
> synchronous operation that can happen on any thread.  I thought the problem
> solved by preprocessing was the ability to re-store modules if they needed
> to be recompiled.

... but we need a file descriptor for calling JS::DeserializeWasmModule.
The parent process sends a blob protocol, we can create a stream from that while
we are still on the main thread in content process. Now if we want to get a file
descriptor from the stream, the stream must open the file which means we have to
do I/O and doing I/O on the main thread is usually not a good idea.
The preprocessing is a way how to avoid I/O on the main thread. AFAIK, it doesn't
do any re-storing of modules. That is done purely in the main process and your
patch removes that code.

Comment 27

[Jan Varga [:janv]]

Comment on attachment 8997999 [details] [diff] [review]
address-review-comments

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

This has issues ...
./mach xpcshell-test dom/indexedDB/test/unit/test_wasm_recompile.js doesn't pass
Let me investigate it.

Comment 28

[Luke Wagner [:luke]]

Oh, no sorry!  I had that issue because of a thinko, then fixed it, then forgot to qref before posting the patch.

Comment 29

[Luke Wagner [:luke]]

Attachment: address-review-comments

Sorry for the bother!

Comment 30

[Jan Varga [:janv]]

Comment on attachment 8998276 [details] [diff] [review]
address-review-comments

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

Ok, looks good, but I have an additional patch that makes it more obvious that the compiled file is not used anymore.

::: dom/indexedDB/ActorsChild.cpp
@@ +1549,5 @@
>    void
>    RunOnOwningThread();
>  
>    void
> +  ProcessCurrentStreams();

This should be ProcessCurrentStream

Comment 31

[Jan Varga [:janv]]

Attachment: additional patch

Comment 32

[Jan Varga [:janv]]

Feel free to merge disable-serialization, address-review-comments and additional patch.

Comment 33

[Luke Wagner [:luke]]

(In reply to Jan Varga [:janv] from comment #32)
> Feel free to merge disable-serialization, address-review-comments and
> additional patch.

Thanks!  Will do.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f4e67b3ae1
Remove support for storing WebAssembly.Modules in IDB (r=lth,janv,Ms2ger)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed9cbebf30e
Baldr: remove dead JS::WasmModule::notifyWhenCompilationComplete() support (r=lth)

Comment 35

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12357 for changes under testing/web-platform/tests

Comment 36

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b0f4e67b3ae1
https://hg.mozilla.org/mozilla-central/rev/2ed9cbebf30e

Comment 38

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 39

[Chris Mills [:cmills]]

I've documented this:

Caching article removed from MDN:
https://developer.mozilla.org/en-US/docs/WebAssembly/Caching_modules

Example code removed from the webassembly-examples repo:
https://github.com/mdn/webassembly-examples/commit/48f1cbfaf095c2a251c46e15b21ad8e4e3b1a866

Caching article removed from the article sidebar:
https://github.com/mdn/kumascript/pull/760

References to this article and code demos removed from all WebAssembly pages:
https://developer.mozilla.org/en-US/docs/WebAssembly

Release note added to the Fx63 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#JavaScript

Let me know if there's anything else needed on the docs side. Thanks!

Comment 40

[Luke Wagner [:luke]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #39)
Thanks!

> Caching article removed from MDN:
> https://developer.mozilla.org/en-US/docs/WebAssembly/Caching_modules

Do you think perhaps the URL could be kept live with just a scare-box "This experimental feature was removed" and a link to to https://github.com/WebAssembly/spec/issues/821?

> Release note added to the Fx63 rel notes:
> https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/
> 63#JavaScript

Could you put "Experimental" in front of "WebAssembly Module IndexedDB serialization support"?

Comment 41

[Kohei Yoshino [:kohei]]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/indexeddb-serialization-support-has-been-removed-from-webassembly-module/

Comment 42

[Chris Mills [:cmills]]

(In reply to Luke Wagner [:luke] from comment #40)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #39)
> Thanks!
> 
> > Caching article removed from MDN:
> > https://developer.mozilla.org/en-US/docs/WebAssembly/Caching_modules
> 
> Do you think perhaps the URL could be kept live with just a scare-box "This
> experimental feature was removed" and a link to to
> https://github.com/WebAssembly/spec/issues/821?
> 
> > Release note added to the Fx63 rel notes:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/
> > 63#JavaScript
> 
> Could you put "Experimental" in front of "WebAssembly Module IndexedDB
> serialization support"?

OK, done both of these.

Comment 43

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Luke Wagner [:luke] from comment #17)
> (In reply to :Ms2ger (⌚ UTC+1/+2) from comment #14)
> > Makes sense. It would be nice to have a negative test for this as well; let
> > me know if I should write it.
> 
> I have an xpc test that does this, but you're right a wpt test should cover
> this as well, for good measure.  I didn't write it since I assumed it might
> fight into an organization scheme you already have and since Dan already
> suggested you write one :)
> (https://github.com/WebAssembly/spec/pull/711#issuecomment-400510215)

I finally got around to this, and found out Google was kind enough to do it first in <https://github.com/web-platform-tests/wpt/pull/12488>. Yay for cooperation!