Closed Bug 629253 Opened 13 years ago Closed 13 years ago

Firefox hangs when dragging two emails from Thunderbird over it

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla9

People

(Reporter: markus.podar+bugzilla.mozilla.org, Assigned: bbondy)

References

Details

(Keywords: hang, regression)

Attachments

(2 files, 1 obsolete file)

WinDbg log
20.96 KB, application/x-zip
Details
Patch for Firefox hang when dragging multiple emails over. v2.
2.25 KB, patch
bbondy
: review+
johnath
: approval-mozilla-aurora-
Details | Diff | Splinter Review 
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.
s/Firefox should not icon/Firefox should not hang/
Attached file 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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → general
Keywords: hang
Severity: normal → critical
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 )
Component: General → Drag and Drop
Keywords: regression
QA Contact: general → drag-drop
OS: Windows 7 → Windows XP
Version: unspecified → Trunk
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
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
FYI,
Minefield does not hang in local build(no PGO, VC++2008),
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
This bug intends for dragging of plural E-mails.
bug 514148 intended for dragging of plural attachments of E-mail.
Both are totally different.
Blocks: 650888
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.
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.
Assignee: nobody → netzen
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.
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.
Attachment #558012 - Flags: review?(tellrob)
Component: Drag and Drop → Widget: Win32
QA Contact: drag-drop → win32
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).
Attachment #558012 - Flags: review?(tellrob) → review?(khuey)
(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.
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.
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.
(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 :-)
> 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.
(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
> ... some COM marshalling thing ...

Oh Good! :)
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 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.
Attachment #558012 - Flags: review?(khuey) → review+
Also, will we need to figure out a story here for multiprocess Firefox?  That's something worth thinking about.
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.
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 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?
Attachment #558012 - Flags: feedback?(benjamin)
Attachment #558012 - Flags: feedback?(benjamin)
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.
Updated comment.
Attachment #558012 - Attachment is obsolete: true
Attachment #559253 - Flags: review+
Attachment #559253 - Attachment is patch: true
(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.
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e48615c33782
Status: NEW → ASSIGNED
http://hg.mozilla.org/mozilla-central/rev/e48615c33782
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Attachment #559253 - Flags: approval-mozilla-aurora?
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)
Attachment #559253 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Riding the train sounds good, thanks for the explanation on candidates.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: