Closed
Bug 1120336
Opened 9 years ago
Closed 9 years ago
XHR read of sliced (IndexedDB) Blob from worker thread hangs / triggers assertion
Categories
(Core :: Storage: IndexedDB, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox36 | --- | unaffected |
firefox37 | --- | fixed |
firefox38 | --- | fixed |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: asuth, Assigned: bent.mozilla)
References
Details
Attachments
(1 file, 3 obsolete files)
21.25 KB,
patch
|
khuey
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Very similar to bug 1113340. In this case we've sliced the Blob instead of just trying to read it in its entirety. I've put this in the IndexedDB component for consistency with that bug and because that's where the test lives, but another component might be better. On a non-debug build, the test just sorta hangs and the backtrace doesn't show anything happening on the main or DOM worker threads. On a debug build the following assertion happens: 18 INFO Sending blob to a worker Assertion failure: mActorWasCreated, at dom/ipc/Blob.cpp:2586 #01: mozilla::dom::workers::CreateURLRunnable::MainThreadRun() (dom/workers/URL.cpp:96) #02: mozilla::dom::workers::WorkerMainThreadRunnable::Run() (dom/workers/WorkerRunnable.cpp:532) #03: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:855 (discriminator 1))
Reporter | ||
Comment 1•9 years ago
|
||
er, and note that the patch involves an "hg cp" operation, so it might look a little confusing in bugzilla's diff views, etc.
Assignee | ||
Comment 2•9 years ago
|
||
Assignee: nobody → bent.mozilla
Attachment #8547435 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8547843 -
Flags: review?(khuey)
Attachment #8547843 -
Flags: review?(khuey) → review+
Reporter | ||
Comment 3•9 years ago
|
||
Thanks for the fix! Just as a heads-up, there is going to be a third one of these involving MultipartFileImpl. Right now with this fix applied I'm now seeing the following hang: 0 pthread_cond_wait@@GLIBC_2.3.2 nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185 1 PR_WaitCondVar nsprpub/pr/src/pthreads/ptsynch.c:385 2 Wait ../dist/include/mozilla/CondVar.h:79 3 Wait ../dist/include/mozilla/Monitor.h:40 4 Wait ../dist/include/mozilla/Monitor.h:88 5 dom::BlobChild::RemoteBlobImpl::CreateStreamHelper::GetStream dom/ipc/Blob.cpp:2474 6 dom::BlobChild::RemoteBlobImpl::GetInternalStream dom/ipc/Blob.cpp:2360 7 MultipartFileImpl::GetInternalStream dom/base/MultipartFileImpl.cpp:42 8 MultipartFileImpl::GetInternalStream dom/base/MultipartFileImpl.cpp:42 9 MultipartFileImpl::GetInternalStream dom/base/MultipartFileImpl.cpp:42 10 dom::FileImplBase::GetSendInfo dom/base/File.cpp:881 11 dom::File::GetSendInfo dom/base/File.cpp:544 12 GetRequestBody dom/base/nsXMLHttpRequest.cpp:2475 13 GetRequestBody dom/base/nsXMLHttpRequest.cpp:2543 14 nsXMLHttpRequest::GetRequestBody dom/base/nsXMLHttpRequest.cpp:2591 15 nsXMLHttpRequest::Send dom/base/nsXMLHttpRequest.cpp:2785 16 nsXMLHttpRequest::Send dom/base/nsXMLHttpRequest.cpp:2655 17 (anonymous namespace)::SendRunnable::MainThreadRun dom/workers/XMLHttpRequest.cpp:1568 18 (anonymous namespace)::WorkerThreadProxySyncRunnable::Run dom/workers/XMLHttpRequest.cpp:1434 19 nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:855 20 NS_ProcessNextEvent xpcom/glue/nsThreadUtils.cpp:265 21 ipc::MessagePump::Run ipc/glue/MessagePump.cpp:99 22 MessageLoop::RunInternal ipc/chromium/src/base/message_loop.cc:233 23 RunHandler ipc/chromium/src/base/message_loop.cc:226 24 MessageLoop::Run ipc/chromium/src/base/message_loop.cc:200 25 nsBaseAppShell::Run widget/nsBaseAppShell.cpp:164 26 nsAppStartup::Run toolkit/components/startup/nsAppStartup.cpp:281 27 XREMain::XRE_mainRun toolkit/xre/nsAppRunner.cpp:4141 28 XREMain::XRE_main toolkit/xre/nsAppRunner.cpp:4217 29 XRE_main toolkit/xre/nsAppRunner.cpp:4437 30 do_main b2g/app/nsBrowserApp.cpp:160 31 main b2g/app/nsBrowserApp.cpp:293 I will try and create an appropriate mochitest for this additional case now, but if it's obvious what's going wrong, please feel free to jump in and fix it! :)
Reporter | ||
Comment 4•9 years ago
|
||
This mochitest reproduces the b2g test failure I observed, the only difference is that I'm not seeing the dom/base/MultipartFileImpl.cpp:42 stuff in triplicate. This generates the following assertion when run in a firefox debug build as a mochitest: Assertion failure: !NS_IsMainThread(), at dom/ipc/Blob.cpp:2447 #01: ~nsRefPtr (xpcom/base/nsRefPtr.h:59) #02: MultipartFileImpl::GetInternalStream(nsIInputStream**) (dom/base/MultipartFileImpl.cpp:42) #03: mozilla::dom::FileImplBase::GetSendInfo(nsIInputStream**, unsigned long*, nsACString_internal&, nsACString_internal&) (dom/base/File.cpp:881) #04: GetRequestBody (dom/base/nsXMLHttpRequest.cpp:2475) #05: nsXMLHttpRequest::GetRequestBody(nsIVariant*, mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&, nsIInputStream**, unsigned long*, nsACString_internal&, nsACString_internal&) (dom/base/nsXMLHttpRequest.cpp:2591) #06: nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&) (dom/base/nsXMLHttpRequest.cpp:2785) #07: nsXMLHttpRequest::Send(nsIVariant*) (dom/base/nsXMLHttpRequest.cpp:2656) #08: MainThreadRun (dom/workers/XMLHttpRequest.cpp:1570) #09: nsRefPtr<mozilla::dom::workers::Proxy>::get() const (dom/workers/../../dist/include/nsRefPtr.h:209)
Assignee | ||
Comment 5•9 years ago
|
||
This fixes both tests. Basically we have to do exactly what we were doing before but we weren't handling multipart blobs before. Now we do!
Attachment #8547843 -
Attachment is obsolete: true
Attachment #8547888 -
Attachment is obsolete: true
Attachment #8548465 -
Flags: review?(khuey)
Reporter | ||
Comment 6•9 years ago
|
||
I can confirm that when built into a b2g-desktop build, the v2 patch makes the email app and its too-cool-for-mozilla-central back-end tests happy again. (And addressing the testing coverage issue by having the front-end test exercise all of these code-paths is a Q1 goal for me. So it will get done even if I have to break the email app to do it. ;)
Attachment #8548465 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1fac42ad172
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8548465 [details] [diff] [review] Patch, v2 Approval Request Comment [Feature/regressing bug #]: 701634 [User impact if declined]: Some uses of IndexedDB-on-Workers + BlobURL + XHR will crash [Describe test coverage new/current, TreeHerder]: Tests included here [Risks and why]: If the email app works then I think we're safe (it uses the most bizarre combination of this stuff that any web app is likely to use) [String/UUID change made/needed]: None
Attachment #8548465 -
Flags: approval-mozilla-beta?
Attachment #8548465 -
Flags: approval-mozilla-aurora?
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c1fac42ad172
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8548465 -
Flags: approval-mozilla-beta?
Attachment #8548465 -
Flags: approval-mozilla-beta+
Attachment #8548465 -
Flags: approval-mozilla-aurora?
Attachment #8548465 -
Flags: approval-mozilla-aurora+
Comment 10•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/eda8c6ec5121 This is hitting some rebase conflicts on beta due to bug 1113340. Not sure if we want to just uplift that or rebase around it?
Comment 13•9 years ago
|
||
Per an IRC convo with bent, this isn't needed after all because bug 701634 didn't land on Gecko 36. At his request, I've landed the tests, however. https://hg.mozilla.org/releases/mozilla-beta/rev/a6e5dedbd0c0
Updated•9 years ago
|
Attachment #8548465 -
Flags: approval-mozilla-beta+
You need to log in
before you can comment on or make changes to this bug.
Description
•