The default bug view has changed. See this FAQ.

Fail index/cursor operations gracefully if they need preprocessing

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: IndexedDB
P3
normal
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: luke, Assigned: janv)

Tracking

(Blocks: 1 bug, {leave-open})

Trunk
mozilla54
leave-open
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52+ fixed, firefox53 fixed, firefox54 fixed, firefox-esr52 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
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
Duplicate of this bug: 1319693
tracking for 52 as that was requested in bug 1319693
tracking-firefox52: --- → +
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?
(Reporter)

Comment 4

3 months ago
(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)
status-firefox53: --- → affected
status-firefox54: --- → affected
Both Jan and Andrew had opinions, I think. needinfoing Jan (he and Andrew had spoken about an option via email).
Flags: needinfo?(jvarga)
(Assignee)

Comment 8

2 months ago
I'm gonna fix this by adding tests and early checks to the code. So the crashes shouldn't happen anymore.
Flags: needinfo?(jvarga)
(Assignee)

Comment 9

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fc2752603d376a9d0ed1fcafc4ef11ef0230919
(Assignee)

Comment 10

2 months ago
Created attachment 8834409 [details] [diff] [review]
patch
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Attachment #8834409 - Flags: review?(bugmail)
(Assignee)

Comment 11

2 months ago
The actual c++ fix was suggested by :asuth, I created the tests.
Comment on attachment 8834409 [details] [diff] [review]
patch

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

Yay tests!
Attachment #8834409 - Flags: review?(bugmail) → review+
(Assignee)

Updated

2 months ago
Keywords: leave-open

Comment 13

2 months ago
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf62bb072031
(Reporter)

Comment 15

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

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)
(Assignee)

Comment 20

a month ago
Ok, I'll provide branch patches ASAP.
Flags: needinfo?(jvarga)
(Assignee)

Comment 21

a month ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/28886b4c135760a6d8aedf7ab7ec8cae5a0c5821
https://hg.mozilla.org/releases/mozilla-beta/rev/d273229cde3cccea129e2450ec1234e9ac51efc4
Does more need to land here still or can this be closed?
Flags: needinfo?(jvarga)

Comment 23

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/d273229cde3c
status-firefox-esr52: --- → fixed
(Assignee)

Comment 24

a month ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox52: affected → fixed
status-firefox53: affected → fixed
status-firefox54: affected → fixed
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
You need to log in before you can comment on or make changes to this bug.