220126 - crash in the Mailnews Filter Dialog [@ PresShell::PostReflowCallback]

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Matthias Versen [:Matti]]

win2k build 20030923..

1) select Theme : modern
2) open Mailnews
3) select Tools/Message Filters
4) select an existing entry and use "Edit"
5( add a rule with "MORE" and select customize and add your special header
6) select label as "work" as action
7) use the scrollbar to scroll the actions list up&down
8) select "OK" or click on the "X"

You should now seen a crash between 6) and 8)

I could reproduce it 5x in a row

Comment 1

[Matthias Versen [:Matti]]

Attachment: stack trace #1 (win2k optimized with symbols)

Comment 2

[Matthias Versen [:Matti]]

Attachment: stack trace #2

Comment 3

[Matthias Versen [:Matti]]

works in 1.4 and I'm unable to change my message filter with a trunk optimized
and/or debug build

Comment 4

[Matthias Versen [:Matti]]

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

Comment 5

[Ian Neal]

Happens in classic too - Talkback ID TB23846475M

Comment 6

[Ian Neal]

Doesn't crash in BuildID 2003091704 on WinXP SP1 but does in BuildID 2003092304

Comment 7

[Ian Neal]

Minimised reproducable case:
1) Start mail
2) Select Tools, Message Filters
3) Hightlight existing filter and click Edit
4) Click ok/cancel/x
5) CRASH!

Comment 8

[Ian Neal]

And for those that haven't got a message filter already set up:
1) Start mailnews
2) select Tools/Message Filters
3) create a new filter by cliking on new eg Subject contains abcxyz and tell it
to move it the trash folder
4) ok the new filter
5) select edit in the list of filters
6) Click on ok/cancel/x and watch it CRASH!

Comment 9

[Ian Neal]

Setting OS->All as dupe is on Linux and this one is on 2k/XP

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Purify output is likely to show the problem quite clearly.  Valgrind might. 
Otherwise finding the problem may require spending a good bit of time in the
debugger.

Can anyone try this under purify?

Comment 11

[Andrew Schultz]

regression between linux trunk 2003091822 and 2003092022
I post results from valgind whenever it actually manages to come up.

Comment 12

[Andrew Schultz]

All I get with valgrind is:
Jump to the invalid address stated on the next line
 0x0: ???
 0x44114A4C: PresShell::ResizeReflow() (nsPresShell.cpp:3053)
 0x4411D776: PresShell::ResizeReflow() (nsPresShell.cpp:6299)
 0x444806B5: nsViewManager::SetWindowDimensions() (nsViewManager.cpp:588)
 0x44483754: nsViewManager::DispatchEvent() (nsViewManager.cpp:1851)
 0x4447BA6B: HandleEvent() (nsView.cpp:76)
 0x45DC8573: nsWidget::DispatchEvent() (nsWidget.cpp:1506)
 0x45DC82B4: nsWidget::DispatchWindowEvent() (nsWidget.cpp:1394)

Mozilla's stack walker sees the same.  gdb can't see past the signal handler.

Comment 13

[Matthias Versen [:Matti]]

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

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 15

[Andrew Schultz]

backing out bug 217201 fixed this bug for me
==> roc

Comment 16

[Stephen Walker]

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

Comment 17

[Stephen Walker]

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

Comment 18

[Ben Fowler]

If I comment out the call to FreeFrame( ) on line 5129, the bug
goes away.

This call does look a little early, but I may on the wrong lines, in saying
this; I think that I am right in suggesting that at some point we
are  calling through bogus pointers.

Comment 19

[Mickey Stein]

I'm not sure if bug 220629 is related to this. I've noticed it in my linux
builds of tbird over the same timeframe, and that it always crashes if you
highlight a message in the inbox and select "Create filter from message". 

I can also duplicate/confirm the crashes in this bug on my builds.

Comment 20

[Mike Cowperthwaite]

I've sent in three talkbacks on this problem; two just now, with
  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6a) Gecko/20030930

Updating product/component; no wonder it was hard to find this bug.

Comment 21

[Matthias Versen [:Matti]]

-> Layout

Comment 22

[lohphat]

um....why?  Please help us understand the process of categorizing bugs to proper
areas.

Comment 23

[George Beker]

I can confirm this immediate and repeatable crash on today's (20031001) nightly. 

G. Beker

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Because this crash stack is in Layout, not Mail/News.

Comment 25

[Charles Fenwick]

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

Comment 26

[Mats Palmgren (inactive)]

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

Comment 27

[Mats Palmgren (inactive)]

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

Comment 28

[Matthias Versen [:Matti]]

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

Comment 29

[Mats Palmgren (inactive)]

It looks like a reentrancy problem in PresShell::HandlePostedReflowCallbacks().
Here is some trace output of the events (without calling FreeFrame so we
should not see two events with the same address):

0x883a918 PresShell::PostReflowCallback - add to empty list 0x88bafc0
0x883a918 PresShell::PostReflowCallback - append 0x88bafc8 as next on 0x88bafc0
0x883a918 IN PresShell::HandlePostedReflowCallbacks 28
      28:  mFirstCallbackEventRequest 0x88bafc0
      28:  mLastCallbackEventRequest 0x88bafc8
      28:  toFree=0x88bafc0
0x883a918 IN PresShell::HandlePostedReflowCallbacks 29
      29:  mFirstCallbackEventRequest 0x88bafc8
      29:  mLastCallbackEventRequest 0x88bafc8
      29:  toFree=0x88bafc8  <------------------------------------------------+
0x883a918 PresShell::PostReflowCallback - add to empty list 0x88a499c	      |
0x883a918 IN PresShell::HandlePostedReflowCallbacks 30			      |
      30:  mFirstCallbackEventRequest 0x88a499c				      |
      30:  mLastCallbackEventRequest 0x88a499c				      |
      30:  toFree=0x88a499c						      |
0x883a918 PresShell::PostReflowCallback - add to empty list 0x88a4b38	      |
0x883a918 IN PresShell::HandlePostedReflowCallbacks 31		            BAD
      31:  mFirstCallbackEventRequest 0x88a4b38				      |
      31:  mLastCallbackEventRequest 0x88a4b38				      |
      31:  toFree=0x88a4b38						      |
0x883a918 OUT PresShell::HandlePostedReflowCallbacks 31			      |
0x883a918 OUT PresShell::HandlePostedReflowCallbacks 30			      |
0x883a918 OUT PresShell::HandlePostedReflowCallbacks 29			      |
      28:  toFree=0x88bafc8  <------------------------------------------------+
0x883a918 PresShell::PostReflowCallback - add to empty list 0x8892294
0x883a918 PresShell::PostReflowCallback - append 0x88924e0 as next on 0x8892294
0x883a918 IN PresShell::HandlePostedReflowCallbacks 32
      32:  mFirstCallbackEventRequest 0x8892294
      32:  mLastCallbackEventRequest 0x88924e0
      32:  toFree=0x8892294
      32:  toFree=0x88924e0
0x883a918 PresShell::PostReflowCallback - add to empty list 0x889529c
0x883a918 IN PresShell::HandlePostedReflowCallbacks 33
      33:  mFirstCallbackEventRequest 0x889529c
      33:  mLastCallbackEventRequest 0x889529c
      33:  toFree=0x889529c
0x883a918 OUT PresShell::HandlePostedReflowCallbacks 33
0x883a918 OUT PresShell::HandlePostedReflowCallbacks 32
0x883a918 OUT PresShell::HandlePostedReflowCallbacks 28

Comment 30

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

o  Add a missing update of mLastCallbackEventRequest in CancelReflowCallback()
   in the case that the last event in the list is cancelled.
   (is not strictly necessary to fix this bug though)

o In HandlePostedReflowCallbacks():
  old code: walk the list doing callbacks, set mLast/FirstCallbackEventRequest
	    pointers to NULL only after the loop is done.
  new code: unlink the first event from the list and update the pointers
	    immediately, then do the callback.

Comment 31

[Boris 'pi' Piwinger]

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

Comment 32

[Boris 'pi' Piwinger]

Just to make sure, 1.6a does not miss it ... blocking1.6a?

pi

Comment 33

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

Comment on attachment 132708 [details] [diff] [review]
Patch rev. 1

looks great!

Comment 34

[Frederic Bezies]

My 0.000000000000002 cents :

I applied the patch (Pacth Rev.1) on my current CVS based Thunderbird sources,
and no more crash while using filter UI.

I don't try with my mozilla trunk source.

I reproduce tests case in comment #7 & #8 and no crashes.

Thanks a lot :)

Comment 35

[Ray Simard]

*NIX build 0.4a (20030930) : After applying patch ID 132708, crash apparently no
longer occurs ("apparently" since I haven't yet tried more than a few times so
far). At this point fix appears solid.

Comment 36

[George Beker]

How about telling us non-technocrats how to apply the patch.  The bug really is
lousy to live with!

Beker

Comment 37

[Ashley Bischoff (blog at handcoding.com)]

George: Your best bet would probably be to wait until the patch is approved and
checked-in. Then it'll be available in the usual nightly builds.

Comment 38

[Mats Palmgren (inactive)]

Comment on attachment 132708 [details] [diff] [review]
Patch rev. 1

David, could you review this one? (or suggest someone else...)
See comment 29 and comment 30 for a description of the problem/patch

Comment 39

[[e-mail]]

Comment on attachment 132708 [details] [diff] [review]
Patch rev. 1

>    nsCallbackEventRequest* node = mFirstCallbackEventRequest;
>+   for (; node ; node = mFirstCallbackEventRequest) {
Code style nit:
while (mFirstCallbackEventRequest) {
  nsCallbackEventRequest* node = mFirstCallbackEventRequest;

Comment 40

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Updated as suggested in comment 39

Comment 41

[Ben Fowler]

The patch referred to in comment 36 is not something that can be downloaded
to replaced the faulty bits of your application with good ones, but a
concise description of the modifications to the sources of Mozilla, and
is useless unless you have a working build tree.

To get back a working lizard, you should download a recent nightly 
(say around 17th September) which does not have the bug, but does
have the features that you want.

You do not want a current build until there is the all clear on this bug.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 133238 [details] [diff] [review]
Patch rev. 2

The change that fixes this bug looks fine.  However, I'm a little puzzled by
this part of CancelReflowCallback:

	nsCallbackEventRequest* toFree = node;
	if (node == mFirstCallbackEventRequest) {
	  mFirstCallbackEventRequest = node->next;
	  node = mFirstCallbackEventRequest;
	  before = nsnull;
	} else {
	  node = node->next;
	  before->next = node;
	}

In the then-clause of the if, the assignment to |node| doesn't change anything,
and seems wrong.  Furthermore, the assignment to |before| doesn't change
anything either.  Shouldn't this code be modified:

-	   node = mFirstCallbackEventRequest;
-	   before = nsnull;
+	   node = node->next;
+	   NS_ASSERTION(before == nsnull, "impossible");

or am I missing something?

Comment 43

[Mats Palmgren (inactive)]

> the assignment to |node| doesn't change anything

You missed the assignment to |mFirstCallbackEventRequest|.
I can change it to |node = mFirstCallbackEventRequest = node->next;|
if you think that is more clear. (the else-clause have a similar construct).

> the assignment to |before| doesn't change anything either

Agreed, it can be removed.

Comment 44

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Replace the |before = nsnull| with assertion that it already is nsnull.

Comment 45

[Mats Palmgren (inactive)]

Hmm, maybe
	  node = node->next;
	  mFirstCallbackEventRequest = node;
is more clear? (would make it look more like the else-clause too)

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I like the proposal in comment 45.

Comment 47

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

OK, so it is.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

->mats

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-10-14 16:29 -0700.

Comment 50

[meniscus]

Currently using 2003101504 on WinXP... does this fix appear in this nightly?

If so, I don't think this is fixed yet... either that, or I'm experiencing a
different bug...

Steps:
1. Open Mail.
2. Select a message.
3. Select [Message] > Create Filter from Message...
4. CRASH!

Comment 51

[Frank Wein [:mcsmurf]]

um is this bug about crashing when closing the Edit dialog? I crash when i click
on the Edit button or when Creating a filter from a message. If it's this bug,
please reopen, this still crashes with a cvs trunk build

Comment 52

[Jerry Baker]

I am also seeing this with 2003101504 trunk. Talkback ID TB24475642Q. In my case
it occurred upon pressing "edit" in the Message Filters window.

Comment 53

[WADA:World Anti-bad-Duping Agency]

Mozilla 2003101604-trunk(mozilla-win32-talkback.zip) still crashed when I
pressed "Edit" button in Tools/Message Filters window.
TalkBackID : TB24475506H

I use "Modern Theme".
No problem on Mozilla 1.5 release buid.

Comment 54

[Boris Zbarsky [:bzbarsky]]

People may be seeing bug 222468 

Comment 55

[Mats Palmgren (inactive)]

REOPEN, I see this too in 2003-10-16-05 on Linux when I click "Edit..." on an
existing mail filter (TB24486614G).  A stack from the talkback would be nice.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Mats, that's bug 222468.  This bug really is fixed...

Comment 57

[Mats Palmgren (inactive)]

OK, thanks for checking.

-> FIXED

Comment 58

[Tobias Weibel]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6a) Gecko/20031018

Comment 59

[Matthias Versen [:Matti]]

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

Comment 60

[Juan Rey]

Is it really fixed or should it be reopened ?

Comment 61

[Boris Zbarsky [:bzbarsky]]

This is fixed, as far as I know.  The filter dialog still crashes, but that's a
different bug...