1505821 - SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray-inl.h:22:17 in ~nsTArray_base

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev b3da3f53f804.  Testcase must be served via a local webserver in order to reproduce.

==30643==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7f751e4c8850 bp 0x7f74f97f2ef0 sp 0x7f74f97f2dc0 T29)
==30643==The signal is caused by a WRITE memory access.
==30643==Hint: address points to the zero page.
    #0 0x7f751e4c884f in ~nsTArray_base /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray-inl.h:22:17
    #1 0x7f751e4c884f in ~nsTArray_Impl /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:940
    #2 0x7f751e4c884f in mozilla::dom::indexedDB::(anonymous namespace)::Cursor::SendResponseInternal(mozilla::dom::indexedDB::CursorResponse&, nsTArray<FallibleTArray<mozilla::dom::indexedDB::StructuredCloneFile> > const&) /builds/worker/workspace/build/src/dom/indexedDB/ActorsParent.cpp:16654
    #3 0x7f751e4c50c9 in mozilla::dom::indexedDB::(anonymous namespace)::Cursor::CursorOpBase::SendFailureResult(nsresult) /builds/worker/workspace/build/src/dom/indexedDB/ActorsParent.cpp:27423:14
    #4 0x7f751e46a693 in mozilla::dom::indexedDB::(anonymous namespace)::TransactionDatabaseOperationBase::SendPreprocessInfoOrResults(bool) /builds/worker/workspace/build/src/dom/indexedDB/ActorsParent.cpp:23497:12
    #5 0x7f751e461ff8 in mozilla::dom::indexedDB::(anonymous namespace)::TransactionDatabaseOperationBase::Run() /builds/worker/workspace/build/src/dom/indexedDB/ActorsParent.cpp
    #6 0x7f75150e7be1 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1246:14
    #7 0x7f75150f098d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:530:10
    #8 0x7f7516364064 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:364:5
    #9 0x7f751625eb5e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #10 0x7f751625eb5e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #11 0x7f751625eb5e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #12 0x7f75150e0023 in nsThread::ThreadFunc(void*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:505:11
    #13 0x7f75393b4676 in _pt_root /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #14 0x7f7538ff86da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
    #15 0x7f7537fd688e in clone /build/glibc-OTsEL5/glibc-2.27/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray-inl.h:22:17 in ~nsTArray_base
Thread T29 (IPDL Background) created by T0 here:
    #0 0x555b1c2986ad in __interceptor_pthread_create /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:210:3
    #1 0x7f75393b13a5 in _PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:433:14
    #2 0x7f75393b0f8e in PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:518:12
    #3 0x7f75150e2f99 in nsThread::Init(nsTSubstring<char> const&) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:719:8
    #4 0x7f75150ef64e in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp:485:22
    #5 0x7f75150f3fa4 in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:143:45
    #6 0x7f751632a1d2 in NS_NewNamedThread<16> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:75:10
    #7 0x7f751632a1d2 in (anonymous namespace)::ParentImpl::CreateBackgroundThread() /builds/worker/workspace/build/src/ipc/glue/BackgroundImpl.cpp:1015
    #8 0x7f751632fa7a in RunOnMainThread /builds/worker/workspace/build/src/ipc/glue/BackgroundImpl.cpp:1330:30
    #9 0x7f751632fa7a in (anonymous namespace)::ParentImpl::CreateActorHelper::Run() /builds/worker/workspace/build/src/ipc/glue/BackgroundImpl.cpp:1351
    #10 0x7f75150e7be1 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1246:14
    #11 0x7f75150f098d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:530:10
    #12 0x7f75150e56ed in SpinEventLoopUntil<mozilla::ProcessFailureBehavior::ReportToCaller, (lambda at /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:954:22)> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:347:25
    #13 0x7f75150e56ed in nsThread::Shutdown() /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:954
    #14 0x7f75172ec6bb in applyImpl<nsIThread, nsresult (nsIThread::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1191:12
    #15 0x7f75172ec6bb in apply<nsIThread, nsresult (nsIThread::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1197
    #16 0x7f75172ec6bb in mozilla::detail::RunnableMethodImpl<RefPtr<nsIThread>, nsresult (nsIThread::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1242
    #17 0x7f75150e7be1 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1246:14
    #18 0x7f75150f098d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:530:10
    #19 0x7f75150eff41 in SpinEventLoopUntil<mozilla::ProcessFailureBehavior::ReportToCaller, (lambda at /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp:558:36)> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:347:25
    #20 0x7f75150eff41 in nsThreadManager::SpinEventLoopUntilInternal(nsINestedEventLoopCondition*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp:558
    #21 0x7f75151268d1 in NS_InvokeByIndex /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106
    #22 0x7f75173eb58e in Invoke /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1723:12
    #23 0x7f75173eb58e in Call /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1268
    #24 0x7f75173eb58e in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1232
    #25 0x7f75173f32e8 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1020:12
    #26 0x7f752546831d in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:468:15
    #27 0x7f752546831d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:560
    #28 0x7f7525451c0d in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:620:12
    #29 0x7f7525451c0d in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3461
    #30 0x7f7525435566 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447:12
    #31 0x7f7525468cc1 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:587:15
    #32 0x7f752546a942 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:633:10
    #33 0x7f7523e3e1f0 in js::fun_apply(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/JSFunction.cpp:1381:12
    #34 0x7f752546831d in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:468:15
    #35 0x7f752546831d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:560
    #36 0x7f7525451c0d in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:620:12
    #37 0x7f7525451c0d in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3461
    #38 0x7f7525435566 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447:12
    #39 0x7f7525468cc1 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:587:15
    #40 0x7f752546a942 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:633:10
    #41 0x7f7524506aea in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2911:12
    #42 0x7f75173cd9a9 in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedJSClass.cpp:1174:23
    #43 0x7f7515127fd8 in PrepareAndDispatch /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:127:37
    #44 0x7f7515126eaa in SharedStub (/home/forb1dden/builds/mc-asan/libxul.so+0x4811eaa)
    #45 0x7f752375d0f4 in nsXREDirProvider::DoStartup() /builds/worker/workspace/build/src/toolkit/xre/nsXREDirProvider.cpp:1094:11
    #46 0x7f7523732329 in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4619:16
    #47 0x7f7523735ba0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4934:8
    #48 0x7f7523737523 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:5026:21
    #49 0x555b1c2e267c in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:237:22
    #50 0x555b1c2e267c in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:329
    #51 0x7f7537ed6b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

==30643==ABORTING

Comment 1

[violet.bugreport]

Interestingly, this bug is caused by two unrelated serialization/deserialization bugs.

The most serious one is the out-of-order backreference in StructuredClone code in JS engine, it's irrelevant to IndexedDB. See bug 1538622. This is the cause of the immediate crash of the whole browser when the original testcase is loaded in the release build.

However, there is still a minor bug in the IndexedDB part. The serialization and deserialization for KeyPath is inconsistent for ['']. That will cause an assertion failure when the testcase is loaded twice.

Comment 2

[violet.bugreport]

Attachment: assertion-failure-when-loaded-twice.html

Comment 3

[violet.bugreport]

Attachment: Bug 1505821 - DeserializeFromString should append an empty string for a trailing comma

KeyPath [''] will be serialized as a single comma, when deserializing it, we should
return back an array with a single empty string, instead of an empty array. Otherwise
we will get inconsistent result with the KeyPath::Parse() method, causing assertions
failure.

Comment 4

[violet.bugreport]

Hi Jan,

Could you review this patch I submitted 9 days ago? It's just a straightforward parsing error, since the major part has already been resolved by my patch to the JS engine.

Thanks!

Comment 5

[Jan Varga [:janv]]

Yeah, I'll take a look today/tomorrow.

Comment 6

[Jan Varga [:janv]]

Friday is the day.

Comment 7

[Jan Varga [:janv]]

Reviewing this now.

Comment 8

[Jan Varga [:janv]]

Ok, the new crash test won't assert if I run it separately.
It obviously depends on 1507229-1.html
Both crash tests share the same database, I'm not sure if this is a good idea.
1507229-1.html should delete the database before finishing and the new crash test should simulate the problem without depending on other tests.
Both crash tests should rather use unique database names (bug numbers?)

Comment 9

[violet.bugreport]

Updated.

Also there is another patch of mine which you've already reviewed: https://phabricator.services.mozilla.com/D24009. I've already added the testcase you requested.

Comment 10

[Jan Varga [:janv]]

If I disable your fix in KeyPath.cpp, the assertion isn't hit at all.
The new crash test should "simulate the problem", so without the fix it should hit the assertion.

Comment 11

[violet.bugreport]

It actually will hit at Test Verify, I discovered this because of TV failure.

I don't know how to make it hit at first load, do you know how to write this test since now the cause is clear?

Comment 12

[Jan Varga [:janv]]

Generally, you shouldn't depend on other tests in the directory.
Can you try to reopen the database in the same test ?

Comment 13

[violet.bugreport]

The assertion failure isn't caused by dependency between two tests, it's caused by loading itself twice. It has nothing to do with other tests. TV can reproduce it.

I couldn't reproduce it by reopen the database in the same file twice.

Comment 14

[Jan Varga [:janv]]

That's not exactly true. I hit the assertion with your original patch.

Comment 15

[Jan Varga [:janv]]

If you want to provide a good test for this you can't rely on the verify mode. It should reproduce problem without that dependency.

Comment 16

[violet.bugreport]

I don't have any idea how to do it because I'm far from an IndexedDB expert. Probably you know how to provide this testcase? I think the cause is really clear.

Comment 17

[Jan Varga [:janv]]

So I should take this bug ?

Comment 18

[violet.bugreport]

So I should take this bug ?

If you think an obscure testcase is really important to an essentially typo-fixing bug, I have no objection if you want to take it. I believe it requires much more effort to find a obscure testcase than fixing the typo itself, and the testcase immediately becomes useless since it's almost impossible to get regressed to a typo in the future.

Comment 19

[Jan Varga [:janv]]

I plan to write a more detailed comment here, but for now ...
Does the test need to be a crash test ?

We just need to create a new database and then force parent process to reload database metadata. It means we need to close the database, reset origin directory (so parent closes all related os files) and reopen database again.

Comment 20

[Jan Varga [:janv]]

(In reply to violet.bugreport from comment #18)

So I should take this bug ?

If you think an obscure testcase is really important to an essentially typo-fixing bug, I have no objection if you want to take it. I believe it requires much more effort to find a obscure testcase than fixing the typo itself, and the testcase immediately becomes useless since it's almost impossible to get regressed to a typo in the future.

I'm sorry I didn't mean to discourage you from working on this. We appreciate any help, you are doing great job.
Anyway, it can be frustrating sometimes, but yes, writing a patch it's only a small part of the job. Reviews can take much more time than writing a patch and writing a test can be really hard. But that's how it works if you want to provide reliable solutions.
For example, if we land the crash test as it is, it can cause trouble in future and can be "frustrating" for someone else and that someone else can blame you or the reviewer for doing it incorrectly.

I'm delagating this and other reviews to Andrew Sutherland since I have to work on new LocalStorage implementation to stabilize it before next soft freeze.

Comment 21

[violet.bugreport]

No problem, and thanks for your time.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06e8fe752e30
DeserializeFromString should append an empty string for a trailing comma r=asuth

Comment 23

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/06e8fe752e30