625745 - filereader (file API) not working on linux, but working on Windows

Status: REOPENED

(Core :: Widget: Gtk, defect, P4)

Description

[beatofthedrum]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Firefox 4.0b8

With the above sample web page, I have tried using firefox 4 beta 8 on Windows and it works fine. I have also tried on a Ubuntu linux machine with firefox 3 and it works well, but when I try firefox 4 beta on Ubuntu it does not work - when I drag an image to display it does not display the image.

Reproducible: Always

Steps to Reproduce:
1.Access the URL http://html5demos.com/file-api
2. Drag a JPG image from your desktop onto the dashed box on the web page
3. If working you should see your image inside the dashed box
Actual Results:  
Image does not appear inside in the dashed box.

Expected Results:  
Image should appear inside dashed box after being dragged into it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Thanks for the bug report!

Could you possibly narrow down a regression range (e.g. which beta the problem first appeared in, for a start)?  If you don't have time to do that, I understand, but if you do it would be much appreciated.

Comment 2

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

I can reproduce this on fedora on tip.  This needs to be a hardblocker imo.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

That demo works here on 32 bit Fedora 13 using trunk build (Gecko/20110112)

Comment 4

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

Interesting.  I'm on 64 bit Fedora 13.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I'm using Gnome, if that matters.

Comment 6

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

This looks very recent. rev 60000 works for me.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I wonder if the problem is in filereader or drag-and-drop.

Comment 8

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

I bisected this to http://hg.mozilla.org/mozilla-central/rev/37e106e25d9c.  I didn't believe it at first, but I tried rev 60111 and 60112 three times each and this is the cause.

Comment 9

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

WARNING: NS_ENSURE_TRUE(aFile) failed: file /home/khuey/dev/mozilla-central3/con
tent/base/src/nsDOMFileReader.cpp, line 492
JavaScript error: , line 0: uncaught exception: [Exception... "Component returne
d failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMFileReader.readAsDa
taURL]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame
:: http://html5demos.com/file-api :: <TOP_LEVEL> :: line 45"  data: no]

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Hmm, does the bug depend on when cycle collection or garbage collection happens?

Comment 11

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

Possibly.  It seems to work intermittently even when "broken".  I imagine the changeset I gave in comment 8 just happened to be unlucky.

Comment 12

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

I think this is a widget issue, we don't appear to be getting anything from the drag session.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Kyle, can you take this, since you can reproduce it?

Comment 14

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

I'll see if I can find time.

Comment 15

[Neil Deakin]

I can reproduce this if I do it just after Firefox is started. The difference is that when it fails, the call to GetTargetDragData from within nsDragService::GetNumDropItems processes a second dragover event which gets all the data, and the original dragover event gets none.

It works if I disable processing drag events (or at least return early from TargetSetLastContext) while GetTargetDragData is being called.

I don't know if this is the same case for others.

Comment 16

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

Interesting.  It looks like the gtk drag impl is async and we spin the event loop while we wait?

I'll poke at Neil's suggestion tomorrow.

Comment 17

[Neil Deakin]

(In reply to comment #16)
> Interesting.  It looks like the gtk drag impl is async and we spin the event
> loop while we wait?

Yes, the retrieval of the data is asynchronous as it needs to wait for the data to be available from the application that was the source of the drag.

Comment 18

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

Attachment: NSPR Log of nsDragSession

Comment 19

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

Attachment: Prevent getting into a situation where we're firing a dragOver when there's a dragOver on the stack.

Comment 20

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

Comment on attachment 505267 [details] [diff] [review]
Prevent getting into a situation where we're firing a dragOver when there's a dragOver on the stack.

Neil's analysis of the problem is correct.  What happens is that we fire a dragOver and something inspects the contents of the dataTransfer which we don't have yet.  We spin up a nested event loop and continue doing stuff.  If we decide to fire another dragOver and we still haven't received the contents of the drag, we'll spin up another nested event loop.  If there's more than one dragOver on the stack, when we unwind things get screwy and break.

This patch causes us to bail early from nsWindow::OnDragMotionEvent if we're already dispatching a dragOver.

Comment 21

[Neil Deakin]

I wasn't intending that to be the actual fix. Doesn't it mean we just ignore a dragover event that we might want to receive?

Comment 22

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

(In reply to comment #21)
> I wasn't intending that to be the actual fix. Doesn't it mean we just ignore a
> dragover event that we might want to receive?

"Perhaps".  Per http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model:

"User agents must, as soon as the drag operation is initiated and every 350ms (±200ms) thereafter for as long as the drag operation is ongoing, queue a task to perform the following steps in sequence:

1.  If the user agent is still performing the previous iteration of the sequence (if any) when the next iteration becomes due, abort these steps for this iteration (effectively "skipping missed frames" of the drag-and-drop operation)."

That's basically what this patch does.

I suppose the alternative is to figure out why the drag impl is throwing away the data and prevent that, but firing multiple events on top of each other seems undesirable regardless.

Comment 23

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

I suppose if we do this though we should do the same to dragenter/leave.

Comment 24

[Neil Deakin]

(In reply to comment #22)
> "Perhaps".  Per
> http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model:
> 
> "User agents must, as soon as the drag operation is initiated and every 350ms
> (±200ms) thereafter for as long as the drag operation is ongoing, queue a task
> to perform the following steps in sequence:
> 
> 1.  If the user agent is still performing the previous iteration of the
> sequence (if any) when the next iteration becomes due, abort these steps for
> this iteration (effectively "skipping missed frames" of the drag-and-drop
> operation)."

I'm not really clear why any of that is relevant here. We're talking about lower level gtk drag and drop. We don't want to ignore a system dragover event do we?

Comment 25

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

Comment on attachment 505267 [details] [diff] [review]
Prevent getting into a situation where we're firing a dragOver when there's a dragOver on the stack.

I think I've found a better solution.

Comment 26

[Karl Tomlinson (:karlt)]

I think attachment 501602 [details] [diff] [review] and attachment 501603 [details] [diff] [review] would resolve this.
They don't deal with generic nested events, which I was planning to deal with in bug 497498, but they do resolve the problem with period dragover events.

Comment 27

[Karl Tomlinson (:karlt)]

> holder.ondragend = function () { this.className = ''; return false; };

I assume this is meant to be ondragleave.
ondragend is sent to the source and there typically is no source in this case.

Based on
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-model
I assume dragleave is preferred over dragexit, even though ondragleave is not mentioned at https://developer.mozilla.org/en/Drag_and_Drop.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So is this fixed now?

Comment 29

[Karl Tomlinson (:karlt)]

The fix in bug 495343 fixed this for me.

http://hg.mozilla.org/mozilla-central/rev/df375746f0cd
http://hg.mozilla.org/mozilla-central/rev/58d3e2cfdbd0

Comment 30

[[:Aleksej]]

Reported by [:chaos]; confirmed by myself with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre  (build ID 20110204030345)

The image does not appear, this error message appears in the console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMFileReader.readAsDataURL]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: http://html5demos.com/file-api :: <TOP_LEVEL> :: line 45"  data: no]

Comment 31

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

(In reply to comment #30)
> Reported by [:chaos]; confirmed by myself with Mozilla/5.0 (X11; Linux i686;
> rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre  (build ID 20110204030345)
> 
> The image does not appear, this error message appears in the console:
> 
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMFileReader.readAsDataURL]" 
> nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
> http://html5demos.com/file-api :: <TOP_LEVEL> :: line 45"  data: no]

I'm sorry, are you claiming this is still broken?

Comment 32

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

I can still reproduce this on tip.  (Anecdotally) it seems to happen less often but it still happens pretty frequently.

Comment 33

[Karl Tomlinson (:karlt)]

I haven't reproduced here, but GTK drag sources can (incorrectly) send a drop message before they have received a status reply from the position message.

If that happens we could end up with two selection requests in flight at the same time (one during dragover, and another during a nested drop event), which nsDragService manages badly.

As part of bug 497498, I'm working on queuing drag events that occur during processing of other drag events, which should prevent such a situation arising in this testcase.

Comment 34

[Karl Tomlinson (:karlt)]

Aleksej and/or Kyle, could you try one of the these builds please to see whether it resolves the issues you are seeing?
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3c09a259a595/tryserver-linux64/firefox-4.0b11pre.en-US.linux-x86_64.tar.bz2
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3c09a259a595/tryserver-linux/firefox-4.0b11pre.en-US.linux-i686.tar.bz2

If still not working properly, can you attach output from a run with NSPR_LOG_MODULES=WidgetDrag:5,nsDragService:5 in the environment, please?

Comment 35

[[:Aleksej]]

(In reply to comment #34)
> http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3c09a259a595/tryserver-linux/firefox-4.0b11pre.en-US.linux-i686.tar.bz2
> If still not working properly, can you attach output from a run with
> NSPR_LOG_MODULES=WidgetDrag:5,nsDragService:5 in the environment, please?

It is not working, and there is no new output.

Comment 36

[Karl Tomlinson (:karlt)]

Thanks Aleksej.  Do you still see (NS_ERROR_INVALID_POINTER) [nsIDOMFileReader.readAsDataURL]?
Is the failure intermittent for you or does it always fail?

Sorry about the lack of logging in that build.  (FORCE_PR_LOG was in the wrong place.)
This build has the additional logging:
http://stage.mozilla.org/pub/mozilla.org/
firefox/tryserver-builds/[e-mail]-8a1e416fa877/tryserver-linux/
firefox-4.0b12pre.en-US.linux-i686.tar.bz2

Comment 37

[[:Aleksej]]

Attachment: log with http://stage.mozilla.org/pub/mozilla.org/ firefox/tryserver-builds/[e-mail]-8a1e416fa877/tryserver-linux/ firefox-4.0b12pre.en-US.linux-i686.tar.bz2

(In reply to comment #36)
> Thanks Aleksej.  Do you still see (NS_ERROR_INVALID_POINTER)
> [nsIDOMFileReader.readAsDataURL]?

Yes, same.

> Is the failure intermittent for you or does it always fail?

Always.

Comment 38

[Karl Tomlinson (:karlt)]

(In reply to comment #37)
> Created attachment 512434 [details]
> log with ...

Thanks again.

It seems your desktop is providing a mime type of text/uri-list, which seems reasonable, but our code does not think that matches application/x-moz-file.

What window manager (or desktop program) do you have?

I seem to be getting lucky with my desktop providing the data in text/plain format:

> nsDragService::IsDataFlavorSupported application/x-moz-file
> checking text/uri-list against application/x-moz-file
> checking text/x-moz-url against application/x-moz-file
> checking text/plain against application/x-moz-file
> good! ( it's text plain and we're checking                    against text/unicode or application/x-moz-file)

Comment 39

[Karl Tomlinson (:karlt)]

The code that assumes the text/plain data is a uri-list seems incorrect and that comes from bug 229327.  Bug 229327 comment 17 seems to suggest that, at one stage, before that bug, text/uri-list was sufficient to allow file drags.

Kyle's intermittent issue must be something different to this text/plain mime type issue.

Comment 40

[Karl Tomlinson (:karlt)]

Providing only one text/uri-list mime type is appropriate for X11 file drags.
 http://www.newplanetsoftware.com/xdnd/dragging_files.html
 "in general, the source should not provide any types other than text/uri-list
  and text/x-xdnd-username. This is because anything on the file system can be
  dragged: plain text, HTML, images, binary executables, or even
  directories. The only type that encompasses all these is text/uri-list. All
  of these can even be dragged at the same time. It is the target's
  responsibility to sort out what was dropped and do something reasonable."

However, I think we need to be careful about converting uri-list to file
because uri-list is currently also what is used at the system level for dragging URLs from anchors, and the user may think they are only dragging a link.  For example a user may drag a URL in a web page while being unaware that the URL points to a local file.
Dropping that link on a web page should not (necessarily) provide the page with the local file.

The basic question is: how can we distinguish file drags from URL drags?

A similar issue was discussed in bug 564738, but for external drags like this
we can't check the principal of the document in the other application.

Treating uri-list as file for all external drags is not right if another app (e.g. mail user agent) presents URL drags as uri-lists.

I wondered about using the list of allowed actions.

The xdnd spec suggests a special default drag action:
 "When dragging files from other programs (e.g. a list of files to search or
  compile), XdndActionPrivate should be the default because it isn't clear
  what the user intends to do."
However, even if file managers are correctly doing that, GDK removes the
distinction by converting XdndActionPrivate to GDK_ACTION_COPY.

It seems that when dragging a URL the link action should be allowed but the
copy action should not.  Currently we allow copy for link drags, perhaps
because we also provide the link in text/plain, text/html (I wonder why?),
etc.  I guess we want to continue to provide text/plain and the text/plain
data needs to be copied, not linked.  Unfortunately, we can't provide different
allowed actions for each mime type. 

Given uri-list is the means of indicating file drags on X11, maybe we should
not add uri-list to the list of mime types for drags that we initiate when
kURLMime is specified but kFileMime is not and the list includes "file:" uris.