Closed Bug 1313176 Opened 8 years ago Closed 7 years ago

Fail index/cursor operations gracefully if they need preprocessing


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




Tracking Status
firefox52 + fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed


(Reporter: luke, Assigned: janv)




(1 file)

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.
Priority: -- → P3
tracking for 52 as that was requested in bug 1319693
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?
(Oops, just noticed I forgot to CC relevant people on this initially.)
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.
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(lhenry)
Both Jan and Andrew had opinions, I think. needinfoing Jan (he and Andrew had spoken about an option via email).
Flags: needinfo?(jvarga)
I'm gonna fix this by adding tests and early checks to the code. So the crashes shouldn't happen anymore.
Flags: needinfo?(jvarga)
Attached patch patchSplinter Review
Assignee: nobody → jvarga
Attachment #8834409 - Flags: review?(bugmail)
The actual c++ fix was suggested by :asuth, I created the tests.
Comment on attachment 8834409 [details] [diff] [review]

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

Yay tests!
Attachment #8834409 - Flags: review?(bugmail) → review+
Keywords: leave-open
Pushed by
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
Is this able to be uplifted to 52/53?
Flags: needinfo?(bugmail)
(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.
Flags: needinfo?(bugmail)
Comment on attachment 8834409 [details] [diff] [review]

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.
Attachment #8834409 - Flags: approval-mozilla-beta?
Attachment #8834409 - Flags: approval-mozilla-aurora?
Comment on attachment 8834409 [details] [diff] [review]

fix indexeddb issue affecting devtools/wasm, aurora53+, beta52+
Attachment #8834409 - Flags: approval-mozilla-beta?
Attachment #8834409 - Flags: approval-mozilla-beta+
Attachment #8834409 - Flags: approval-mozilla-aurora?
Attachment #8834409 - Flags: approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(jvarga)
Ok, I'll provide branch patches ASAP.
Flags: needinfo?(jvarga)
Does more need to land here still or can this be closed?
Flags: needinfo?(jvarga)
(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.
Closed: 7 years ago
Flags: needinfo?(jvarga)
Resolution: --- → FIXED
Summary: support index/cursor operations on stored WebAssembly modules → Fail index/cursor operations gracefully if they need preprocessing
Target Milestone: --- → mozilla54
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.