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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: 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))

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

er, and note that the patch involves an "hg cp" operation, so it might look a little confusing in bugzilla's diff views, etc.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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! :)

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Attachment: 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)

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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!

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

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. ;)

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1fac42ad172

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1fac42ad172

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, we should uplift that too

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/eda8c6ec5121

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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