Closed Bug 1278939 Opened 4 years ago Closed 3 years ago

DataTransfer::mozGetDataAt("application/x-moz-file", 0) returns File versus nsIFile

Categories

(Core :: DOM: Drag & Drop, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- fixed

People

(Reporter: enndeakin, Assigned: Nika)

References

Details

(Keywords: regression, Whiteboard: btpp-active)

Attachments

(2 files, 2 obsolete files)

Bug 906420 made this return DOM File objects rather than nsIFile objects. Since existing code expects these to be nsIFiles, various code doesn't work.

One case where this doesn't work:

1. Open data:text/html,<body contenteditable> in non-e10s window
2. Drag image to the content area

Expected: the image to appear
Actual: nothing happens
Assuming this is a regression from 906420.
Depends on: 906420
Flags: needinfo?(michael)
Keywords: regression
This also fixes some issues with the original patch related to hiding strings when dragging file data when in e10s mode, as well as preventing crashes when the clipboard changes between the data transfer being created, and used.
Attachment #8764969 - Flags: review?(enndeakin)
Assignee: nobody → michael
Flags: needinfo?(michael)
Whiteboard: btpp-active
Attachment #8764969 - Attachment is obsolete: true
Attachment #8764969 - Flags: review?(enndeakin)
As you requested, I've rebased the patches onto master. If you want, you can also just pull the entire tree from github here: https://github.com/mystor/mozilla-central/commits/bug906420
Comment on attachment 8766013 [details] [diff] [review]
Store nsIFile entries as nsIFile, but continue to produce dom::File objects from relevant APIs

>     }
>-  } else if (nsCOMPtr<BlobImpl> blobImpl = do_QueryInterface(aSupports)) {
>-    MOZ_ASSERT(blobImpl->IsFile());
>-    file = File::Create(GetParentObject(), blobImpl);
>+    MOZ_ASSERT(mFileCache, "File should be created successfully");

Should this be an assertion? Is it possible for this to fail from normal use?

>+
>+  // File cache for nsIFile application/x-moz-file entries.
>+  RefPtr<File> mFileCache;


Since this only supports one file, it would be clearer if this was called mCachedFile (and the comment updated).


I get an assertion on the testcase I am about to attach:
 Assertion failure: oldKind == Kind() (Kind should not have changed after setting data), at /builds/moz2/working/dom/events/DataTransferItem.cpp:223
#01: mozilla::dom::DataTransferItem::FillInExternalData()[/builds/moz2/working/output/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3a98775]

Otherwise I did some extensive testing and don't see other issues, (although I did find some unrelated to this patch).
Attachment #8766013 - Flags: review?(enndeakin) → review-
Attached file dragorclip-simple.xul
Cut and paste between the two areas caused the assertion mentioned in the previous comment. Needs to be privileged to cause this.
(In reply to Neil Deakin from comment #6)
> Created attachment 8766432 [details]
> dragorclip-simple.xul
> 
> Cut and paste between the two areas caused the assertion mentioned in the
> previous comment. Needs to be privileged to cause this.

I believe that the problem in this case is that you are trying to store an nsIFile on a clipboard dataTransfer.

This means that we put the nsIFile into a transferrable, which gets stored, and tell the OS that we have some data of type x-moz-file. 

When we paste, we look at the MIME type, and see an x-moz-file, which we think means that we have a file, so our type predictor adds an DataTransferItem of type FILE. Later, when we try to get the actual data from the OS, the OS calls back to us. We extract the nsIFile from the transfer, and then try to get out some primitive data from it, with nsPrimitiveHelpers::CreateDataFromPrimitive, which I believe fails, and thus the data never gets sent back. This means that we didn't get a file, so the type changes.

If you think this should not be an assertion, I'm fine with that, but I'm not sure we can make it work with nsIFiles easily. What do you think?
Flags: needinfo?(enndeakin)
Shouldn't the data from GetTransferData be null in that case?
Flags: needinfo?(enndeakin)
This patch no longer fails when we get data of an unexpected type from the OS, and also renames mFileCache to mCachedFile.
Attachment #8767707 - Flags: review?(enndeakin)
Attachment #8766013 - Attachment is obsolete: true
Attachment #8767707 - Flags: review?(enndeakin) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0aef4586bd
Store nsIFile entries as nsIFile, but continue to produce dom::File objects from relevant APIs, r=enndeakin
https://hg.mozilla.org/mozilla-central/rev/3d0aef4586bd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Version: unspecified → Trunk
Depends on: 1289900
Depends on: 1306645
You need to log in before you can comment on or make changes to this bug.