344701 - Leaking nsMacWindow objects for top-level windows

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Mark Mentovai]

We're leaking toplevel windows like mad.  smichaud pointed out that he wasn't ever receiving kEventWindowClosed on the 1.8[.1] branch and trunk - this is indicates that DisposeWindow is not being called, and we do that in the nsMacWindow destructor.

Comment 1

[Mark Mentovai]

Attachment: Fix

Comment 2

[Mark Mentovai]

Fix checked in on trunk.

Comment 3

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

Comment on attachment 229270 [details] [diff] [review]
Fix

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and marked fixed1.8.1 when you have done so.

Comment 4

[Mark Mentovai]

Checked in on MOZILLA_1_8_BRANCH for 1.8.1b2.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

I've added this patch to a CVS tree that I pulled last night from the
trunk, then rebuilt the thing.  I can confirm that the JEP now
receives a kEventWindowClosed event when a top-level window gets
closed, into which at least one Java applet has ever been loaded.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

I've noticed a possible problem with this fix (at least on the trunk,
in Firefox):

Try the following in a trunk build that doesn't contain this fix
(e.g. 2006-07-13-07-trunk), and one that does (either build your own,
or wait until tomorrow's trunk nightly is available):

1) Make sure that exactly one browser window is open, and open a Java
   applet inside it.
2) Close that window.
3) Open another browser window and try to enter text in any of its
   text fields.

Without this fix, there are no problems doing step 3.  But with this
fix, text input doesn't work in any text field, in any browser window,
until the browser is restarted.

If the problem only existed in a single window, I'd suspect the Java
Embedding Plugin was at fault.  But it exists in all windows (those
opened after step 2), so I suspect that some aspect of keyboard input
processing is dependent on the broken behavior that was fixed by this
patch.

As best I can tell, this problem isn't caused by the current TSM doc
being set incorrectly.

(I haven't yet tested on the 1.8 branch.  And since most of
Mozilla.org's servers will be down tomorrow, I'll probably have to
wait until Sunday to find out if the 1.8 branch has the same problem.)

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> I haven't yet tested on the 1.8 branch.

I've now done so, and it does have the same problem.

(I built Firefox 2.0 Beta 1 from source, plus the patch from this
bug.)

Comment 8

[Mark Mentovai]

Steven, I can reproduce the behavior you describe in comment 6.  This appears to me to be a JEP bug.  (The rest of this paragraph isn't for you, it's background for people following along in the peanut gallery.)  Once JEP is loaded, it uninstalls the browser's TSM handlers in MaybeInstallTSMAEEventHandler, replacing them with its own TSM handler, HandleAETSM.  HandleAETSM gives the applet a crack at TSM events if it as focus - if not, it's able to forward the events to the original saved handlers.  It refuses to do so if JEP's never heard of the focused window, which the comments indicate is a safety check to avoid forwarding events when sending text to the active window would be nonsense.  Why this check as implemented actually does what it advertises is something of a mystery to me, though.

In this case, that exact check is failing.  JEP hasn't marked the second window with its property tag, so HandleAETSM is returning (Handlers.m:2180) before forwarding the event.  Now, check this out: if you were to load an applet in the second window (via some method other than typing it, obviously), that window would be tagged by JEP, HandleAETSM would stop kicking out early, and text input would resume functioning.

The reason that this worked on the 1.8[.1] branch and trunk before I applied the patch in attachment 229270 [details] [diff] [review] was that the leaked nsMacWindow was causing us to never call DisposeWindow for top-level windows.  We would always have at least one window in the window list marked with JEP's property, so the check that's giving us trouble now would never fail.

The reason that this works on the 1.8.0 branch is that it's got the old key event code that is able to dispatch NS_KEY_PRESS Gecko events on native key-down in addition to doing it from the TSM AE handler.  TSM as a whole is probably broken, but enough of the keys important to US keyboard users were getting through that the problem probably didn't gather the critical mass of pain necessary to get it fixed.  (I haven't actually run it through the deubgger in 1.8.0 to test this theory.)

This probably was broken on the trunk and 1.8[.1] branch between the time that I rewrote the key events and the time that I introduced the native window leak.  Create a bug, fix a bug, create a bug, fix a bug.

P.S. if you don't want to do your own builds but do want to test something a little bit more current than what you find in "nightly," go poking around in the "tinderbox-builds" directory, nightly's sibling on ftp.mozilla.org.  It contains the most recent successful build a tinderbox has produced, less mirror propagation delay.  Pick a directory corresponding to an OS X tinderbox build for the branch you're interested in and have at it.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

You're right, Mark -- it's the kPropertyNSWindow check that's not
working right:  Once you've created at least one Java applet in a
browser session, the kPropertyNSWindow check will always fail if
you've closed all the browser windows in which you ever created a Java
applet.  (Though, as you noticed, the problem (temporarily) goes away
when you create another applet.)

This stops all keyboard input on recent trunk and 1.8 branch builds
(those that rely exclusively on the kUnicodeNotFromInputMethod Apple
event for keyboard input).  It also stops IME input (e.g. Chinese and
Japanese language input) and input using Unicode keyboard layouts
(e.g. US Extended) -- even on the 1.8.0 branch.

So this is pretty bad :-(  I intend to release a new JEP (0.9.5+g) in
the next few days that fixes this problem, plus all but one of the
other problems that Mark has discovered (plus a couple that I
discovered on my own, which only effect that trunk and 1.8 branches).

The kPropertyNSWindow check has been in HandleAETSM() forever, and
(though the problem it addresses was once quite real), I'm not sure
it's needed any more.  But I notice that Firefox's window list
contains _lots_ of invisible windows -- so I've replaced the
kPropertyNSWindow check with one that refuses to pass along TSM Apple
events if they're (supposedly) happening in an invisible window.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

I've just released version 0.9.5+g of the Java Embedding Plugin, which
fixes the problem described in comment 8 and comment 9:

http://javaplugin.sourceforge.net/

Comment 11

[Mark Mentovai]

JEP 0.9.5+g has been checked in, and it fixes the problem described in comment 6 et. seq.

Comment 12

[Daniel Veditz [:dveditz]]

Is this needed on the 1.8.0 branch, or is it a fix for a regression from some other non-1.8.0 fix?