594964 - nsDOMDataTransfer creates nsDOMFiles with no document

Status: VERIFIED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Scott Greenlay [:sgreenlay]]

Attachment: 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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Uh oh!  I'll take a look at this today.  Shame we didn't catch this with tests :-(

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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"]

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Confirmed that if I back out 583863 this is fixed.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is probably responsible for the drag and drop test at http://studio.html5rocks.com/ not working on trunk.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand. Why doesn't the target document work?

Also, what guarentees that the nsDOMFile outlives its document?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Actually, MXR leads me to believe that mDragTarget is *always* null.  I'll investigate how internal drops function.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 474416 [details] [diff] [review]
Patch

Clearing reviews until I can investigate further.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 ...

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

I'll write a test for this later today.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I meant to s/sourceDoc/doc/, consider that done.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Testing this is proving to be more difficult than I expected, probably won't get to it till this weekend.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm still wondering about the question in comment 15:

Can we get to the event from data-transfer?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Nope, everything dealing with events is handled in nsEventStateManager and not passed to the nsDOMDataTransfer.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: : Make nsDOMFile's principal interact properly with drag and drop.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/a660900a793f

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Verified in yesterday's nightly.