Closed Bug 1422335 Opened 2 years ago Closed 2 years ago

Cannot upgrade IndexedDB schema with CryptoKey: UnknownError

Categories

(Core :: DOM: IndexedDB, defect, P2)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: stefan, Assigned: baku)

References

Details

(Keywords: csectype-dos, sec-low)

Attachments

(5 files, 4 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

We have an IndexedDB database that contains WebCrypto crypto keys, e.g., AES-CBC, AES-KW and so on. During schema upgrade, we create an additional index over a field that does not contain CryptoKeys but strings or numbers.


Actual results:

In Firefox 57.0.1 we get the following error: "UnknownError: The operation failed for reasons unrelated to the database itself and not covered by any other error code."
Firefox 59.0a1 (2017-12-01) simply crashes printing twice the following exception: "** Unknown exception behavior: -2147483647".


Expected results:

The migration should have succeeded and the index should have been created. This is possibly harmful since one could send a link and crash a user's browser, e.g., this link: https://jsfiddle.net/sechel/9esrng0f/
Sample crash report:

https://crash-stats.mozilla.com/report/index/1f632f7b-92b6-40d2-b65d-ed5db0171201

The parent process crashes, which seems kinda serious. :btseng or :janv, can you take a look?
Group: firefox-core-security → core-security
Component: Untriaged → DOM: IndexedDB
Flags: needinfo?(jvarga)
Flags: needinfo?(btseng)
Product: Firefox → Core
Group: core-security → dom-core-security
This may even be related to the issue we had earlier with CryptoKeys and migration of Firefox versions: https://bugzilla.mozilla.org/show_bug.cgi?id=1348279
Andrew, could your patch for bug 1410106 have affected this since it touched IndexedDB?
Flags: needinfo?(bugmail)
(In reply to Al Billings [:abillings] from comment #3)
> Andrew, could your patch for bug 1410106 have affected this since it touched
> IndexedDB?

No, that was principal-related gating of access to IDB, very far from the JS engine or structured clone issues happening here.  (And technically that was :baku's fix, I just wrote tests and really wanted to see it land. :)

Because this crash does involve thread-local storage, it's worth mentioning that in 1421277 :bkelly saw problems that seemed like thread-local-storage could be having problems.  There was some confusion about processors in the initial analysis there, but it's worth being aware of.  But Stefan's theory about the existing problem with structured clone keys seem more relevant.

Stefan, do you have an IDB database you could safely share and attach to the bug?  If not, if you could characterize the object that's being put into IDB and where the crypto-keys are in it, that would be useful.  That is, are they very simple {id, key} objects like in https://bugzilla.mozilla.org/show_bug.cgi?id=1348279#c1, or are they more complicated, with multiple keys in an object and possibly Blob instances, etc.?
Flags: needinfo?(bugmail) → needinfo?(stefan)
The object is very simple, i.e., { id: 1, value: 1, key }. Once the object is put to IDB, adding an index over the value field leads to the error. Generally speaking, adding a new index to a table containing at least one object containing at least one WebCrypto key leads to the error.

Stefan's jsFiddle at the top of this report (https://bugzilla.mozilla.org/show_bug.cgi?id=1422335#c0) contains this easy test case. Open in Firefox stable to see the error without crashing your browser.
Just as Ben said, its very simple. In https://jsfiddle.net/sechel/9esrng0f/ I create a CryptoKey with the command

  crypto.subtle.generateKey({name: 'AES-CBC', length: 128}, true, ['encrypt', 'decrypt']);

This key is then put into IndexedDB (version 1) contained in an object {id: 1, value: 1, key}. Then I close the db and open it again with version 2. In the upgrade handler I add an extra index over the `value` keyPath. As a result I get an error in Firefox 57.0.1 and a crash in Firefox Nightly. In the Fiddle there are two versions, the first one uses the Promise based IndexedDB library Dexie. I created a second one using the callback based native API. There is a different Error in both cases but both versions crash Firefox Nightly.
Flags: needinfo?(stefan)
Update what I've found so far:
1. The crash of loading https://jsfiddle.net/sechel/9esrng0f/ is 100% reproducible.
2. In debug build, we will hit an assertion eariler instead of QI an nullptr:
   https://searchfox.org/mozilla-central/source/js/xpconnect/wrappers/WrapperFactory.cpp#714-716
3. According to the call stack, the crash happens when creating an index which trying to read a WEBCRYPTO_KEY.
4. The problem is that why the JS context in the argument is not able to convert into an nsIGlobalObject.

Here is the call stack in debug build for reference:
Thread 78 "IndexedDB #2" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffa69ff700 (LWP 4438)]
0x00007fffdfe29b79 in xpc::NativeGlobal (obj=0x7fffa318c060)
    at /home/bevis/Projects/Build/gecko/src/js/xpconnect/wrappers/WrapperFactory.cpp:714
714         MOZ_ASSERT((GetObjectClass(obj)->flags & (JSCLASS_PRIVATE_IS_NSISUPPORTS |
(gdb)
#0  0x00007fffdfe29b79 in xpc::NativeGlobal (obj=0x7fffa318c060)
    at /home/bevis/Projects/Build/gecko/src/js/xpconnect/wrappers/WrapperFactory.cpp:714
#1  0x00007fffe0c717e4 in mozilla::dom::StructuredCloneHolder::ReadFullySerializableObjects (
    aCx=0x7fffa5afe000, aReader=0x7fffa69fb618, aTag=4294934538)
    at /home/bevis/Projects/Build/gecko/src/dom/base/StructuredCloneHolder.cpp:362
#2  0x00007fffe2d37832 in mozilla::dom::(anonymous namespace)::CommonStructuredCloneReadCallback<mozilla::dom::(anonymous namespace)::IndexDeserializationHelper> (aCx=0x7fffa5afe000, aReader=0x7fffa69fb618, 
    aTag=4294934538, aData=0, aClosure=0x7fffa69fc648)
    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/IDBObjectStore.cpp:1257
#3  0x00007fffe6cde952 in JSStructuredCloneReader::startRead (this=0x7fffa69fb618, vp=...)
    at /home/bevis/Projects/Build/gecko/src/js/src/vm/StructuredClone.cpp:2320
#4  0x00007fffe6cd74f1 in JSStructuredCloneReader::read (this=0x7fffa69fb618, vp=...)
    at /home/bevis/Projects/Build/gecko/src/js/src/vm/StructuredClone.cpp:2623
#5  0x00007fffe6cd706f in ReadStructuredClone (cx=0x7fffa5afe000, data=..., 
    scope=JS::StructuredCloneScope::SameProcessSameThread, vp=..., 
    cb=0x7fffecdc2f88 <mozilla::dom::IDBObjectStore::DeserializeIndexValue(JSContext*, mozilla::dom::indexedDB::StructuredCloneReadInfo&, JS::MutableHandle<JS::Value>)::callbacks>, cbClosure=0x7fffa69fc648)
    at /home/bevis/Projects/Build/gecko/src/js/src/vm/StructuredClone.cpp:634
#6  0x00007fffe6ce09a9 in JS_ReadStructuredClone (cx=0x7fffa5afe000, buf=..., version=8, 
    scope=JS::StructuredCloneScope::SameProcessSameThread, vp=..., 
    optionalCallbacks=0x7fffecdc2f88 <mozilla::dom::IDBObjectStore::DeserializeIndexValue(JSContext*, mozilla::dom::indexedDB::StructuredCloneReadInfo&, JS::MutableHandle<JS::Value>)::callbacks>, 
    closure=0x7fffa69fc648) at /home/bevis/Projects/Build/gecko/src/js/src/vm/StructuredClone.cpp:2664
#7  0x00007fffe2d37209 in mozilla::dom::IDBObjectStore::DeserializeIndexValue (aCx=0x7fffa5afe000, 
    aCloneReadInfo=..., aValue=...)
    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/IDBObjectStore.cpp:1487
#8  0x00007fffe2cc0448 in mozilla::dom::indexedDB::(anonymous namespace)::CreateIndexOp::UpdateIndexDataValuesFunction::OnFunctionCall (this=0x7fffa5806850, aValues=0x7fffd0d0f2e0, _retval=0x7fffa69fc838)
    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsParent.cpp:25238
#9  0x00007fffdff71056 in mozilla::storage::(anonymous namespace)::basicFunctionHelper (
    aCtx=0x7fffa56d9ae0, aArgc=4, aArgv=0x7fffa56d9b10)
    at /home/bevis/Projects/Build/gecko/src/storage/mozStorageConnection.cpp:249
#10 0x00007ffff6640224 in sqlite3VdbeExec (p=0x7fffa56c9460)
    at /home/bevis/Projects/Build/gecko/src/db/sqlite3/src/sqlite3.c:86154
#11 0x00007ffff65f19f9 in sqlite3Step (p=0x7fffa56c9460)
    at /home/bevis/Projects/Build/gecko/src/db/sqlite3/src/sqlite3.c:77535
#12 0x00007ffff65f153d in sqlite3_step (pStmt=0x7fffa56c9460)
    at /home/bevis/Projects/Build/gecko/src/db/sqlite3/src/sqlite3.c:77598
#13 0x00007fffdff6cd28 in mozilla::storage::Connection::stepStatement (this=0x7fffa58de460, 
    aNativeConnection=0x7fffa553e400, aStatement=0x7fffa56c9460)
    at /home/bevis/Projects/Build/gecko/src/storage/mozStorageConnection.cpp:1180
#14 0x00007fffdff98f10 in mozilla::storage::Statement::ExecuteStep (this=0x7fffa6fa8b80, 
    _moreResults=0x7fffa69fd6ef)
    at /home/bevis/Projects/Build/gecko/src/storage/mozStorageStatement.cpp:583
#15 0x00007fffdff98c88 in mozilla::storage::Statement::Execute (this=0x7fffa6fa8b80)
    at /home/bevis/Projects/Build/gecko/src/storage/mozStorageStatement.cpp:549
#16 0x00007fffe2cbf420 in mozilla::dom::indexedDB::(anonymous namespace)::CreateIndexOp::InsertDataFromObjectStoreInternal (this=0x7fffa58df120, aConnection=0x7fffa575a890)
    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsParent.cpp:24911
#17 0x00007fffe2cbeb19 in mozilla::dom::indexedDB::(anonymous namespace)::CreateIndexOp::InsertDataFromObjectStore (this=0x7fffa58df120, aConnection=0x7fffa575a890)
    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsParent.cpp:24871
#18 0x00007fffe2cbe5ff in mozilla::dom::indexedDB::(anonymous namespace)::CreateIndexOp::DoDatabaseWork
    (this=0x7fffa58df120, aConnection=0x7fffa575a890)
    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsParent.cpp:25075
#19 0x00007fffe2c794b6 in mozilla::dom::indexedDB::(anonymous namespace)::TransactionDatabaseOperationBase::RunOnConnectionThread (this=0x7fffa58df120)
    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsParent.cpp:23676
#20 0x00007fffe2c78424 in mozilla::dom::indexedDB::(anonymous namespace)::TransactionDatabaseOperationBase::Run (this=0x7fffa58df120)
    at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsParent.cpp:23847
#21 0x00007fffdebe3195 in nsThread::ProcessNextEvent (this=0x7fffa9123710, aMayWait=true, 
    aResult=0x7fffa69fe48e) at /home/bevis/Projects/Build/gecko/src/xpcom/threads/nsThread.cpp:1033
#22 0x00007fffdec0941c in NS_ProcessNextEvent (aThread=0x7fffa9123710, aMayWait=true)
    at /home/bevis/Projects/Build/gecko/src/xpcom/threads/nsThreadUtils.cpp:508
#23 0x00007fffe2c659ef in mozilla::SpinEventLoopUntil<(mozilla::ProcessFailureBehavior)1, mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::ThreadRunnable::Run()::$_1>(mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::ThreadRunnable::Run()::$_1&&, nsIThread*) (
    aPredicate=<unknown type in /home/bevis/Projects/Build/gecko/src/obj-firefox-central/dist/bin/libxul.so, CU 0xa26ac81, DIE 0xa299d92>, aThread=0x0)
    at /home/bevis/Projects/Build/gecko/src/xpcom/threads/nsThreadUtils.h:323
#24 0x00007fffe2c6575e in mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::ThreadRunnable::Run (this=0x7fffa6f221f0) at /home/bevis/Projects/Build/gecko/src/dom/indexedDB/ActorsParent.cpp:13306
#25 0x00007fffdebe3195 in nsThread::ProcessNextEvent (this=0x7fffa9123710, aMayWait=false, 
    aResult=0x7fffa69fec5e) at /home/bevis/Projects/Build/gecko/src/xpcom/threads/nsThread.cpp:1033
#26 0x00007fffdec0941c in NS_ProcessNextEvent (aThread=0x7fffa9123710, aMayWait=false)
    at /home/bevis/Projects/Build/gecko/src/xpcom/threads/nsThreadUtils.cpp:508
#27 0x00007fffdf5a6661 in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0x7fffa6f20580, 
    aDelegate=0x7fffc1df0c80) at /home/bevis/Projects/Build/gecko/src/ipc/glue/MessagePump.cpp:334
#28 0x00007fffdf4e2855 in MessageLoop::RunInternal (this=0x7fffc1df0c80)
    at /home/bevis/Projects/Build/gecko/src/ipc/chromium/src/base/message_loop.cc:326
#29 0x00007fffdf4e27d5 in MessageLoop::RunHandler (this=0x7fffc1df0c80)
    at /home/bevis/Projects/Build/gecko/src/ipc/chromium/src/base/message_loop.cc:319
#30 0x00007fffdf4e27ad in MessageLoop::Run (this=0x7fffc1df0c80)
    at /home/bevis/Projects/Build/gecko/src/ipc/chromium/src/base/message_loop.cc:299
#31 0x00007fffdebe0e28 in nsThread::ThreadFunc (aArg=0x7fffc8dd1718)
    at /home/bevis/Projects/Build/gecko/src/xpcom/threads/nsThread.cpp:423
#32 0x00007ffff7f0eb45 in _pt_root (arg=0x7fffa91b9700)
    at /home/bevis/Projects/Build/gecko/src/nsprpub/pr/src/pthreads/ptthread.c:216
#33 0x00007ffff7bc16ba in start_thread (arg=0x7fffa69ff700) at pthread_create.c:333
Flags: needinfo?(btseng)
(In reply to Bevis Tseng[:bevis][:btseng] from comment #7)
> 4. The problem is that why the JS context in the argument is not able to
> convert into an nsIGlobalObject.
This JSContext comes from ThreadLocalJSContext::GetOrCreate() at:
https://searchfox.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#24848
After further investigation, I just realized that the support of StructuredClone for WEBCRYPTO_KEY is incomplete especially in IndexedDB if we are trying to de-serialize a stored object(like creating an index) which contains any WebCrypto key in parent process(ActorsParent):
1. ThreadLocalJSContext/NormalJSContext doesn't provide the support of nsIGlobalObject. (As explained in comment 7 & comment 8)
2. The CryptoKey class is a DOM object which implements Add/Release relying on CycleCollector on the creation thread.
   (CreateIndexOp::DoDatabaseWork() is run on a connection thread from an internal poor where no CycleCollector is available!)

We could replace the assertion in
https://searchfox.org/mozilla-central/source/js/xpconnect/wrappers/WrapperFactory.cpp#714-716,721,733
with warning messages and return nullptr to prevent this crash.

However, item#2 seems to be a blocker to support StructuredClone for WEBCRYPTO_KEY or any other(most of all) DOM objects which requires cycle collector in indexedDB.
Bevis, thanks for the investigation!
Flags: needinfo?(jvarga)
NI :baku for more input of the structured-cloned issues in indexedDB mentioned in comment 9, especially for creating a cycle-collectable object on non-main-thread and non-worker thread:
https://searchfox.org/mozilla-central/source/dom/base/StructuredCloneHolder.cpp#361-387
Flags: needinfo?(amarchesini)
I prefer to exclude any CCed DOM object from IDB instead adding the support for them. Currently we have similar issues with: CryptoKey, URLSearchParams and RTCCertificate.
Flags: needinfo?(amarchesini)
Attached patch idb.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8934166 - Flags: review?(btseng)
(In reply to Andrea Marchesini [:baku] from comment #12)
> I prefer to exclude any CCed DOM object from IDB instead adding the support
> for them. Currently we have similar issues with: CryptoKey, URLSearchParams
> and RTCCertificate.

IndexedDB is literally the only place you can persist a CryptoKey with its not-exposed-to-content private keys, though.  That's how the API was designed.  If we do this, we're basically removing support for the API.
:asuth is right, this cannot be an option. You would completely break products using E2E encryption with browser support like the messenger Wire.
In fact I believe they suffer from this very bug already, they simply haven't tracked it down: https://github.com/dfahlander/Dexie.js/issues/298
Priority: -- → P2
Comment on attachment 8934166 [details] [diff] [review]
idb.patch

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

These DOM objects are able to be stored and retrieved in current build because they are serialized and de-serialized in content process via IDBObjectStore APIs with proper nsIGlobalObject from nsGlobalWindow or WorkerPrivate.
The problem of this bug is that if de-serialization happens when creating a new IDBIndex later on a stored object, there will be a crash on nightly or error on release build.
As :asuth said, if we disable it entirely, we are not able to store/retrieve these DOM objects anymore.
Attachment #8934166 - Flags: review?(btseng) → review-
If it is such a pain to solve right now, maybe you can go along the lines of our current workaround and move the data to a temporary table, create the index on the original table (which will succeed as it is empty now) and then insert the data back again.
Maybe as a motivation: The folks over at Webkit have a quite similar problem with IndexedDB indices and WebCrypto keys: https://fiddle.jshell.net/sechel/qn7oesaf/
Here the database does not create the index during insert. Index creation during migration however works.
You can find the corresponding bug here: https://bugs.webkit.org/show_bug.cgi?id=177350
Unfortunately (for us) Webkit doesn't crash otherwise they would be more responsive about it :-)
I have written some patches. I want to see how it goes on try first.
Instead doing what we are currently doing (we create plain JS objects for DOM Blob and we set properties for size/type/name/...), I'm using WebIDL dictionaries.
I'm going to submit patches tomorrow.
It shall be possible to define a dummy nsIGlobalObject similar to SimpleGlobalObject and call JS_SetPrivate() in NormalJSContext::Init() to append it and add 2 more flags of JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS to the NormalJSContext::sGlobalClass:
https://searchfox.org/mozilla-central/source/dom/bindings/SimpleGlobalObject.cpp#84-85,143

The problem to be clarified is how to minimize the effort/resource to enable nsCycleCollector in these connection threads used in ActorsParent.cpp to allow us to create these DOM objects in this scenario.
Yes, I'm using the main-thread. We can change it and use a worker, but that would mean that we need to keep it alive, we must deal with its shutdown and so on: it requires a bigger architecture.
Attachment #8934166 - Attachment is obsolete: true
Attachment #8935322 - Flags: review?(bugmail)
Attached patch part 2 - Upgrade value (obsolete) — Splinter Review
Same logic, for upgrade value.
Attachment #8935323 - Flags: review?(bugmail)
Attached patch part 3 - no templates (obsolete) — Splinter Review
We don't need a template system for the StructuredClone
Attachment #8935324 - Flags: review?(bugmail)
This patch had a test failure on try in dom/indexedDB/test/unit/test_mutableFileUpgrade.js, "Assertion failure: aFile.mBlob, at /builds/worker/workspace/build/src/dom/indexedDB/IDBObjectStore.cpp:906" resulting from the parent-child asymmetry of mozilla::dom::indexedDB::StructuredCloneFile as it relates to Blobs.  (And patch 2's removal of the custom CreateAndWrap* calls.)  In the PBackground-parent, Blobs have an mFileInfo but no mBlob.  It's the pair of SerializeStructuredCloneFiles/DeserializeStructuredCloneFiles that reifies the blobs.  As per the pre-patch logic, we don't actually need to have the Blob around, but we do need CreateAndWrapMutableFile and CreateAndWrapBlobOrFile to not freak out.

It seems like there are a few options:
a) Go back to overriding those methods with ones that perform no-ops.
b) Do something minimal to try and make sure the expected invariants hold superficially.
c) Directly call Serialize/Deserialize and related logic, faking IPC transit.
d) Actually send the data over (same-process) IPC.  This would be a step towards actually recomputing the indices in the context that is performing the database mutations, and is a little reminiscent of the Preprocess mechanism.
Comment on attachment 8935322 [details] [diff] [review]
part 1 - Deserialize on main-thread using a sandbox

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1395,5 @@
> +      MOZ_ASSERT(xpc, "This should never be null!");
> +
> +      // Grab the system principal.
> +      nsCOMPtr<nsIPrincipal> principal;
> +      nsContentUtils::GetSecurityManager()->GetSystemPrincipal(getter_AddRefs(principal));

Why do you need the SystemPrincipal here?  Why can't you just pass null to CreateSandbox so a null principal is used?  (And the answer should be a comment that explains why the null principal doesn't work, and how the system principal cannot possibly allow something bad to happen.)
Comment on attachment 8935322 [details] [diff] [review]
part 1 - Deserialize on main-thread using a sandbox

Clearing review until comment 26's test failure is addressed given its deep entanglement with the fix.
Attachment #8935322 - Flags: review?(bugmail)
Attachment #8935323 - Flags: review?(bugmail)
Attachment #8935324 - Flags: review?(bugmail)
Attachment #8935322 - Attachment is obsolete: true
Attachment #8942239 - Flags: review?(bugmail)
Attachment #8935323 - Attachment is obsolete: true
Attachment #8942241 - Flags: review?(bugmail)
Attachment #8935324 - Attachment is obsolete: true
Attachment #8942242 - Flags: review?(bugmail)
I made you a test.  This test fails because of WASM-related breakage:

in upgrade, creating index
GECKO(18882) | Assertion failure: aFile.mWasmModule, at /home/visbrero/rev_control/hg/mozilla-central/dom/indexedDB/IDBObjectStore.cpp:1007
GECKO(18882) | #01: CreateAndWrapWasmModule (/home/visbrero/rev_control/hg/mozilla-central/dom/indexedDB/IDBObjectStore.cpp:1007)
GECKO(18882) | #02: JSStructuredCloneReader::startRead(JS::MutableHandle<JS::Value>) (/home/visbrero/rev_control/hg/mozilla-central/js/src/vm/StructuredClone.cpp:2357)
GECKO(18882) | #03: JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>) (/home/visbrero/rev_control/hg/mozilla-central/js/src/vm/StructuredClone.cpp:2667)
GECKO(18882) | #04: ReadStructuredClone(JSContext*, JSStructuredCloneData&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) (/home/visbrero/rev_control/hg/mozilla-central/js/src/vm/StructuredClone.cpp:634)
GECKO(18882) | #05: JS_ReadStructuredClone(JSContext*, JSStructuredCloneData&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) (/home/visbrero/rev_control/hg/mozilla-central/js/src/vm/StructuredClone.cpp:2708)
GECKO(18882) | #06: DeserializeIndexValue (/home/visbrero/rev_control/hg/mozilla-central/dom/indexedDB/IDBObjectStore.cpp:1479)
GECKO(18882) | #07: mozilla::Maybe<mozilla::AutoTimeDurationHelper>::reset() (/home/visbrero/rev_control/hg/mozilla-central/obj-firefox-debug/dist/include/mozilla/Maybe.h:445)
GECKO(18882) | #08: NS_ProcessNextEvent(nsIThread*, bool) (/home/visbrero/rev_control/hg/mozilla-central/xpcom/threads/nsThreadUtils.cpp:517)
GECKO(18882) | #09: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/visbrero/rev_control/hg/mozilla-central/ipc/glue/MessagePump.cpp:125)
GECKO(18882) | #10: MessageLoop::RunInternal() (/home/visbrero/rev_control/hg/mozilla-central/ipc/chromium/src/base/message_loop.cc:327)
GECKO(18882) | #11: MessageLoop::RunHandler() (/home/visbrero/rev_control/hg/mozilla-central/ipc/chromium/src/base/message_loop.cc:320)
GECKO(18882) | #12: MessageLoop::Run() (/home/visbrero/rev_control/hg/mozilla-central/ipc/chromium/src/base/message_loop.cc:298)
GECKO(18882) | #13: nsBaseAppShell::Run() (/home/visbrero/rev_control/hg/mozilla-central/widget/nsBaseAppShell.cpp:159)
GECKO(18882) | #14: nsAppStartup::Run() (/home/visbrero/rev_control/hg/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:289)
GECKO(18882) | #15: XREMain::XRE_mainRun() (/home/visbrero/rev_control/hg/mozilla-central/toolkit/xre/nsAppRunner.cpp:4702)
GECKO(18882) | #16: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) (/home/visbrero/rev_control/hg/mozilla-central/toolkit/xre/nsAppRunner.cpp:4841)
GECKO(18882) | #17: XRE_main(int, char**, mozilla::BootstrapConfig const&) (/home/visbrero/rev_control/hg/mozilla-central/toolkit/xre/nsAppRunner.cpp:4933)
GECKO(18882) | #18: mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) (/home/visbrero/rev_control/hg/mozilla-central/toolkit/xre/Bootstrap.cpp:50)
GECKO(18882) | #19: do_main(int, char**, char**) (/home/visbrero/rev_control/hg/mozilla-central/browser/app/nsBrowserApp.cpp:232)
GECKO(18882) | #20: main (/home/visbrero/rev_control/hg/mozilla-central/browser/app/nsBrowserApp.cpp:304)
GECKO(18882) | #21: __libc_start_main (/build/glibc-mXZSwJ/glibc-2.24/csu/../csu/libc-start.c:325)
GECKO(18882) | #22: _start (/home/visbrero/rev_control/hg/mozilla-central/obj-firefox-debug/dist/bin/firefox)
GECKO(18882) | #23: ??? (???:???)
Attachment #8943283 - Flags: review?(amarchesini)
Comment on attachment 8942239 [details] [diff] [review]
part 1 - Deserialize on main-thread using a sandbox

Clearing review until the test passes.  I have to shovel some snow and run an errand, but should be able to do a brief investigation when I get back (~2.5 hrs) as to why the test is failing and maybe provide a fix and then do reviews.
Attachment #8942239 - Flags: review?(bugmail)
Attachment #8942241 - Flags: review?(bugmail)
Attachment #8942242 - Flags: review?(bugmail)
And that assertion stack is from running:
  ./mach test --headless --keep-open=false dom/indexedDB/test/test_upgrade_add_index.html

The xpcshell test variant(s) of the test also experience the same failure, but the stack also looks a little different.
Attachment #8943307 - Flags: review?(bugmail)
Comment on attachment 8942239 [details] [diff] [review]
part 1 - Deserialize on main-thread using a sandbox

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +905,5 @@
>                 aData.tag == SCTAG_DOM_BLOB);
>      MOZ_ASSERT(aFile.mType == StructuredCloneFile::eBlob);
> +
> +    RefPtr<Blob> blob = aFile.mBlob;
> +    if (!blob) {

Please add a comment along the lines of:

If we are creating an index, we do not have an mBlob but do have an mInfo.  Unlike other index or upgrade cases, we do need a real-looking Blob/File instance because the index's key path can reference their properties.  Rather than create a fake-looking object, create a real Blob.
Attachment #8942239 - Flags: review+
Comment on attachment 8942241 [details] [diff] [review]
part 2 - Upgrade value

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +871,5 @@
>                             JS::MutableHandle<JSObject*> aResult)
>    {
>      MOZ_ASSERT(aCx);
> +
> +    // If we have eBlob, we are in an update where we don't care about a real

Phrasing nit: s/an update/an IDB SQLite schema upgrade/

Update or upgrade on their own are ambiguous.
Attachment #8942241 - Flags: review+
Comment on attachment 8942239 [details] [diff] [review]
part 1 - Deserialize on main-thread using a sandbox

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +912,5 @@
> +      if (!file) {
> +        return false;
> +      }
> +
> +      blob = File::CreateFromFile(nullptr, file);

For consistency/invariant purposes, since we're going through the work to create a real File here, I think we perhaps want to change this up so the fileId gets in there.  (Which I believe is only used for super thorough unit tests.)  So:

  RefPtr<FileBlobImpl> impl = new FileBlobImpl(file);
  impl->SetFileId(file.mFileInfo->Id());
  blob = File::Create(null, impl);
Comment on attachment 8943307 [details] [diff] [review]
part 4 - crash fixed

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

These are all very nice cleanups!

From a design perspective, I was initially not crazy about the conditionals in the top of the CreateAndWrap* methods, but I took a quick look at adding some additional state to StructuredCloneReadInfo and this patchset's way with comments is cleaner.
Attachment #8943307 - Flags: review?(bugmail) → review+
For my own use going forward, here are my review notes for my own personal understanding:

DeserializeIndexValueHelper:
- Cross-thread blocking helper with monitor notification.
- Exists to address the fundamental issue that we need a global that can handle
  cycle-collected DOM bindings.  Attempting to deserialize with the
  CreateIndexOp::ThreadLocalJSContext was not good enough.  Using a Worker
  would be cleaner, but is future work.
- Empty data still results in fast-path that avoids the main-thread transit.

DeserializeUpgradeValueHelper:
- Replaces UpgradeDeserializationHelper, with a similar rationale to
  DeserializeIndexValueHelper and accordingly a similar implementation that also
  uses SandboxHolder, etc.
- The big difference is PopulateFileIds is refactored out of the
  on-IO-thread UpgradeFileIdsFunction::OnFunctionCall into the on-main-thread
  helper.  (The structured clone deserialization and interpretation of the
  IDB StructuredCloneReadInfo is all moved there.)

Changes to ValueDeserializationHelper:
- CreateAndWrapBlobOrFile gains logic to create the missing Blob instance in
  part 1.  We don't really need to be creating the File here other than to avoid
  surprise later.  The important meta-info for the blob/file for index creation
  for the evaluation of any key paths is set lower down in the method by the
  calls to SetLazyData.  This logic is merely approximating the net result
  of ActorParent.cpp's SerializeStructuredCloneFiles and ActorChild.cpp's
  DeserializeStructuredCloneFiles which:
  - Create a FileBlobImpl wrapping the result of GetCheckedFileForId() and
    annotated with SetFileId().  The file id is used exclusively for unit tests
    that need to pierce the abstraction barrier via superpowers.  Since the blob
    is never surfaced that we produce, the lack of setting the fileid doesn't
    matter, although maybe we should... noting in review.  Then IPC serialize.
  - In the child, performs IPC deserialization and then does Blob::Create which
    amounts to File::Create, so it ends up the same.
- CreateAndWrapMutableFile gains logic to change the StructuredCloneFile mType
  to be eMutableFile if it was eBlob in part 2.  This is part of the
  DeserializeUpgradeValueHelper migration which depended on the structured clone
  read to tell us what the type of the blob is.
- CreateAndWrapWasmModule gets dummy-object logic in part 4.  This relaxes the
  assertion that mWasmModule is present that only holds true if
  ActorsChild.cpp's DeserializeStructuredCloneFiles populated it.  We don't
  really need the assertion for correctness, it's just more of an anti-crash
  guard, so the failsafe and existing functionality tests should cover that
  guarantee.

Removed CreateIndexOp::ThreadLocalJSContext : NormalJSContext (part 1):
- Superseded by SandboxHolder.
Removed NormalJSContext (part 2):
- This created a custom context with a basically no-op global.

SandboxHolder:
- Correctly uses StaticRefptr.

GetFileForFileInfo is moved from a soupy existence in ActorsParent.cpp to live
next to similar methods in FileInfo.{h,cpp} so that IDBObjectStore.cpp's
ValueDeserializationHelper::CreateAndWrapBlobOrFile can reify the blob given
the mFileInfo.
Depends on: 1432133
Group: dom-core-security → core-security-release
Comment on attachment 8943283 [details] [diff] [review]
Test index creation with existing values containing crypto keys and wasm modules

Already landed.
Attachment #8943283 - Flags: review?(amarchesini) → review+
Flags: sec-bounty?
sec-low bugs don't qualify for the bug bounty
Flags: sec-bounty? → sec-bounty-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.