514148 - Thundirbird 3 AND Firefox freeze when you drag a mail attachment over a Firefox Window

Status: RESOLVED FIXED

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

Description

[Sylvain Spinelli]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729) XPCOMViewer/0.9a
Build Identifier: 

If you drag a mail attachment over a firefox window, Thunderbird and Firefox freeze and Firefox memory grows indefinitely.

Tested on Windows XP with Firefox 3.5.2 and :
- thunderbird/nightly/2009-09-01-15-comm-central-trunk 
- thunderbird/nightly/2009-09-01-11-comm-1.9.1/
- Xulrunner 1.9.1 (with my own application Scenari)

Problem does not appear on Linux.

Reproducible: Always




With 1.9.1 XulRunner, in my code (similar to thunderbird http://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#1837), my DataProvider.getFlavorData() method is nether called by the framework. This method is called in thunderbird 2.0.

Comment 1

[Phil Lacy]

I only get this when dragging over the tabs. Especially noticeable since I have treestyle addon working because of the big tab area, but does same even if addon is disabled and I drag over a tab or tab area.

Try dragging and avoid the tabs and see if it is a problem. Then try just over a tab.
I have 3.5.3 and build shredder to tip as of yesterday.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090912 Shredder/3.0b4pre

Comment 2

[Phil Lacy]

bug 227305 shows evidence of this problem.

Although the reporter hasn't confirmed comment #1, I'm going to mark this confirmed.

I think it needs to block 1.9.1 as well.

Comment 3

[Phil Lacy]

Attachment: insanity check due to faulty widget dnd

This is a needed insanity check since the return value is undefined under certain conditions. And this function has ff depending on calls to external app's implementation of IdataObject.

However this problem occurred because our own implementation is broke (TB3 1.9.1).
If my patch 497797 and all its depending patches are in place then this insanity check never hits and doesn't have a problem with our own apps.

Roc maybe you can steer this to correct reviewers. However you may note the implications of this bug and other dnd bugs mentioned that are not landed yet.

Comment 4

[Phil Lacy]

(In reply to comment #3)
> If my patch 497797 and all its depending patches are in place then this

bug 497797

Comment 5

[David :Bienvenu]

Thunderbird would really like to add drag drop of messages to the desktop for 3.0, but we'd like to know that Firefox is going to fix its bug first, so we don't mess up Firefox users. Is it likely this or the other fixes will make it into a Firefox point release?

Comment 6

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

Which bug is the bug that causes GetData to fail here?

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 403002 [details] [diff] [review]
insanity check due to faulty widget dnd

Approved for 1.9.1.4, a=dveditz

Comment 8

[David :Bienvenu]

landed for 1.9.1.4

Comment 9

[David :Bienvenu]

Phil, should we mark this fixed, or is there something we should land on the trunk and/or 1.9.2?

Comment 10

[Phil Lacy]

I'm not sure if this fix is entirely correct. Before the patch the count of dropped items was undefined and therefore it was hanging because it returned some huge number.

Now the patch says if we check the drag data source if it has a file flavor and it does then we do a getdata and then use that to see number of files.

One problem if it does proceed to getdata. TB (or others) may return an error and now with the patch assumes there is only one. But 0 files would do the same thing and may be more appropriate.

But another problem is DragQueryFileW() get number of files of a successful drop and this function is called on a dragover or whenever. Once dropped I don't think there is a Idataobject. So I'm not sure what DragQueryFileW() would actually return if it ever gets hit.

I'd say check this in to trunk because we can't have an undefined count being returned regardless of how broken the function is.

Comment 11

[Phil Lacy]

(In reply to comment #6)
> Which bug is the bug that causes GetData to fail here?

bug 491239 patch can also fix this particular bug. But I posted a comment there why that isn't a solution here. However the patch demonstrates why this bug causes GetData to fail. We assign CF_HDROP to kfilemime and kfilepromisemime but do not implement them in GetData.

The other bug(s) in a tree starting with bug 497797 implement dragging kfilemime and also makes temp files for kfilepromisemime both when internal is acting as a drag source.

kfilemime would be useful in dragging files out of the downloadmanagerUI. Or in other xul apps. kfilepromisemime would be useful for dragging attachments or TB messages direct to editors or other foreign apps. Or in other xul apps. Nothing urgent for TB I don't think.

Comment 12

[David :Bienvenu]

Phil, should this fix go into 1.9.2? I believe that's what current versions of Firefox use (it's hard to keep track...)

Comment 13

[Phil Lacy]

yes. (In reply to comment #12)

Comment 14

[Phil Lacy]

maybe I shouldn't change status when we have request flags out yet, sorry

Comment 15

[David :Bienvenu]

I've landed this fix on the trunk to bake a bit before getting 1.9.2 approval. Resolving fixed since its landed on the trunk.

Comment 16

[Samuel Sidler (old account; do not CC)]

This needs to land on 1.9.2 since it's landed on trunk and 1.9.1 now. Nominating for blocking so it doesn't fall off the radar.

Comment 17

[Phil Lacy]

(In reply to comment #6)
> Which bug is the bug that causes GetData to fail here?
Bug 491239 is the principle cause of GetData to fail.

kFilePromiseMime loads the windows format CF_HDROP into the transferdata and is dragged across other apps windows who then call GetData for CF_HDROP which we do not support for that flavor.

My other bugs mentioned will support GetData for that flavor but 491239 will remove that problem here and bring our IDataObject up to conformance until we decide to support those flavors.

That bug is r=- but I don't know what else to fix on it.

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 403002 [details] [diff] [review]
insanity check due to faulty widget dnd

bienvenu, this is a 1.9.2 blocker, so please land asap (no need for additional approval for 1.9.2 blockers yet).

Comment 19

[David :Bienvenu]

landed for 1.9.2 - I set status to final-fixed; not sure if that's quite right.

Comment 20

[Samuel Sidler (old account; do not CC)]

(In reply to comment #19)
> landed for 1.9.2 - I set status to final-fixed; not sure if that's quite right.

It is.