794619 - crash when storing a file returned by DeviceStorage.enumerate() into IndexedDB

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[David Flanagan [:djf]]

Attachment: reduced test case

If I call use DeviceStorage.enumerate() to get a list of pictures (on desktop or device) and then take the file and attempt to store it in an indexed db, gecko crashes.

Instead of storing the file directly, I can workaround the crash by using file.slice(0, file.size, file.type). Storing that blob prevents the crash.

The attachment is a patch that you can apply to Gaia to turn the Template app into a test case.  Just click on the template icon and you'll get a crash on either desktop or otoro.

Comment 1

[David Flanagan [:djf]]

Possibly the same underlying bug as https://bugzilla.mozilla.org/show_bug.cgi?id=792307

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pretty sure this is bug 792307, but we can do the work here with the nice small testcase.

bent, assigning to you just because bug 792307 is.  We can horse trade later on.

Comment 3

[Doug Turner (:dougt)]

child process crash:

#0  getpid () at bionic/libc/arch-arm/syscalls/getpid.S:11
#1  0x40c01c3e in base::MessagePumpLibevent::ScheduleWork (this=0x837b98) at /builds/mozilla-central/ipc/chromium/src/base/message_pump_libevent.cc:362
#2  0x40bf0d4a in MessageLoop::PostTask_Helper (this=0x100ffdf4, from_here=<value optimized out>, task=<value optimized out>, delay_ms=0, nestable=<value optimized out>)
    at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:297
#3  0x40bf0d88 in MessageLoop::PostTask (this=<value optimized out>, from_here=<value optimized out>, task=<value optimized out>) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:241
#4  0x40aee42c in mozilla::ipc::AsyncChannel::ProcessLink::SendMessage (this=<value optimized out>, msg=0xe45378) at /builds/mozilla-central/ipc/glue/AsyncChannel.cpp:191
#5  0x40aee99e in mozilla::ipc::AsyncChannel::Send (this=0x837988, _msg=0xe45378) at /builds/mozilla-central/ipc/glue/AsyncChannel.cpp:428
#6  0x40af352a in mozilla::ipc::RPCChannel::Send (this=0x837988, msg=0xe45378) at /builds/mozilla-central/ipc/glue/RPCChannel.cpp:110
#7  0x40b45ce4 in mozilla::dom::PBlobStreamChild::Send__delete__ (actor=0xe44e70, params=...) at /builds/mozilla-central/objdir-gonk/ipc/ipdl/PBlobStreamChild.cpp:100
#8  0x40ad7f38 in mozilla::dom::ipc::Blob<(mozilla::dom::ipc::ActorFlavorEnum)1>::RecvPBlobStreamConstructor (this=<value optimized out>, aActor=0xe44e70) at /builds/mozilla-central/dom/ipc/Blob.cpp:971
#9  0x40b45624 in mozilla::dom::PBlobChild::OnMessageReceived (this=0xd46b30, __msg=<value optimized out>) at /builds/mozilla-central/objdir-gonk/ipc/ipdl/PBlobChild.cpp:352
#10 0x40b4f392 in mozilla::dom::PContentChild::OnMessageReceived (this=0x837980, __msg=...) at /builds/mozilla-central/objdir-gonk/ipc/ipdl/PContentChild.cpp:2009
#11 0x40aeea38 in mozilla::ipc::AsyncChannel::OnDispatchMessage (this=0x837988, msg=...) at /builds/mozilla-central/ipc/glue/AsyncChannel.cpp:473
#12 0x40af3862 in mozilla::ipc::RPCChannel::OnMaybeDequeueOne (this=0x837988) at /builds/mozilla-central/ipc/glue/RPCChannel.cpp:402
#13 0x40ad85b6 in DispatchToMethod<mozilla::dom::ContentParent, void (mozilla::dom::ContentParent::*)()> (this=<value optimized out>) at /builds/mozilla-central/ipc/chromium/src/base/tuple.h:383
#14 RunnableMethod<mozilla::dom::ContentParent, void (mozilla::dom::ContentParent::*)(), Tuple0>::Run (this=<value optimized out>) at /builds/mozilla-central/ipc/chromium/src/base/task.h:307
#15 0x40af2218 in mozilla::ipc::RPCChannel::RefCountedTask::Run (this=<value optimized out>) at ../../dist/include/mozilla/ipc/RPCChannel.h:425
#16 mozilla::ipc::RPCChannel::DequeueTask::Run (this=<value optimized out>) at ../../dist/include/mozilla/ipc/RPCChannel.h:448
#17 0x40befdf0 in MessageLoop::RunTask (this=0xbea4e93c, task=0xbea4df4c) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:326
#18 0x40bf0c1a in MessageLoop::DeferOrRunPendingTask (this=0x837988, pending_task=<value optimized out>) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:334
#19 0x40bf17f8 in MessageLoop::DoWork (this=0xbea4e93c) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:434
#20 0x40af1bd4 in mozilla::ipc::DoWorkRunnable::Run (this=<value optimized out>) at /builds/mozilla-central/ipc/glue/MessagePump.cpp:42
#21 0x40bcfe52 in nsThread::ProcessNextEvent (this=0x8456f8, mayWait=<value optimized out>, result=0xbea4e027) at /builds/mozilla-central/xpcom/threads/nsThread.cpp:612
#22 0x40bb080e in NS_ProcessNextEvent_P (thread=0x837988, mayWait=false) at /builds/mozilla-central/objdir-gonk/xpcom/build/nsThreadUtils.cpp:220
#23 0x40af1ce4 in mozilla::ipc::MessagePump::Run (this=0x833f68, aDelegate=0xbea4e93c) at /builds/mozilla-central/ipc/glue/MessagePump.cpp:82
#24 0x40af1d96 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x833f68, aDelegate=0xbea4e93c) at /builds/mozilla-central/ipc/glue/MessagePump.cpp:231
#25 0x40befda0 in MessageLoop::RunInternal (this=0x1000000) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:208
#26 0x40befe56 in MessageLoop::RunHandler (this=0xbea4e93c) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
#27 MessageLoop::Run (this=0xbea4e93c) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:175
#28 0x40a7e4b4 in nsBaseAppShell::Run (this=0xa36878) at /builds/mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:163
#29 0x404215c8 in XRE_RunAppShell () at /builds/mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:646
#30 0x40af1d64 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x833f68, aDelegate=0xbea4e93c) at /builds/mozilla-central/ipc/glue/MessagePump.cpp:198
#31 0x40befda0 in MessageLoop::RunInternal (this=0xa36878) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:208
#32 0x40befe56 in MessageLoop::RunHandler (this=0xbea4e93c) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:201
#33 MessageLoop::Run (this=0xbea4e93c) at /builds/mozilla-central/ipc/chromium/src/base/message_loop.cc:175
#34 0x4042196e in XRE_InitChildProcess (aArgc=<value optimized out>, aArgv=<value optimized out>, aProcess=GeckoProcessType_Content) at /builds/mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:485
#35 0x00008450 in main (argc=5, argv=0xbea4ea94) at /builds/mozilla-central/ipc/app/MozillaRuntimeMain.cpp:48


It looks like:

DeviceStorageRequestChild::Recv__delete__() is called.
We take blob and create a jsval using it (WrapNative())
Then we pass that into script.

Comment 4

[Doug Turner (:dougt)]

Unable to duplicate the problem on mac or linux desktop.  gc zeal revealed nothing.


if I take a slice of the blob, the crash goes away:

   var copy = f.slice();
   var req = store.add({ name: f.name, file: copy });


I also reduce the test case a bit.  If i just reuse the blob just within device storage, I continue to see the crash.  That is:

create a blob in script
store that blob using device storage
get the blob from device storage
storage the blob again (under a different name) using device storage <---- crashes

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 6

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

Comment on attachment 666003 [details] [diff] [review]
patch v.1

Review of attachment 666003 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +935,5 @@
>      if (!typeChecker->Check(cursorStorageType, file->mFile)) {
>        continue;
>      }
> +
> +    if (XRE_GetProcessType() != GeckoProcessType_Default) {

So, I think we should move this to nsDOMDeviceStorageCursor::Continue(). Otherwise we're doing two asynchronous bounces when we really only want one.

@@ +946,5 @@
> +
> +      PDeviceStorageRequestChild* child = new DeviceStorageRequestChild(mRequest, file);
> +      DeviceStorageGetParams params(cursorStorageType, file->mPath, fullpath);
> +      ContentChild::GetSingleton()->SendPDeviceStorageRequestConstructor(child, params);
> +      cursor->mOkToCallContinue = true;

It's not actually ok to call continue again yet. You've got to wait for the response first.

@@ +949,5 @@
> +      ContentChild::GetSingleton()->SendPDeviceStorageRequestConstructor(child, params);
> +      cursor->mOkToCallContinue = true;
> +      return NS_OK;
> +    }
> +    

Nit: trailing whitespace.

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 8

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

Comment on attachment 666587 [details] [diff] [review]
patch v.2

Review of attachment 666587 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these changes.

::: dom/devicestorage/DeviceStorageRequestChild.h
@@ +17,5 @@
>  
> +class DeviceStorageRequestChildCallback
> +{
> +  public:
> +    virtual void RequestComplete(class DeviceStorageRequestChild* aRequest) = 0;

You never use the request here, just make this take no args?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +750,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(aWindow, "Null Window");
>  
> +  if (aFile == nullptr) {

Nit: !aFile

@@ +912,5 @@
>  {
>  }
>  
> +void
> +ContinueCursorEvent::GetNextFile(DeviceStorageFile** aFile)

Can you make this just return already_AddRefed<DeviceStorageFile> instead? That would make this much cleaner.

@@ +960,5 @@
> +
> +  nsString fullpath;
> +  nsresult rv = file->mFile->GetPath(fullpath);
> +  if (NS_FAILED(rv)) {
> +    // silently fail

Won't this mean that you never deliver a request event? I'd just assert or handle it like being done with enumeration (NS_DispatchToMainThread).

@@ +971,5 @@
> +
> +  DeviceStorageRequestChild* child = new DeviceStorageRequestChild(mRequest, file);
> +  child->SetCallback(cursor);
> +  DeviceStorageGetParams params(cursorStorageType, file->mPath, fullpath);
> +  ContentChild::GetSingleton()->SendPDeviceStorageRequestConstructor(child, params);

After this succeeds you could null out mRequest since you've effectively transferred it to the actor.

@@ +978,5 @@
> +NS_IMETHODIMP
> +ContinueCursorEvent::Run()
> +{
> +  nsRefPtr<DeviceStorageFile> file;
> +  GetNextFile(getter_AddRefs(file));

You should probably assert here that file is always null if you're in a child process.

@@ +1180,5 @@
>  
> +void
> +nsDOMDeviceStorageCursor::RequestComplete(DeviceStorageRequestChild* aRequest)
> +{
> +  mOkToCallContinue = true;

Maybe assert first that mOkToCallContinue was false.

Comment 9

[Doug Turner (:dougt)]

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

Comment 10

[Ed Morley [:emorley]]

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