1313176 - Fail index/cursor operations gracefully if they need preprocessing

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P3)

Description

[Luke Wagner [:luke]]

According to bug 1311466 comment 12: objectStore.getAll(), index.get(), index.getAll() and cursors

I also noticed this will fix an assertion failure in the hidden-by-default devtools Storage pane when attempting to explore an object store containing a wasm module.

Comment 2

[Julien Cristau [:jcristau]]

tracking for 52 as that was requested in bug 1319693

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Gentle ping about this: as stated in bug 1319693 comment 0, it's an easy way to crash the browser (especially since emscripten will try to cache wasm modules by default, iiuc): store a wasm module in idb, open devtools and look at the wasm module in idb.

Implementing the full feature might take some time, but maybe we can mitigate the crash in the time being in 52 (target for wasm release) by just failing softly?

Comment 4

[Luke Wagner [:luke]]

(Oops, just noticed I forgot to CC relevant people on this initially.)

Comment 5

[Andrew Overholt [:overholt]]

Liz, FYI, there's a chance the fix that lands here is going to be asked to be uplifted to 52 due to wasm shipping in 52.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, sounds good, can you help find someone to work on it? I notice it's currently P3 without anyone assigned. We'll be building beta 4 next Monday, so still pretty early in the 52 cycle.

Comment 7

[Andrew Overholt [:overholt]]

Both Jan and Andrew had opinions, I think. needinfoing Jan (he and Andrew had spoken about an option via email).

Comment 8

[Jan Varga [:janv]]

I'm gonna fix this by adding tests and early checks to the code. So the crashes shouldn't happen anymore.

Comment 9

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fc2752603d376a9d0ed1fcafc4ef11ef0230919

Comment 10

[Jan Varga [:janv]]

Attachment: patch

Comment 11

[Jan Varga [:janv]]

The actual c++ fix was suggested by :asuth, I created the tests.

Comment 12

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

Comment on attachment 8834409 [details] [diff] [review]
patch

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

Yay tests!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf62bb072031
Fail index/cursor operations gracefully if they need preprocessing. This fixes the crash when devtools try to inspect idb databases with WebAssembly modules). It's a temporary fix until we implement preprocessing for indexes and cursors. A bunch of new tests is added to test indexes and cursors with wasm; r=asuth

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf62bb072031

Comment 15

[Luke Wagner [:luke]]

Is this able to be uplifted to 52/53?

Comment 16

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

(In reply to Luke Wagner [:luke] from comment #15)
> Is this able to be uplifted to 52/53?

Yes, but in my experiences from uplifting my bug 1319531 patch, slight changes will be required for the tests if they're going along with the uplift.  (They probably should.)  Specifically, I think:
- s/function* testSteps()/function testSteps/ to get old generator semantics because the IDB test driver assumes old-style generators on 52.
- s/finishTest();/finishTest();\nyield undefined;/ because of how the test-driver works again; otherwise StopIteration gets thrown or something and fails the test.

In the interest of time I'll do the flag requests now.

Comment 17

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

Comment on attachment 8834409 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1311466 provided the ability to store WebAssembly modules in IndexedDB.

[User impact if declined]:
Release asserts may fire when retrieving stored WebAssembly blobs via index/cursor operations.  In particular, as comment 3 indicates, this happens for devtools when using its IndexedDB inspector.

[Is this code covered by automated tests?]:
Yes, the patch includes :janv's trademark comprehensive test coverage.

[Has the fix been verified in Nightly?]:
Yes, baking since Feb 8th.

[Needs manual test from QE? If yes, steps to reproduce]: 
No, the automated test coverage is thorough.

[List of other uplifts needed for the feature/fix]:
No other uplifts are needed, but for beta we'll probably need to adjust the tests slightly to compensate for test infrastructure changes.  (See comment 16.)

[Is the change risky?]:
No; the non-test changes are to error out early before anything dangerous can be done.  (Specifically, we no longer have to worry about the rest of the IndexedDB code having its invariants violated.  When those invariants get violated, we either release-assert or violate correctness guarantees in our results provided to content.  The error is far correct and safer.)

[Why is the change risky/not risky?]:
The minimal change and extensive tests.

[String changes made/needed]:
No string changes needed.


NOTE: Beta likely needs custom uplift patch fixes; even though it might apply cleanly, I do expect the new tests to fail with the IDB test infra on 52 without the comment 16 fixes.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8834409 [details] [diff] [review]
patch

fix indexeddb issue affecting devtools/wasm, aurora53+, beta52+

Comment 19

[Carsten Book [:Tomcat]]

needs rebasing for aurora

Comment 20

[Jan Varga [:janv]]

Ok, I'll provide branch patches ASAP.

Comment 21

[Jan Varga [:janv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/28886b4c135760a6d8aedf7ab7ec8cae5a0c5821
https://hg.mozilla.org/releases/mozilla-beta/rev/d273229cde3cccea129e2450ec1234e9ac51efc4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Does more need to land here still or can this be closed?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d273229cde3c

Comment 24

[Jan Varga [:janv]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Does more need to land here still or can this be closed?

This bug morphed into a quick crash fix. I filed a new bug 1341271 for real fix.

Comment 25

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.