241503 - Crash in GKLAYOUT.DLL on attempt to drag'n'drop mail messages/addresses

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Max Alekseyev]

OS/2 build 2004042208

To reproduce:
1. Start Mozilla and open MailNews window
2. Try to drag'n'drop some message from one folder to another
3. Mozilla crashes

------------------------------------------------------------

04-23-2004  14:06:17  SYS3175  PID 0350  TID 0001  Slot 00c3
P:\INTERNET\MOZILLA\MOZILLA.EXE
c0000005
13627df5
P1=00000001  P2=00000000  P3=XXXXXXXX  P4=XXXXXXXX  
EAX=00000000  EBX=00000000  ECX=0012e0e8  EDX=010bc3c4
ESI=00dcfc40  EDI=0012e428  
DS=0053  DSACC=f0f3  DSLIM=ffffffff  
ES=0053  ESACC=f0f3  ESLIM=ffffffff  
FS=150b  FSACC=00f3  FSLIM=00000030
GS=0000  GSACC=****  GSLIM=********
CS:EIP=005b:13627df5  CSACC=f0df  CSLIM=ffffffff
SS:ESP=0053:0012e0c8  SSACC=f0f3  SSLIM=ffffffff
EBP=0012e120  FLG=00012246

GKLAYOUT.DLL 0001:00257df5

Comment 1

[Mike Kaply [:mkaply]]

Jan,

This crash is happening here:

http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2018

Any ideas?

I couldn't look at mSlots, but I am assuming it is null.

Comment 2

[Jan Varga [:janv]]

Hmm, it seems that NS_DRAG_ENTER was not fired.

Comment 3

[Mike Kaply [:mkaply]]

I wonder if on OS/2 we are only sending NS_DRAGDROP_ENTER in a real "enter" case
vs. when the drag is started in a Window.

I believe in this case the crash happens as you leave the window where you began
the drag.

Comment 4

[Rich Walsh]

On OS/2, platform-specific code only generates an NS_DRAGDROP_ENTER event when
the mouse moves from one native window to another.  This appears to be the same
as in the Mac, Win, and XLib code I examined.  AFAICT, internal enter/exit
events are generated by  nsEventStateManager::GenerateDragDropEnterExit()
(http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#2728).
  
You might want to investigate whether this code sends an enter event to the
source widget at the inception of a drag, or only when the drag moves from one
widget to the next.

However, I'm not sure if this is relevant.  Shouldn't the source receive at
least one dragover event before the exit?  If so, the crash should occur almost
immediately rather than on exit because the dragover code is the first to access
the presumably-null mSlots.  Instead, the problem may be that this code is
receiving two NS_DRAGDROP_EXIT events:  one from GenerateDragDropEnterExit() 
and one from the platform-specific code (note that the listing window, the
separator bar, and the folder tree are all separate, native nsWindows).

Comment 5

[Jan Varga [:janv]]

I think I now know what's happening here. Thanks for providing all these details.

Comment 6

[Mike Kaply [:mkaply]]

Jan, is it something that is fixable? :)

Or should we attempt to fix it on OS/2?

Thanks

Comment 7

[Jan Varga [:janv]]

Mike, I'm planning to fix it on all platforms, since there are problems not only
on OS/2.

Comment 8

[Andy Willis (abwillis)]

If this bug is cross platform should the OS be changed to all?

Comment 9

[Sergey Lipnevich]

I'm seeing this on Linux 2.6.7 with XOrg recent CVS and GTK 2.4. The whole setup
is fairly non-standard (everything's compiled from source, using Source Mage
GNU/Linux) and compiled with -O3 (Mozilla compiled with -O2) with GCC 3.4.0, but
it looks like I'm not the only one.

Comment 10

[Mike Kaply [:mkaply]]

*** Bug 252231 has been marked as a duplicate of this bug. ***

Comment 11

[Timofey Vorobyov]

Worksforme 1.8a2

Comment 12

[Steve Richards]

No it is still quite broken as of the 7/12 build which is the latest OS/2 I see

Comment 13

[Jan Varga [:janv]]

I'm working on a fix, I plan to land it for 1.8a3/1.8b

Comment 14

[Mike Kaply [:mkaply]]

*** Bug 253596 has been marked as a duplicate of this bug. ***

Comment 15

[Mike Kaply [:mkaply]]

*** Bug 260455 has been marked as a duplicate of this bug. ***

Comment 16

[Felix Miata]

This remains a critical problem in OS/2 trunk as of 22 Sept at least (newer
builds are useless).

Comment 17

[Lewis Rosenthal]

I still see this with 1.8a4 (from mozilla.org; not one of Peter's builds). In
the NG, Peter reported that he hadn't seen the GKLAYOUT problems with 1.8a4, but
I see the same (now old) problem when dragging messages to folders.

Anyway, apologies for the spam.

Lewis
Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8a4) Gecko/20040930 MultiZilla/1.6.4.0b

Comment 18

[Paul Lazaga]

When I first installed 1.8a4 it seemed to be gone and drag/drop worked.  It now
causes something similar to the orginal error, if not the same error.

10-05-2004  19:26:32  SYS3175  PID 0062  TID 0001  Slot 00b0
D:\BIN\MOZILLA.ORG\MOZILLA\MOZILLA.EXE
c0000005
1d8bc5f5
P1=00000001  P2=00000000  P3=XXXXXXXX  P4=XXXXXXXX  
EAX=00000000  EBX=00000000  ECX=0012e128  EDX=08740d50
ESI=0012e498  EDI=043aa4e0  
DS=0053  DSACC=f0f3  DSLIM=ffffffff  
ES=0053  ESACC=f0f3  ESLIM=ffffffff  
FS=150b  FSACC=00f3  FSLIM=00000030
GS=0000  GSACC=****  GSLIM=********
CS:EIP=005b:1d8bc5f5  CSACC=f0df  CSLIM=ffffffff
SS:ESP=0053:0012e108  SSACC=f0f3  SSLIM=ffffffff
EBP=0012e160  FLG=00012246

GKLAYOUT.DLL 0001:0024c5f5


(In reply to comment #17)
> I still see this with 1.8a4 (from mozilla.org; not one of Peter's builds). In
> the NG, Peter reported that he hadn't seen the GKLAYOUT problems with 1.8a4, but
> I see the same (now old) problem when dragging messages to folders.
> 
> Anyway, apologies for the spam.
> 
> Lewis
> Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8a4) Gecko/20040930
MultiZilla/1.6.4.0b

Comment 19

[Lewis Rosenthal]

I don't normally post "me too" bug spam, but in the manner of confirmation of
Paul's comment #18, my POPUPLOG shows:

10-03-2004  22:18:14  SYS3175  PID 0064  TID 0001  Slot 00ac
J:\MOZILLA.ORG\MOZILLA\MOZILLA.EXE
c0000005
1716c5f5
P1=00000001  P2=00000000  P3=XXXXXXXX  P4=XXXXXXXX  
EAX=00000000  EBX=00000000  ECX=0012e138  EDX=00c492c8
ESI=0012e4a8  EDI=004ca640  
DS=0053  DSACC=f0f3  DSLIM=ffffffff  
ES=0053  ESACC=f0f3  ESLIM=ffffffff  
FS=150b  FSACC=00f3  FSLIM=00000030
GS=0000  GSACC=****  GSLIM=********
CS:EIP=005b:1716c5f5  CSACC=f0df  CSLIM=ffffffff
SS:ESP=0053:0012e118  SSACC=f0f3  SSLIM=ffffffff
EBP=0012e170  FLG=00052246

GKLAYOUT.DLL 0001:0024c5f5

Comment 20

[Andy Willis (abwillis)]

Comment #18 
It seems to work once and only once with a new install.  In fact, I moved one
message from the inbox view across to the folders without the crash but then it
crashed crossing back over to the inbox view.  To make sure I am clear on what I
mean, I dragged one across to the folders but did not drop it, I then moved my
mouse back across the line and it crashed.
Andy

Comment 21

[Mike Kaply [:mkaply]]

*** Bug 267156 has been marked as a duplicate of this bug. ***

Comment 22

[Peter Weilbacher]

(In reply to comment #13)
> I'm working on a fix, I plan to land it for 1.8a3/1.8b

Jan, are you still hopeful that you can solve this soon or should someone try to
fix the OS/2 side of things? From the feedback I see for OS/2 this is the main
reason why all of the alphas get little testing. (1.8a5 is still broken.)

Comment 23

[Frank Wein [:mcsmurf]]

*** Bug 276573 has been marked as a duplicate of this bug. ***

Comment 24

[Peter Weilbacher]

*** Bug 286212 has been marked as a duplicate of this bug. ***

Comment 25

[Mark Banner (:standard8)]

On today's suite trunk build (Linux) I am seeing this assertion just after
starting to drag items in address book and the bookmark manager.

###!!! ASSERTION: can't get drag session: 'mSlots->mDragSession', file
/opt/mozmaster/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 1898
Break: at file
/opt/mozmaster/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 1898

Is this likely to be related to this bug? It is in a similar area of the code.

Comment 26

[mike.hitchcock]

Also get crashes with  Wapzilla EMX nightly when trying to read an 
message that has an attachment (PDF file and another that had a zip file).

Warpzilla dissappears from desktop and some minutes later get a SYS3175
error "A program generated an access violation at 1da7fdb8.
GKLAYOUT.DLL 001:001bfdb8

Comment 27

[Mark Banner (:standard8)]

(In reply to comment #25)
> On today's suite trunk build (Linux) I am seeing this assertion just after
> starting to drag items in address book and the bookmark manager.
Ignore this, I don't believe its part of this bug, I raised it as a seperate bug
- bug 278630

Comment 28

[Peter Weilbacher]

This is definitely one of the major bugs on OS/2 and still the reason why many
people get angry with the current trunk builds. That why I ask for blocking again...

Otherwise it would be great if someone could give a hint on where to look for
the cause of the missing event message. Are there documents somewhere that
explain Mozilla message generation and how those messages are related to native
messages?

Comment 29

[Rich Walsh]

I can now descibe the nature of the problem but don't know enough about the
underlying window structure & event-dispatch mechanism to fix it properly.

The problem isn't in nsTreeBodyFrame windows where the crashes occur but in
whatever window is their parent.  To use Thunderbird as an example, it has two
nsTreeBodyFrames separated by a gray area which I assume is their parent.

Normally, when the native drag code generates an NS_DRAGDROP_EXIT event, it
passes through nsEventStateManager::GenerateDragDropEnterExit() and is forwarded
to the window the mouse just left.  However, when the mouse exits this gray
parent window, the native EXIT event doesn't pass through normal channels and is
incorrectly directed to the window just entered.

While most windows can handle getting an EXIT before an ENTER, nsTreeBodyFrame
can't.  It relies on its first-ever ENTER event to initialize its mSlots member.
 When its first-ever event is an EXIT - and that code relies on mSlots - it
crashes with a null pointer.

I can provide additional info, such as a rudimentary trace of events, to
whomever can use it,  Meanwhile, here's a quick-and-dirty fix for
nsTreeBodyFrame.cpp at line 2012:

  else if (aEvent->message == NS_DRAGDROP_EXIT) {
+   // this event was meant for another window, so ignore it
+   if (!mSlots)
+     return NS_OK;
  // Clear out all our tracking vars.

Comment 30

[Mike Kaply [:mkaply]]

Attachment: Workaround per rich

Comment 31

[Andrew Schultz]

==> trees

Comment 32

[Peter Weilbacher]

Comment on attachment 189920 [details] [diff] [review]
Workaround per rich

Mike, could you perhaps check in the workaround (for now)? It doesn't look like
we would find the real solution for this soon...
If anybody is worried about bad influence on other platforms maybe use #ifdef
XP_OS2 for those lines?

Comment 33

[Mike Kaply [:mkaply]]

I want to. I tried to get a trees person to review it...

Comment 34

[Brian Ryner (not reading)]

Comment on attachment 189920 [details] [diff] [review]
Workaround per rich

Seems like we should check this for the other NS_DRAGDROP_ messages as well. 
r=me with that.

Comment 35

[Mike Kaply [:mkaply]]

Attachment: New patch per bryner

Comment 36

[Felix Miata]

*** Bug 304637 has been marked as a duplicate of this bug. ***

Comment 37

[Arjan van Kasteel]

for your info: firefox 1.5 and 1.6 works fine, without this problem

Comment 38

[Arjan van Kasteel]

Stupid me: Thunderbird ofcourse !!

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 192418 [details] [diff] [review]
New patch per bryner

>Index: nsTreeBodyFrame.cpp
>-    if (! mView)
>+    if ((! mView) || (! mSlots))

No need for extra parens.  Just do:

  if (!mView || !mSlots) 

>+     // this event was meant for another window, so ignore it

Another window?  Or another frame?  Or another widget?	Or another docshell? 
Do we know for sure?  If not, I'd say "another frame" here for now.

Please make sure to put the bug number of the bug you file on the real problem
in all these comments; make sure to cc me and "roc@ocal" on it.

sr=bzbarsky with those fixed.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Oh, and let me know if anything here needs checkin, ok?

Comment 41

[Rich Walsh]

(In reply to comment #39)
>
> >+     // this event was meant for another window, so ignore it
> 
> Another window?  Or another frame?  Or another widget? Or another docshell?
> Do we know for sure?  If not, I'd say "another frame" here for now.

It's whatever you'd call the gray bar that separates the folder tree from the
email listing - that's the thingie the exit event was meant for.

Comment 42

[Boris Zbarsky [:bzbarsky]]

That's a splitter, so you want "another frame"...

Comment 43

[Felix Miata]

Did this get checked into trunk? I can no longer drag a link from
http://www.squarefree.com/bookmarklets/forms.html#remember_password to the
personal toolbar. I get the you can't to that symbol if I try.

Comment 44

[Boris Zbarsky [:bzbarsky]]

No, this hasn't been checked in (as you could have seen for yourself by looking
at the CVS log for the files the patch touches); the patch needs to be updated
to review comments.  Sounds like you're seeing a separate bug that you should file.

Comment 45

[Rich Walsh]

(In reply to comment #43)
> I can no longer drag a link [...] to the personal toolbar.
> I get the you can't to that symbol if I try.

This was reported in the Warpzilla newsgroup - you can't drop anything anywhere.
 I don't know if a bug has been opened yet.  If you create one, please put me on
the CC list.  I plan to pursue this tomorrow.

Comment 46

[Peter Weilbacher]

Attachment: Patch for checkin

This takes into account the review comments and is ready for checkin. mkaply or
bz if you have the time would be nice if you could checkin to trunk.

This is really harmless and both TB and SeaMonkey on OS/2 are useless to many
people without this patch, so I request approval as well.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 48

[Scott MacGregor]

triage team: this looks like a low risk crash fix. But maybe we should make this
wait until 1.8b5 since we are so close to our b4 deadline and this just got
checked into the trunk. 

Comment 49

[Asa Dotzler [:asa]]

Comment on attachment 194942 [details] [diff] [review]
Patch for checkin

this should wait until we open the tree for beta2.

Comment 50

[Asa Dotzler [:asa]]

note, we're still working on firefox beta 1, do not land until we've opened up
for beta 2. thanks.

Comment 51

[Felix Miata]

Since this is OS/2-only, don't you think it should be up to the OS/2 port
owner/driver (currently on vacation) to decide when it goes in?

Comment 52

[Peter Weilbacher]

Well, the fix is in XP code, and the a- does not keep us from fixing it locally
for the next OS/2 release. (It's also partly our fault as we waited with this
for so long...)

Comment 53

[Peter Weilbacher]

Comment on attachment 194942 [details] [diff] [review]
Patch for checkin

Seems like the 1.8 branch is open again. bz, can you check this in there,
please?

Comment 54

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.