25364 - We leak one webshell per address in the compose window.

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[Scott MacGregor]

I've noticed that whenever I reply to messages with a couple recipients, I'm
seeing a lot of webshell leaks.

It looks like for every recipient row in the envelope, we leak the gfx text
webshell associated with that row!!

*Yikes*

Jean-Francois, who owns the widget that we enter our email addresses into? Would
buster be the right person to get the ball rolling with this?

(I fixed a leak in gfx text widgets once before where we were leaking
webshells...maybe that leak is back? I'll go look)

Comment 1

[Scott MacGregor]

Nominating for beta1 status.

Comment 2

[Scott MacGregor]

This may require some leak detection work on our own first. Just bringing up the
compose window and then closing the compose window leaked nsMsgCompose and
everything underneath it!!! Uhoh...

Comment 3

[Jean-Francois Ducarroz]

nsMsgCompose leak (and everything under it) because of bug 21056.

Comment 4

[Scott MacGregor]

Okay, then we should nominate that bug for dogfood. Although according to that
bug, nsMsgCompose should only be leaked for the first compose window.

The webshell leaks I'm reporting here for each email address happen for every
compose window I bring up. if I reply to a message with 30 people, we leak 30
web shells.

So I'll still leave this bug as a stand alone bug from #21056 and we can
nominate that one for beta1 separately.

Comment 5

[leger]

Moving [MLK] to mlk keyword

Comment 6

[leger]

Putting on PDT+ radar for beta1.

Comment 7

[lchiang]

Scott - can you nominate that other bug for Beta1 then? It's currently not set 
as beta1.  I'm sure you can include a better explanation than me :-)

Comment 8

[Jean-Francois Ducarroz]

putting buster in cc list in case he has any idea why input element created dynamically from JS are leaking.

Comment 9

[buster]

no, there's nothing special about being created dynamically from the point of 
view of the text control.  the webshell must be being leaked because the frame 
itself is being leaked.  either that, or something really odd is going on due to 
some webshell/docshell change. one experiment would be to undefine 
NEW_WEBSHELL_INTERFACES in nsGfxTextControlFrame.cpp.  If you don't see the 
leak, the leak is probably caused by something holding onto the webshell as a 
result of: mWebShell->SetDocument(...)

Comment 10

[Jean-Francois Ducarroz]

no are probably right. Sofar here is what I have found:

case 1, without creating a second recipient:
 create a WS for the top level window
   create a WS for the html inner frame
    create a WS for the gfx text control frame
    ...
    delete gfx text control frame WS
   delete html inner frame WS
  delete top level window WS

case 2, creating a second recipient:
 create a WS for the top level window
   create a WS for the html inner frame
    create a WS for the gfx text control frame (1st recipient)
    create a WS for the gfx text control frame (2nd recipient)
    ...
  delete top level window WS

in this second case, we are leaking the inner frame as well the two gfx text widgets!

Comment 11

[buster]

maybe the tree widget itself is being leaked.

Comment 12

[Jean-Francois Ducarroz]

I just did the quick test with NEW_WEBSHELL_INTERFACES undified. The result are the same, we are still leaking 
the same way.

Comment 13

[Jean-Francois Ducarroz]

I make some progress. We are leaking because somebody hold a reference on the DocumentViewer member variable 
mPresShell. If a manally decreasse the refcount during the destroy process, all the WebShell are then correctly 
released.

Comment 14

[Jean-Francois Ducarroz]

cc'ing sfraser

Comment 15

[Jean-Francois Ducarroz]

I am kind of lost with this bug! I would really appreciate some help.

Comment 16

[Jean-Francois Ducarroz]

Peter, this is apparently a circular reference problem with webshell. I don't know if it's a tree or gfxTextWidget 
problem.  As I am totally lost on this one, I reassign this bug to your team. Let me know if I can help on something 
else (that was my last PDT+).

Comment 17

[Peter Trudelle]

If there are any tree issues here, they are almost certainly unique to this 
uber-widget, and its singular usage of trees.  Please sit with Hyatt to get his 
assistance on this.  As his hands are once again unusable, you'll have to do the 
typing. I think you are still the right person to own this bug, so I'm 
reassigning it and cc'ing phil.

Comment 18

[Chris Waterson]

i'll take a look at this one.

Comment 19

[Chris Waterson]

travis and I looked at this for a while tonight. as soon as you hit "enter" in 
the first address field, it leaks. (other random typing doesn't seem to induce 
the leak.)

the webshells that are leaked are the two ender widgets (for the first address 
that you just entered, and the second address line that got created), and the 
main message body area.

the outermost webshell is destroyed, as is its presshell. the inner presshells 
are not destroyed.

must sleep now. will look more tomorrow.

Comment 20

[Chris Waterson]

This is a complete rats nest. We are leaking a PresShell, which is touched by
just about everything under the sun. This is going to be very hard to track
down, I think.

Comment 21

[Chris Waterson]

Attachment: fix.

Comment 22

[Chris Waterson]

Attachment: better fix

Comment 23

[Chris Waterson]

Come to daddy, PresShell.

hyatt: can you code review me?

Comment 24

[travis]

In your better fix case, shouldn't the first nsIPresShell* be 
nsCOMPtr<nsIPresShell> ?

Comment 25

[Chris Waterson]

Attachment: heh. third time's the charm.

Comment 26

[Chris Waterson]

Heh. Oh yeah. Guess I should compile first.

Comment 27

[Scott MacGregor]

oh man...I can't believe you found it!! Nice one chris. I'm very impressed. You
can use me as a reviewer if you still need one.

Comment 28

[Chris Waterson]

fix checked in; r=hyatt, travis. a=jar.

Comment 29

[Jean-Francois Ducarroz]

Great job guys. Thanks

Comment 30

[Suresh]

Number of webshell leaks that are dumped to the console looks right while bring 
up the compose window, typing addresses, and then closing the compose window. 
Marking this as Verified. Tested on Debug build that is pulled around yesterday 
evening. 

(Thanks to mscott in helping me to verify this bug).