Closed Bug 794619 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: djf, Assigned: dougt)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Possibly the same underlying bug as https://bugzilla.mozilla.org/show_bug.cgi?id=792307
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.
Assignee: nobody → bent.mozilla
Blocks: 792307
blocking-basecamp: --- → +
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.
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
Attached patch patch v.1 (obsolete) — Splinter Review
Assignee: bent.mozilla → doug.turner
Attachment #666003 - Flags: review?(bent.mozilla)
Component: DOM: IndexedDB → DOM: Device Interfaces
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.
Attachment #666003 - Flags: review?(bent.mozilla) → review-
Attached patch patch v.2Splinter Review
Attachment #666003 - Attachment is obsolete: true
Attachment #666587 - Flags: review?(bent.mozilla)
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.
Attachment #666587 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/d7c8896af07a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.