Last Comment Bug 1313176 - Fail index/cursor operations gracefully if they need preprocessing
: Fail index/cursor operations gracefully if they need preprocessing
Status: RESOLVED FIXED
: leave-open
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: Trunk
: Unspecified Unspecified
P3 normal with 1 vote (vote)
: mozilla54
Assigned To: Jan Varga [:janv]
:
: Hsin-Yi Tsai [:hsinyi]
Mentors:
: 1319693 (view as bug list)
Depends on: 1311466
Blocks: 1276029
  Show dependency treegraph
 
Reported: 2016-10-26 13:04 PDT by Luke Wagner [:luke]
Modified: 2017-02-21 05:17 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed
fixed
fixed


Attachments
patch (23.23 KB, patch)
2017-02-07 06:43 PST, Jan Varga [:janv]
bugmail: review+
jcristau: approval‑mozilla‑aurora+
jcristau: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Luke Wagner [:luke] 2016-10-26 13:04:02 PDT
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 1 User image Andrew Overholt [:overholt] 2016-11-29 12:48:13 PST
*** Bug 1319693 has been marked as a duplicate of this bug. ***
Comment 2 User image Julien Cristau [:jcristau] 2016-11-30 02:34:02 PST
tracking for 52 as that was requested in bug 1319693
Comment 3 User image Benjamin Bouvier [:bbouvier] 2017-01-05 01:55:12 PST
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 User image Luke Wagner [:luke] 2017-01-06 11:59:00 PST
(Oops, just noticed I forgot to CC relevant people on this initially.)
Comment 5 User image Andrew Overholt [:overholt] 2017-02-03 10:21:03 PST
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 User image Liz Henry (:lizzard) (needinfo? me) 2017-02-03 11:11:06 PST
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 User image Andrew Overholt [:overholt] 2017-02-03 12:27:19 PST
Both Jan and Andrew had opinions, I think. needinfoing Jan (he and Andrew had spoken about an option via email).
Comment 8 User image Jan Varga [:janv] 2017-02-06 05:05:15 PST
I'm gonna fix this by adding tests and early checks to the code. So the crashes shouldn't happen anymore.
Comment 10 User image Jan Varga [:janv] 2017-02-07 06:43:27 PST
Created attachment 8834409 [details] [diff] [review]
patch
Comment 11 User image Jan Varga [:janv] 2017-02-07 07:32:57 PST
The actual c++ fix was suggested by :asuth, I created the tests.
Comment 12 User image Andrew Sutherland [:asuth] 2017-02-07 23:21:07 PST
Comment on attachment 8834409 [details] [diff] [review]
patch

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

Yay tests!
Comment 13 User image Pulsebot 2017-02-08 02:12:35 PST
Pushed by jvarga@mozilla.com:
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 User image Wes Kocher (:KWierso) 2017-02-08 16:11:52 PST
https://hg.mozilla.org/mozilla-central/rev/cf62bb072031
Comment 15 User image Luke Wagner [:luke] 2017-02-13 12:19:03 PST
Is this able to be uplifted to 52/53?
Comment 16 User image Andrew Sutherland [:asuth] 2017-02-13 12:28:11 PST
(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 User image Andrew Sutherland [:asuth] 2017-02-13 12:39:50 PST
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 User image Julien Cristau [:jcristau] 2017-02-14 02:58:03 PST
Comment on attachment 8834409 [details] [diff] [review]
patch

fix indexeddb issue affecting devtools/wasm, aurora53+, beta52+
Comment 19 User image Carsten Book [:Tomcat] 2017-02-14 06:31:46 PST
needs rebasing for aurora
Comment 20 User image Jan Varga [:janv] 2017-02-14 06:32:38 PST
Ok, I'll provide branch patches ASAP.
Comment 22 User image Ryan VanderMeulen [:RyanVM] 2017-02-16 06:29:46 PST
Does more need to land here still or can this be closed?
Comment 23 User image Ryan VanderMeulen [:RyanVM] 2017-02-17 06:22:32 PST
https://hg.mozilla.org/releases/mozilla-esr52/rev/d273229cde3c
Comment 24 User image Jan Varga [:janv] 2017-02-21 05:17:03 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.