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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1160013
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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)
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 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+
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
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)
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(Jan.Varga)
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.
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+
Try build looks reasonable.  I'm going to push it to inbound.
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
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1164100
Blocks: 1168135
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.