1188818 - [non-e10s] directory uploads: DnD event.dataTransfer.getFilesAndDirectories() returns File instead of Directory

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[silverwind]

Attachment: drag and drop test case

Followup to bug 1164310.

In the attached test case, when dropping a directory onto the page, it shows a [Object File], instead of an expected [Object Directory].

I'm seeing this on OS X 10.11, but expect it to be the same on all platforms.

Comment 1

[Kohei Yoshino]

I have implemented the Directory Upload API on my BzDeck app (https://www.bzdeck.com/) and can confirm the issue. The directory picker is working well.

Comment 2

[Jonathan Watt [:jwatt]]

Have you flipped any prefs to non-default values?

Comment 3

[silverwind]

Yeah, I have flipped a ton of prefs. I just tried in a fresh profile and it looks to be working fine.

Is there an easy way to dump all changed prefs? Any suggestion on which prefs could be interfering?

Comment 4

[silverwind]

Alright, I found it on first try: The bug only shows with e10s disabled.

Comment 5

[Kohei Yoshino]

Indeed, e10s was disabled on my profile. Enabled e10s with a clean profile and DnD uploads worked fine on my BzDeck app.

Comment 6

[Jonathan Watt [:jwatt]]

The short story is that this all hinges on what happens when DataTransfer::GetFilesAndDirectories calls DataTransfer::GetFiles to populate mFiles. If the QI to nsIFile in GetFiles succeeds, as is the case when e10s is disabled, things go wrong as GetFiles will then call File::CreateFromFile creating a new BlobImpl to wrap the nsIFile. This BlobImpl will have no knowledge of whether it represents a directory or not. On returning to GetFilesAndDirectories the |mFiles->Item(i)->Impl()->IsDirectory()| check evaluates to false and we fail to create a |new Directory|.

Comment 7

[Jonathan Watt [:jwatt]]

The following gives a more detailed description of the code flow that allows us to avoid this problem when e10s is enabled.

After the nsIDragSession is created on the Chrome process, the first thing to access its DataTransfer (creating it on-demand, then cached) happens to be when event.dataTransfer is accessed in:

https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml

  mozilla::dom::DataTransfer::DataTransfer
  mozilla::dom::DataTransfer::DataTransfer
  nsContentUtils::SetDataTransferInEvent
  mozilla::dom::DragEvent::GetDataTransfer
  mozilla::dom::DragEventBinding::get_dataTransfer

(Note the other DataTransfer ctor is called again to clone the first when SetDataTransferInEvent calls DataTransfer::Clone. This Clone may happen multiple times if multiple 'drag' events are fired, but the non-clone DataTransfer ctor doesn't seem to be called for these extra events so presumably the DataTransfer is being reused across events.)

The DataTransfer on the chrome side contains nsIFile contents, but an nsISupportsArray of BlobImplFile objects is created from these under:

  mozilla::dom::BlobImplFile::BlobImplFile
  nsContentUtils::TransferableToIPCTransferable
  nsContentUtils::TransferablesToIPCTransferables
  mozilla::dom::ContentParent::MaybeInvokeDragSession
  mozilla::EventStateManager::DispatchCrossProcessEvent
  mozilla::EventStateManager::HandleCrossProcessEvent
  mozilla::EventStateManager::PostHandleEvent
  PresShell::HandleEventInternal
  PresShell::HandlePositionedEvent
  PresShell::HandleEvent
  nsViewManager::DispatchEvent
  nsView::HandleEvent
  nsChildView::DispatchEvent
  nsBaseWidget::DispatchInputEvent

TransferableToIPCTransferable calls LookupAndCacheIsDirectory to set eIsDir on the new BlobImplFile as appropriate. On returning up to MaybeInvokeDragSession that calls PContentParent::SendInvokeDragSession to dispatch the blobimpls to the content process.

On the content process RemoteBlobImpl objects are created under:

  mozilla::dom::BlobChild::RemoteBlobImpl::RemoteBlobImpl
  mozilla::dom::BlobChild::CommonInit
  mozilla::dom::BlobChild::BlobChild
  mozilla::dom::BlobChild::BlobChild
  mozilla::dom::BlobChild* BlobChild::CreateFromParams<nsIContentChild>
  mozilla::dom::BlobChild::Create
  mozilla::dom::nsIContentChild::AllocPBlobChild
  mozilla::dom::ContentChild::AllocPBlobChild

Later ContentChild::RecvInvokeDragSession is called and calls |new DataTransfer| and adds the RemoteBlobImpl objects that have eIsDir set on them as appropriate. RecvInvokeDragSession then calls nsBaseDragService::SetDataTransfer to set the populated DataTransfer.

As a result of all this, when DataTransfer::GetFiles is called the QI to nsIFile fails and we enter the block where we QI to BlobImpl instead. We then create DOM File objects that wrap the BlobImpl objects that have eIsDir set as appropriate.

On returning to GetFilesAndDirectories the |mFiles->Item(i)->Impl()->IsDirectory()| check evaluates to true and we create a |new Directory| as we should.

Comment 8

[Jonathan Watt [:jwatt]]

The following gives a more detailed description of the code flow that results in the problem when e10s is disabled.

When DragEvent::GetDataTransfer is called there is no pre-existing DataTransfer so nsContentUtils::SetDataTransferInEvent creates a |new DataTransfer|:

  mozilla::dom::DataTransfer::DataTransfer
  nsContentUtils::SetDataTransferInEvent
  mozilla::dom::DragEvent::GetDataTransfer

It passes that object to nsBaseDragService::SetDataTransfer, but then calls DataTransfer::Clone and sets the clone on the drag event object.

When DataTransfer::GetFilesAndDirectories calls GetFiles we create the DOM File objects on-demand. While doing that we end up creating nsIFile objects under:

  NS_NewLocalFile
  nsDragService::GetData
  mozilla::dom::DataTransfer::FillInExternalData
  mozilla::dom::DataTransfer::MozGetDataAt
  mozilla::dom::DataTransfer::GetFiles
  mozilla::dom::DataTransfer::GetFilesAndDirectories
  mozilla::dom::DataTransferBinding::getFilesAndDirectories

(Because, on Mac in this case, nsDragService::GetData gets an NSArray of NSStrings (from the NSPasteboard) that are file paths.)

Just after the NS_NewLocalFile call the nsIFile is set on the nsTransferable by calling nsTransferable::SetTransferData.

Back up in DataTransfer::GetFiles we then find that the QI to nsIFile succeeds and as a result we call File::CreateFromFile creating a new BlobImpl to wrap the nsIFile. This BlobImpl doesn't know whether its nsIFile represents a directory or not so on returning to GetFilesAndDirectories the |mFiles->Item(i)->Impl()->IsDirectory()| check evaluates to false and we fail to create a |new Directory|.

Comment 9

[Jonathan Watt [:jwatt]]

This is a bit of a pain because we create the DOM File objects lazily on-demand on the main thread at the time that GetFiles/GetFilesAndDirectories is called. It makes sense to do this lazily since the page may not actually look at any files that are dragged into the window. However, the result is that with the code as it is we would need to synchronously hit the file system to determine whether we should create Directory objects or not. We don't want to do that on the main thread.

Comment 10

[Kohei Yoshino]

Fixing the title. I think this is a blocker for Bug 1188880, because e10s is disabled by default on the Beta and Release channels, and this issue happens only when e10s is disabled.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: no_dir.patch

On linux, debug build, non-e10s, we crash dragging a directory.
I don't see anything that prevent having a nsIFile directory there.

Then, we should know if we want to have FileList with Directories as we have in HTMLInputElement or not... this decision should be taken asap so that we don't have 2 different behavior in DataTransfer and in HTMLInputElement.

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 8735869 [details] [diff] [review]
no_dir.patch

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

Rather than removing this assert, can you fix it? I think maybe put something like this is the |if| block:

  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default,
             "nsIFile objects are not expected on the content process");

And maybe something like this is the |else| block:

  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Content,
             "BlobImpl objects are not expected on the default process");

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95a4888a3da1

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95a4888a3da1