If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

XHR read of sliced (IndexedDB) Blob from worker thread hangs / triggers assertion

RESOLVED FIXED in Firefox 37, Firefox OS v2.2

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: asuth, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

Trunk
mozilla38
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 unaffected, firefox37 fixed, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8547435 [details] [diff] [review]
blob-worker-slice-test-v1.diff

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

3 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.
Created attachment 8547843 [details] [diff] [review]
Patch, v1
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

3 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

3 years ago
Created attachment 8547888 [details] [diff] [review]
mochitest to reproduce the XHR POST of a multi-file blob hang/assertion

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)
Created attachment 8548465 [details] [diff] [review]
Patch, v2

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

3 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1fac42ad172
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?
https://hg.mozilla.org/mozilla-central/rev/c1fac42ad172
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
Attachment #8548465 - Flags: approval-mozilla-beta?
Attachment #8548465 - Flags: approval-mozilla-beta+
Attachment #8548465 - Flags: approval-mozilla-aurora?
Attachment #8548465 - Flags: approval-mozilla-aurora+
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?
status-firefox37: affected → fixed
Flags: needinfo?(bent.mozilla)
Keywords: branch-patch-needed
Yeah, we should uplift that too
Flags: needinfo?(bent.mozilla)
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/eda8c6ec5121
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Keywords: branch-patch-needed
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
status-firefox36: affected → unaffected
Attachment #8548465 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.