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)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: markus.podar+bugzilla.mozilla.org, Assigned: bbondy)
References
Details
(Keywords: hang, regression)
Attachments
(2 files, 1 obsolete file)
20.96 KB,
application/x-zip
|
Details | |
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.
Reporter | ||
Comment 1•13 years ago
|
||
s/Firefox should not icon/Firefox should not hang/
Comment 2•13 years ago
|
||
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
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•13 years ago
|
Product: Firefox → Core
QA Contact: general → general
Updated•13 years ago
|
Severity: normal → critical
Comment 3•13 years ago
|
||
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 )
Updated•13 years ago
|
OS: Windows 7 → Windows XP
Version: unspecified → Trunk
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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
Comment 9•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: nobody → netzen
Assignee | ||
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Component: Drag and Drop → Widget: Win32
QA Contact: drag-drop → win32
Comment 14•13 years ago
|
||
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)
Attachment #558012 -
Attachment is patch: true
(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.
Assignee | ||
Comment 16•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
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 :-)
Assignee | ||
Comment 19•13 years ago
|
||
> 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
Assignee | ||
Comment 21•13 years ago
|
||
> ... 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.
Assignee | ||
Comment 25•13 years ago
|
||
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.
Assignee | ||
Comment 26•13 years ago
|
||
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.
Assignee | ||
Comment 27•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #558012 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 28•13 years ago
|
||
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.
Assignee | ||
Comment 29•13 years ago
|
||
Updated comment.
Attachment #558012 -
Attachment is obsolete: true
Attachment #559253 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
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.
Assignee | ||
Comment 31•13 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d318057a2de8
Assignee | ||
Comment 32•13 years ago
|
||
Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/e48615c33782
Status: NEW → ASSIGNED
Comment 33•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e48615c33782
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Assignee | ||
Updated•13 years ago
|
Attachment #559253 -
Flags: approval-mozilla-aurora?
Comment 34•13 years ago
|
||
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-
Assignee | ||
Comment 35•13 years ago
|
||
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.
Description
•