Closed
Bug 1134671
Opened 9 years ago
Closed 9 years ago
Service Worker Cache should cache the sqlite connection across Actions
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(1 file, 2 obsolete files)
24.99 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
Right now the Service Worker Cache creates a new sqlite connection for every action. This is obviously not very performant. We should cache the sqlite connection in the Context. The threading strategy for the Context needs to be finalized, however. If we want to use a thread pool with the context, then we need bug 1121282.
Assignee | ||
Updated•9 years ago
|
Blocks: serviceworker-cache
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
This formalizes the one-thread-per-Context relationship and then adds a shared Connection object across Actions run on the same Context. Note, the Connection is not shared with the first SetupAction because that is run on the QuotaManager IOThread right now. Cleanup is a little tricky in that we need to free the shared connection before ~Context() runs. So we begin disposing it when Context drops its self-ref. Any Actions dispatched after that point will fallback to the old behavior of creating a new temporary connection. This works in local testing. I'm waiting for try results to see if it impacts the timeouts in the wpt cache-match tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a87f72e2074
Attachment #8602241 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•9 years ago
|
||
Apparently the Cache tests are in wpt3 on some platforms and wpt4 on others. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4619dd00b123
Comment 3•9 years ago
|
||
Comment on attachment 8602241 [details] [diff] [review] Keep sqlite connection open between Cache API operations. r=ehsan Review of attachment 8602241 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/Context.cpp @@ +86,5 @@ > > +class Context::Data final : public Action::Data > +{ > +public: > + Data(nsIThread* aTarget) Nit: explicit. @@ +617,5 @@ > AutoRestore<bool> executingRunOnTarget(mExecutingRunOnTarget); > mExecutingRunOnTarget = true; > > + nsRefPtr<Data> data; > + data.swap(mData); Nit: you can do this like: nsRefPtr<Data> data(mData.forget()); @@ +947,5 @@ > + new ActionRunnable(this, mData, mTarget, aAction, mQuotaInfo); > + > + if (aDoomData) { > + nsRefPtr<Data> data; > + data.swap(mData); Why not just say: mData = nullptr;
Attachment #8602241 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7257315c37a
Attachment #8602241 -
Attachment is obsolete: true
Attachment #8602329 -
Flags: review+
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3347a2c69b7
Assignee | ||
Comment 6•9 years ago
|
||
This cannot land yet. Tons of mulet M(1) orange and its triggering assertions like this in linux64 debug M-e10s(1): 19:30:25 INFO - Assertion failure: !mQuotaObjects.Count(), at /builds/slave/try-l64-d-00000000000000000000/build/src/dom/quota/QuotaObject.h:97 19:30:48 INFO - #01: nsTArray_Impl<nsRefPtr<mozilla::dom::quota::OriginInfo>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [xpcom/glue/nsTArray.h:1755] 19:30:48 INFO - #02: mozilla::dom::quota::QuotaManager::LockedRemoveQuotaForOrigin(mozilla::dom::quota::PersistenceType, nsACString_internal const&, nsACString_internal const&) [dom/quota/QuotaManager.cpp:2975] 19:30:48 INFO - #03: mozilla::dom::quota::OriginClearRunnable::DeleteFiles(mozilla::dom::quota::QuotaManager*, mozilla::dom::quota::PersistenceType) [dom/quota/QuotaManager.h:142] 19:30:48 INFO - #04: mozilla::dom::quota::OriginClearRunnable::Run() [dom/quota/QuotaManager.cpp:3856] 19:30:48 INFO - #05: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:868] 19:30:48 INFO - #06: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/glue/nsThreadUtils.cpp:265] 19:30:48 INFO - #07: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:355] 19:30:48 INFO - #08: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:234] 19:30:48 INFO - #09: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:517] 19:30:48 INFO - #10: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:366] 19:30:48 INFO - #11: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:215] 19:30:48 INFO - #12: libpthread.so.0 + 0x7e9a 19:30:48 INFO - #13: libc.so.6 + 0xf42ed
Assignee | ||
Comment 7•9 years ago
|
||
Actually, it appears to be a use-after-free via QuotaManager. Jan, Ben, does this ring any bells or point to what I am doing wrong? I verified locally that my OfflineStorage is still alive when ~Connection() runs here. 19:47:24 INFO - ================================================================= 19:47:24 INFO - ==3972==ERROR: AddressSanitizer: heap-use-after-free on address 0x60800051b748 at pc 0x7f2449a54b51 bp 0x7f241011e050 sp 0x7f241011e048 19:47:24 INFO - READ of size 8 at 0x60800051b748 thread T135 (DOMCacheThread) 19:47:26 INFO - #0 0x7f2449a54b50 in Remove /builds/slave/try-l64-asan-00000000000000000/build/src/xpcom/glue/pldhash.cpp:709 19:47:26 INFO - #1 0x7f2449a54b50 in PL_DHashTableRemove(PLDHashTable*, void const*) /builds/slave/try-l64-asan-00000000000000000/build/src/xpcom/glue/pldhash.cpp:754 19:47:26 INFO - #2 0x7f244e2afd8e in RemoveEntry /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/quota/../../dist/include/nsTHashtable.h:179 19:47:26 INFO - #3 0x7f244e2afd8e in Remove /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/quota/../../dist/include/nsBaseHashtable.h:148 19:47:26 INFO - #4 0x7f244e2afd8e in mozilla::dom::quota::QuotaObject::Release() /builds/slave/try-l64-asan-00000000000000000/build/src/dom/quota/QuotaObject.cpp:101 19:47:26 INFO - #5 0x7f244ad65597 in assign_assuming_AddRef /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/storage/src/../../dist/include/nsRefPtr.h:53 19:47:26 INFO - #6 0x7f244ad65597 in assign_with_AddRef /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/storage/src/../../dist/include/nsRefPtr.h:37 19:47:26 INFO - #7 0x7f244ad65597 in operator= /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/storage/src/../../dist/include/nsRefPtr.h:144 19:47:26 INFO - #8 0x7f244ad65597 in (anonymous namespace)::xClose(sqlite3_file*) /builds/slave/try-l64-asan-00000000000000000/build/src/storage/src/TelemetryVFS.cpp:357 19:47:26 INFO - #9 0x7f245d8f3a99 in sqlite3OsClose /builds/slave/try-l64-asan-00000000000000000/build/src/db/sqlite3/src/sqlite3.c:16443 19:47:26 INFO - #10 0x7f245d8f3a99 in sqlite3PagerClose /builds/slave/try-l64-asan-00000000000000000/build/src/db/sqlite3/src/sqlite3.c:45576 19:47:26 INFO - #11 0x7f245d912653 in sqlite3BtreeClose /builds/slave/try-l64-asan-00000000000000000/build/src/db/sqlite3/src/sqlite3.c:55277 19:47:26 INFO - #12 0x7f245d8497da in sqlite3LeaveMutexAndCloseZombie /builds/slave/try-l64-asan-00000000000000000/build/src/db/sqlite3/src/sqlite3.c:128602 19:47:26 INFO - #13 0x7f245d86e318 in sqlite3Close /builds/slave/try-l64-asan-00000000000000000/build/src/db/sqlite3/src/sqlite3.c:128545 19:47:26 INFO - #14 0x7f244ad3fb6b in mozilla::storage::Connection::internalClose(sqlite3*) /builds/slave/try-l64-asan-00000000000000000/build/src/storage/src/mozStorageConnection.cpp:929 19:47:26 INFO - #15 0x7f244ad3ad2a in mozilla::storage::Connection::Close() /builds/slave/try-l64-asan-00000000000000000/build/src/storage/src/mozStorageConnection.cpp:1167 19:47:26 INFO - #16 0x7f244ad3a7c2 in mozilla::storage::Connection::~Connection() /builds/slave/try-l64-asan-00000000000000000/build/src/storage/src/mozStorageConnection.cpp:506 19:47:26 INFO - #17 0x7f244ad3b2e9 in mozilla::storage::Connection::Release() /builds/slave/try-l64-asan-00000000000000000/build/src/storage/src/mozStorageConnection.cpp:540 19:47:26 INFO - #18 0x7f2449a46324 in NS_ProxyRelease(nsIEventTarget*, nsISupports*, bool) /builds/slave/try-l64-asan-00000000000000000/build/src/xpcom/glue/nsProxyRelease.cpp:38 19:47:26 INFO - #19 0x7f244ad6aa5d in mozilla::storage::Service::unregisterConnection(mozilla::storage::Connection*) /builds/slave/try-l64-asan-00000000000000000/build/src/storage/src/mozStorageService.cpp:335 19:47:26 INFO - #20 0x7f244ad3b31f in mozilla::storage::Connection::Release() /builds/slave/try-l64-asan-00000000000000000/build/src/storage/src/mozStorageConnection.cpp:534 19:47:26 INFO - #21 0x7f2449a46324 in NS_ProxyRelease(nsIEventTarget*, nsISupports*, bool) /builds/slave/try-l64-asan-00000000000000000/build/src/xpcom/glue/nsProxyRelease.cpp:38 19:47:26 INFO - #22 0x7f244d746572 in NS_ProxyRelease<mozIStorageConnection> /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/cache/../../dist/include/nsProxyRelease.h:33 19:47:26 INFO - #23 0x7f244d746572 in ~Data /builds/slave/try-l64-asan-00000000000000000/build/src/dom/cache/Context.cpp:116 19:47:26 INFO - #24 0x7f244d746572 in Release /builds/slave/try-l64-asan-00000000000000000/build/src/dom/cache/Context.cpp:123 19:47:26 INFO - #25 0x7f244d746572 in ~nsRefPtr /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/cache/../../dist/include/nsRefPtr.h:66 19:47:26 INFO - #26 0x7f244d746572 in mozilla::dom::cache::Context::ActionRunnable::Run() /builds/slave/try-l64-asan-00000000000000000/build/src/dom/cache/Context.cpp:635 19:47:26 INFO - #27 0x7f24499ee7c4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/try-l64-asan-00000000000000000/build/src/xpcom/threads/nsThread.cpp:868 19:47:26 INFO - #28 0x7f2449a4fcfa in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/try-l64-asan-00000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:265 19:47:26 INFO - #29 0x7f244a2aa988 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/try-l64-asan-00000000000000000/build/src/ipc/glue/MessagePump.cpp:355 19:47:26 INFO - #30 0x7f244a236f6c in RunInternal /builds/slave/try-l64-asan-00000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:233 19:47:26 INFO - #31 0x7f244a236f6c in RunHandler /builds/slave/try-l64-asan-00000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:226 19:47:26 INFO - #32 0x7f244a236f6c in MessageLoop::Run() /builds/slave/try-l64-asan-00000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:200 19:47:26 INFO - #33 0x7f24499eb2b8 in nsThread::ThreadFunc(void*) /builds/slave/try-l64-asan-00000000000000000/build/src/xpcom/threads/nsThread.cpp:364 19:47:26 INFO - #34 0x7f246106a135 in _pt_root /builds/slave/try-l64-asan-00000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212 19:47:26 INFO - #35 0x7f2464585e99 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x7e99) 19:47:26 INFO - #36 0x7f24636952ec (/lib/x86_64-linux-gnu/libc.so.6+0xf42ec) 19:47:26 INFO - 0x60800051b748 is located 40 bytes inside of 96-byte region [0x60800051b720,0x60800051b780) 19:47:26 INFO - freed by thread T98 (Storage I/O) here: 19:47:26 INFO - #0 0x471f11 in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64 19:47:26 INFO - #1 0x7f244e2b3c15 in ~OriginInfo /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/quota/../../dist/include/mozilla/mozalloc.h:210 19:47:26 INFO - #2 0x7f244e2b3c15 in Release /builds/slave/try-l64-asan-00000000000000000/build/src/dom/quota/QuotaObject.h:83 19:47:26 INFO - #3 0x7f244e2b3c15 in ~nsRefPtr /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/quota/../../dist/include/nsRefPtr.h:66 19:47:26 INFO - #4 0x7f244e2b3c15 in Destruct /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/quota/../../dist/include/nsTArray.h:518 19:47:26 INFO - #5 0x7f244e2b3c15 in DestructRange /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/quota/../../dist/include/nsTArray.h:1756 19:47:26 INFO - #6 0x7f244e2b3c15 in nsTArray_Impl<nsRefPtr<mozilla::dom::quota::OriginInfo>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/quota/../../dist/include/nsTArray.h:1449 19:47:26 INFO - #7 0x7f244e2a545a in RemoveElementAt /builds/slave/try-l64-asan-00000000000000000/build/src/obj-firefox/dom/quota/../../dist/include/nsTArray.h:1455 19:47:26 INFO - #8 0x7f244e2a545a in LockedRemoveOriginInfo /builds/slave/try-l64-asan-00000000000000000/build/src/dom/quota/QuotaObject.cpp:366 19:47:26 INFO - #9 0x7f244e2a545a in mozilla::dom::quota::QuotaManager::LockedRemoveQuotaForOrigin(mozilla::dom::quota::PersistenceType, nsACString_internal const&, nsACString_internal const&) /builds/slave/try-l64-asan-00000000000000000/build/src/dom/quota/QuotaManager.cpp:2973 19:47:26 INFO - #10 0x7f244e2abdfb in RemoveQuotaForOrigin /builds/slave/try-l64-asan-00000000000000000/build/src/dom/quota/QuotaManager.h:142 19:47:26 INFO - #11 0x7f244e2abdfb in mozilla::dom::quota::OriginClearRunnable::DeleteFiles(mozilla::dom::quota::QuotaManager*, mozilla::dom::quota::PersistenceType) /builds/slave/try-l64-asan-00000000000000000/build/src/dom/quota/QuotaManager.cpp:3803 19:47:26 INFO - #12 0x7f244e2acaf2 in mozilla::dom::quota::OriginClearRunnable::Run() /builds/slave/try-l64-asan-00000000000000000/build/src/dom/quota/QuotaManager.cpp:3852 From here: https://treeherder.mozilla.org/logviewer.html#?job_id=7290240&repo=try
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(Jan.Varga)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(Jan.Varga)
Assignee | ||
Comment 8•9 years ago
|
||
I see the problem. I am clearing the shared connection ref on the target thread, but dispatching to the main thread to complete the Context cleanup first. The failure triggers more on e10s because parent process main thread is more responsive there.
Assignee | ||
Comment 9•9 years ago
|
||
Tweak how we clear the mData ref in ActionRunnable::Run() so that its always done before dispatching back to the main thread. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ca283171614 If this is green in the morning it will make my road trip with the family infinitely better. I may sing the Frozen soundtrack.
Attachment #8602329 -
Attachment is obsolete: true
Attachment #8602491 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
Try build looks reasonable. I'm going to push it to inbound.
Comment 12•9 years ago
|
||
This was missing an 'override' keyword, causing local Werror bustage in clang 3.6 & newer. Pushed a fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/48a6531ce81b
https://hg.mozilla.org/mozilla-central/rev/53f9298c1753
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•