Last Comment Bug 629253 - Firefox hangs when dragging two emails from Thunderbird over it
: Firefox hangs when dragging two emails from Thunderbird over it
Status: RESOLVED FIXED
: hang, regression
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows XP
: -- critical with 1 vote (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
: 631228 650888 (view as bug list)
Depends on:
Blocks: 650888
  Show dependency treegraph
 
Reported: 2011-01-26 17:41 PST by Markus Fischer
Modified: 2011-11-09 14:30 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WinDbg log (20.96 KB, application/x-zip)
2011-01-26 18:16 PST, Alice0775 White
no flags Details
Patch for Firefox hang when dragging multiple emails over (1.41 KB, patch)
2011-09-02 18:54 PDT, Brian R. Bondy [:bbondy]
khuey: review+
Details | Diff | Review
Patch for Firefox hang when dragging multiple emails over. v2. (2.25 KB, patch)
2011-09-08 12:54 PDT, Brian R. Bondy [:bbondy]
netzen: review+
bugzilla: approval‑mozilla‑aurora-
Details | Diff | Review

Description Markus Fischer 2011-01-26 17:41:29 PST
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10

When I select two emails in Thunderbird and drag them over Firefox, it hangs.

Reproducible: Always

Steps to Reproduce:
1. Open Thunderbird, select any two emails from a list view
2. Open Firefox, empty tab is fine
3. Start a drag of the two emails and move the mouse over Firefox. A drop is not necessary, from that point on, FF never responds and I've to kill the process
4. Thunderbird hangs as well, but once I kill Firefox, Thunderbird continues working
Actual Results:  
Firefox hangs

Expected Results:  
Firefox should not icon

The mouse cursors icon indicates that a drop is not possible, nevertheless Firefox hangs.

Killing Thunderbird instead of Firefox leaves Firefox hanging.
Comment 1 Markus Fischer 2011-01-26 17:42:18 PST
s/Firefox should not icon/Firefox should not hang/
Comment 2 Alice0775 White 2011-01-26 18:16:18 PST
Created attachment 507335 [details]
WinDbg log


Confirmed.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11pre) Gecko/20110126 Firefox/4.0b11pre ID:20110126064706
and
Mozilla/5.0 (Windows; U; Windows NT 6.1; ja-JP; rv:1.9.2.13) Gecko/20101207 Thunderbird/3.1.7 ID:20101207091341
Comment 3 Alice0775 White 2011-01-27 08:24:19 PST
Regression window;
Works
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b1pre) Gecko/20080919032843 Minefield/3.1b1pre
fails:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b1pre) Gecko/20080920033605 Minefield/3.1b1pre

(Landing bugs http://forums.mozillazine.org/viewtopic.php?f=23&t=861075 )
Comment 4 Benjamin Smedberg [:bsmedberg] 2011-02-03 07:32:28 PST
*** Bug 631228 has been marked as a duplicate of this bug. ***
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-02-03 07:42:12 PST
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d2413f22a98d&tochange=6b6ce24cef9f

Related checkins:
f682b584b24b	Neil Deakin — Bug 410569, tabbrowser should be using drag action rather than checking the keyboard state itself, r=mano
11044a8e4c26	Neil Deakin — Bug 454986, multiple images being dropped, r+sr=neil
45e42cafab3f	Marco Bonardo — Bug 418671 - Clean up places views drag and drop code, r=mano
Comment 6 Alice0775 White 2011-02-03 08:32:17 PST
Regression window in comment#3 & comment#5 shows that browser hang when emails drag over the browser.

If drop is performed, the regression window is different.
Regression window
Works:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1a2pre) Gecko/20080827031643 Shiretoko/3.1a2pre ID:20080827031643
Fails:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1a2pre) Gecko/20080828034823 Shiretoko/3.1a2pre ID:20080828034823
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-27+23%3A00%3A00&enddate=2008-08-29+03%3A00%3A00
Comment 7 Alice0775 White 2011-02-04 05:34:21 PST
FYI,
Minefield does not hang in local build(no PGO, VC++2008),
Comment 8 Phil Lacy 2011-02-06 07:14:40 PST
I can't look into this for quite awhile but if someone wants to make sure this bug is not an issue with multiple drag objects here's the number
bug 514148
Comment 9 Alice0775 White 2011-02-06 07:54:23 PST
This bug intends for dragging of plural E-mails.
bug 514148 intended for dragging of plural attachments of E-mail.
Both are totally different.
Comment 10 Wayne Mery (:wsmwk, NI for questions) 2011-05-24 13:39:30 PDT
encountered this just now, accidentally :(

I don't have the time to retry just now, but I thought I was moving a single message, not multiple.
Comment 11 ibatterb 2011-09-02 00:31:14 PDT
I stumbled across  this bug just now.. definitely exhibits for me for multi object drags only. I was actually dragging messages into a TB folder and accidently moved the mouse outside TB's window and over a FF window. Reproduable every time.
Comment 12 Brian R. Bondy [:bbondy] 2011-09-02 05:36:49 PDT
Wow that's a bad one, the hang is reproducible always for me too on both FF6 and Nightly.  I'll work on it soon.
Comment 13 Brian R. Bondy [:bbondy] 2011-09-02 18:54:55 PDT
Created attachment 558012 [details] [diff] [review]
Patch for Firefox hang when dragging multiple emails over

This Firefox hang happens if you're dragging more than one email from Thunderbird and you happen to pass over top of some part of the Firefox window.

The fix: If the count cannot be determined for a drop just return 0.
Before we left the count uninitialized and that was sometimes a huge value, for me it was always 4294967295.  So looping through each of those items took minutes of hung Firefox.

This can happen if we have collection data of type MULTI_MIME ("Mozilla/IDataObjectCollectionFormat") on the clipboard, but we can't obtain an IID_IDataObjCollection.  This happens with different versions of Firefox and Thunderbird.  For example a normal Thunderbird but a Nightly or Aurora Firfox.
Comment 14 Rob Arnold [:robarnold] 2011-09-06 08:54:10 PDT
Comment on attachment 558012 [details] [diff] [review]
Patch for Firefox hang when dragging multiple emails over

Switching review to khuey as he probably knows this code better than I do (or else I am confused and he doesn't; nonetheless I do not know this code very well).
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-06 09:10:07 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #13)
> This can happen if we have collection data of type MULTI_MIME
> ("Mozilla/IDataObjectCollectionFormat") on the clipboard, but we can't
> obtain an IID_IDataObjCollection.  This happens with different versions of
> Firefox and Thunderbird.  For example a normal Thunderbird but a Nightly or
> Aurora Firfox.

Hmm, why would this be the case?  IID_IDataObjCollection hasn't changed in almost two years ... so I would expect that release Thunderbird and Nightly Firefox would work fine here.
Comment 16 Brian R. Bondy [:bbondy] 2011-09-06 09:12:01 PDT
I just assumed that's why it couldn't query the interface.  I'll investigate more the cause of not being able to get the interface.
Comment 17 Brian R. Bondy [:bbondy] 2011-09-06 09:15:50 PDT
So there may be an additional fix if there's a good reason querying the interface fails, but I think when we can't query it we should either 1) initialize that value to 0 instead of leaving it to some huge number, or return an error code and add the error checking when calling it.
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-06 09:17:51 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #17)
> So there may be an additional fix if there's a good reason querying the
> interface fails, but I think when we can't query it we should either 1)
> initialize that value to 0 instead of leaving it to some huge number, or
> return an error code and add the error checking when calling it.

I agree that the patch as written is a good fix to take, I just don't understand how it fixes the bug :-)
Comment 19 Brian R. Bondy [:bbondy] 2011-09-06 09:23:04 PDT
> I just don't understand how it fixes the bug :-)

The bug is that the query interface fails and we leave the value uninitialized for the number of items. This for me was some very huge number.  Then after that call it loops through each of these items (like 4 billion in my case).

/content/events/src/nsDOMDataTransfer.cpp

> PRUint32 count;
> dragSession->GetNumDropItems(&count);
> for (PRUint32 c = 0; c < count; c++) {

So the fix was instead setting the count to 0 in this case so it won't loop and hang the browser for a long time.

Why the interface fails to query for IID_IDataObjCollection when the item comes from Thunderbird I have no idea yet though.
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-06 09:27:09 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #19)
> Why the interface fails to query for IID_IDataObjCollection when the item
> comes from Thunderbird I have no idea yet though.

Yes, that's the part I don't understand (and think we should investigate further).  I wouldn't be surprised if this is some COM marshalling thing ... I've done my best to erase all memories of that :-D
Comment 21 Brian R. Bondy [:bbondy] 2011-09-06 09:28:46 PDT
> ... some COM marshalling thing ...

Oh Good! :)
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-08 10:04:50 PDT
To be slightly more verbose (and also because I had a nightmare about this bug last night), what I expect is happening is that COM is refusing to marshal IDataObjCollection across the process boundary because it has no idea how to do that.
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-08 10:06:23 PDT
Comment on attachment 558012 [details] [diff] [review]
Patch for Firefox hang when dragging multiple emails over

I believe this is still the correct fix for this bug, but please change the comment to indicate that this happens any time we have a MULTI_MIME format but the data is coming from another process.  Please also verify that the problem here is the COM marshalling before landing.
Comment 24 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-08 10:07:18 PDT
Also, will we need to figure out a story here for multiprocess Firefox?  That's something worth thinking about.
Comment 25 Brian R. Bondy [:bbondy] 2011-09-08 11:07:06 PDT
I was working on this task this morning amongst some other things.

I was thinking a general multi process lack of support as well, and was going to suggest just doing a comment change from my last patch since we don't need support for these drops from Thunderbird anyway. 

But then I tried 2 instances of Thunderbird with different profiles and -no-remote.  It turns out that you can drag from one Thunderbird process to the other no problem for these multi dragged emails.  

I'm currently thinking maybe it's something related to security between different processes maybe there is some check on the module executable that started the process? I'm not sure.  

It would take a while for me to debug the exact reason we don't support it, so I'm going to try asking bsmedberg first.  I'm currently building Thunderbird though in case I have to debug it.

In any case I think we could do this work outside of this ticket if any work is needed.  I think this is the right fix (w/ comment change) for this task and will stop the hanging.
Comment 26 Brian R. Bondy [:bbondy] 2011-09-08 11:10:49 PDT
There is a chance that the only reason Thunderbird cross process drag worked without a hang was because the uninitialized variable happened to be 0 instead of some huge number.
Comment 27 Brian R. Bondy [:bbondy] 2011-09-08 11:14:08 PDT
Comment on attachment 558012 [details] [diff] [review]
Patch for Firefox hang when dragging multiple emails over

Thought you might know the answer to this before I spend a day or two debugging the real reason.  The patch needs a comment change but we were thinking maybe it's a general lack of support for cross process XPCOM in this way?
Comment 28 Brian R. Bondy [:bbondy] 2011-09-08 12:47:05 PDT
So it turns out this collection object is not an XPCOM object at all. I just thought it was because of it's nsI prefix.  It is a MS COM object though.  When we create this object we're currently just heap allocating it with new.  If we ever need to support cross process we should use CoCreateInstance w/ CLSCTX_LOCAL_SERVER.  

I'll update the comment and run the current fix through try then push to mozilla-inbound.
Comment 29 Brian R. Bondy [:bbondy] 2011-09-08 12:54:59 PDT
Created attachment 559253 [details] [diff] [review]
Patch for Firefox hang when dragging multiple emails over. v2.

Updated comment.
Comment 30 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-08 12:57:17 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #28)
> So it turns out this collection object is not an XPCOM object at all. I just
> thought it was because of it's nsI prefix.  It is a MS COM object though. 
> When we create this object we're currently just heap allocating it with new.
> If we ever need to support cross process we should use CoCreateInstance w/
> CLSCTX_LOCAL_SERVER.  

Well, we'd need a lot more than that, but yes.
Comment 31 Brian R. Bondy [:bbondy] 2011-09-08 13:14:57 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d318057a2de8
Comment 32 Brian R. Bondy [:bbondy] 2011-09-08 18:08:26 PDT
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e48615c33782
Comment 33 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-09 07:45:50 PDT
http://hg.mozilla.org/mozilla-central/rev/e48615c33782
Comment 34 Johnathan Nightingale [:johnath] 2011-09-20 14:32:04 PDT
Comment on attachment 559253 [details] [diff] [review]
Patch for Firefox hang when dragging multiple emails over. v2.

Discussed in triage today, and we don't think this is an Aurora candidate. We should only take things on Aurora which represent fixes to regressions introduced in the current version, or which address severe security or stability issues. This fix looks like it fixes an older regression, and should therefore just ride the train through to release.

Please re-nominate with rationale if you think we've misunderstood here. (And in general, please provide rationale for aurora/beta nominations, with some basic risk/reward assessment)
Comment 35 Brian R. Bondy [:bbondy] 2011-09-20 15:52:51 PDT
Riding the train sounds good, thanks for the explanation on candidates.
Comment 36 Wayne Mery (:wsmwk, NI for questions) 2011-11-09 14:30:29 PST
*** Bug 650888 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.