481133 - FormFillController holds XPCOM objects too long

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Stuart Morgan]

Like 440002, we need to be more careful about releasing core objects before XPCOM shuts down. We have a bunch of warnings on quit in debug builds, and several of them trace back to FormFillController and KeychainAutoCompleteSession session having core objects as members that stay until dealloc.

I'm not sure yet whether I'll put a listener in KACS as well, or just have each FFC release its KACS early, since it's the only owners of them.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Would this (help) fix bug 480330, or is that more complicated?

Comment 2

[Stuart Morgan]

I don't think it would, since that crash is from trying to talk to another one of our own objects based on a core trigger; this is kind of the reverse.

Comment 3

[Stuart Morgan]

Attachment: fix

Rather than sprinkle shutdown listeners everywhere, I just had BrowserWrapper take care of tearing down the whole formfill stack, since all FormFillControllers are BrowserWrapper-owned.

Comment 4

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 366069 [details] [diff] [review]
fix

sr=pink

Comment 5

[Stuart Morgan]

Landed on CVS trunk.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Stuart, this patch appears to cause us to leak more Gecko stuff when quitting with a username child window visible.

STR:
1) Launch Camino to about:blank in a fresh profile
2) http://mail.google.com (which redirects to the Google account login)
3) Select the Keychain-filled username and delete it, then down-arrow to bring up the child window
4) Cmd-Q while the child window is still visible

Pre-patch:

 nsStringStats
  => mAllocCount:           9265
  => mReallocCount:          952
  => mFreeCount:            8605  --  LEAKED 660 !!!
  => mShareCount:           4288
  => mAdoptCount:            711
  => mAdoptFreeCount:        709  --  LEAKED 2 !!!

Post-patch:

 nsStringStats
  => mAllocCount:          10511
  => mReallocCount:          992
  => mFreeCount:            9755  --  LEAKED 756 !!!
  => mShareCount:           5524
  => mAdoptCount:            836
  => mAdoptFreeCount:        834  --  LEAKED 2 !!!

For poor comparison purposes, a launch-about:blank-shutdown usually shows this:

 nsStringStats
  => mAllocCount:           6039
  => mReallocCount:          270
  => mFreeCount:            6031  --  LEAKED 8 !!!
  => mShareCount:           1686
  => mAdoptCount:            325
  => mAdoptFreeCount:        325

I have similar numbers on branch (we're much more leaky there on my about:blank baseline to begin with).

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Per the meeting, landed on the MOZILLA_1_8_BRANCH as-is; I'll spin the leak stuff in comment 6 off into a new bug.