Closed Bug 594964 Opened 9 years ago Closed 9 years ago

nsDOMDataTransfer creates nsDOMFiles with no document

Categories

(Core :: Drag and Drop, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sgreenlay, Assigned: khuey)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached file Test Case
See test case.

Under FF3.6 dragging multiple files onto 'Drop Here' will pass all files to handleFiles(), under trunk it only passes the first file
blocking2.0: --- → ?
Keywords: regression
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd13b6ce36bd&tochange=67aef7ffb282

I'll bet money this is a regression from bug 583863.  And that this should block b6.
Blocks: 583863
Uh oh!  I'll take a look at this today.  Shame we didn't catch this with tests :-(
This has nothing to do with multiple files:

Error: uncaught exception: [Exception... "Access to restricted URI denied"  code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)"  location: "https://bug594964.bugzilla.mozilla.org/attachment.cgi?id=473754 Line: 121"]
Assignee: nobody → khuey
Keywords: testcase
OS: Mac OS X → All
Hardware: x86 → All
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/dev/mo
zilla-central3/obj-i686-pc-mingw32/netwerk/base/src/../../../../netwerk/base/src
/nsIOService.cpp, line 610
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/dev/mo
zilla-central3/obj-i686-pc-mingw32/content/base/src/../../../../content/base/src
/nsDOMFileReader.cpp, line 516
JavaScript error: , line 0: uncaught exception: [Exception... "Access to restric
ted URI denied"  code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)"  loc
ation: "https://bug594964.bugzilla.mozilla.org/attachment.cgi?id=473754 Line: 12
1"]
nsFileDataProtocolHandler::NewChannel is failing because it can't find the information it needs to create a new channel.
Confirmed that if I back out 583863 this is fixed.
All 583863 did was expose this by using the URL to implement the FileReader.  The real issue is that nsDOMDataTransfer creates files with a null document.  We don't register the file with the mozfiledata protocol handler when the document is null because nothing will be around to unregister us (we really should just throw NS_ERROR_FAILURE instead).

This may or may not be a regression, I haven't tested anything but my 583863-less build yet.
Summary: REGRESSION: Multiple file drop gives only first file → nsDOMDataTransfer creates nsDOMFiles with no document
Attached patch Patch (obsolete) — Splinter Review
This works, I'm just not sure whether or not it's safe.  The idea is that if the drag originates from outside Gecko we make the owning document the current document.  This leads us both to actually register the URI and give it a principal that makes sense.  I also cleaned up some weakref stuff that's really not necessary since a DOMFile should never outlive its parent document.
Attachment #474416 - Flags: superreview?(bzbarsky)
Attachment #474416 - Flags: review?
blocking2.0: ? → betaN+
This is probably responsible for the drag and drop test at http://studio.html5rocks.com/ not working on trunk.
I don't understand. Why doesn't the target document work?

Also, what guarentees that the nsDOMFile outlives its document?
targetDoc doesn't work because mDragTarget is always null if the file came from outside of Gecko.  This is essentially Bug 572139, just exposed differently because FileReader is now implemented using File.url.

It's possible I'm wrong about the nsDOMFile outliving its document, but as I understand it the only things that can keep it alive are references from script and the owning reference that nsFileDataProtocolHandler holds when a url is asked for.  Those owning references are freed when the document is destroyed.  I can remove that part of the patch if you like.
Actually, MXR leads me to believe that mDragTarget is *always* null.  I'll investigate how internal drops function.
Comment on attachment 474416 [details] [diff] [review]
Patch

Clearing reviews until I can investigate further.
Attachment #474416 - Flags: superreview?(bzbarsky)
Attachment #474416 - Flags: review?(jonas)
(In reply to comment #12)
> targetDoc doesn't work because mDragTarget is always null if the file came from
> outside of Gecko.  This is essentially Bug 572139, just exposed differently
> because FileReader is now implemented using File.url.

Oh, I guess I was thinking of the event-target, not the drop target. I.e. the node that the drop event is fired at. Can we get to the event from data-transfer?

> ...Those owning references are freed when the document is destroyed.

Where? The only thing I know of that is freed when the document is destroyed is the entries in the url->file hash. But that doesn't affect the nsDOMFile object.

> I can remove that part of the patch if you like.

I would prefer that yes.
Yeah, this is broken for an internal drag too.  Try dragging from the download manager to the test case.  mDragTarget is always null.  It appears that when nsDOMDataTransfer was implemented nobody knew what mDragTarget and AddElement was supposed to do ...
Attached patch Patch (obsolete) — Splinter Review
I'll write a test for this later today.
Attachment #474416 - Attachment is obsolete: true
Attachment #475540 - Flags: review?(jonas)
I meant to s/sourceDoc/doc/, consider that done.
Testing this is proving to be more difficult than I expected, probably won't get to it till this weekend.
I'm still wondering about the question in comment 15:

Can we get to the event from data-transfer?
Nope, everything dealing with events is handled in nsEventStateManager and not passed to the nsDOMDataTransfer.
The only reason we ever want the document at all is because the document is responsible for unregistering URIs that we gave to scripts through file.url.  What if instead of doing this we simply create the file objects with a nsIPrincipal reference instead of a nsIDocument reference and then in nsDOMFile::GetUrl() we can call GetDocumentFromCaller() to figure out who should unregister the url.
Actually, file.url exists no more. But we still use the document to figure out a principal for the url. But we can change that by making turning internalUrl into a function and make it take an nsIDocument or nsIPrincipal parameter.
Attachment #475540 - Attachment is obsolete: true
Attachment #475540 - Flags: review?(jonas)
Comment on attachment 476446 [details] [diff] [review]
: Make nsDOMFile's principal interact properly with drag and drop.

Please add an NS_ENSURE_STATE or similar check to make sure we bail if aPrincipal is null in GetInternalUrl.

r=me with that.
Attachment #476446 - Flags: review?(jonas) → review+
http://hg.mozilla.org/mozilla-central/rev/a660900a793f
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Verified in yesterday's nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.