376563 - nsFormFillController::Unload doesn't ever get called

Status: RESOLVED INVALID

(Toolkit :: Form Manager, defect)

Description

[Martijn Wargers (dead)]

This is a follow-up from bug 374881.
When attempting to fix that bug, I noticed that nsFormFillController::Unload doesn't seem to get called at all.
Normally you would expect that to get called, for instance when you've focused Google's search text input and then visit another page.
Because it doesn't get called, formfillcontroller keeps references of stuff of the old page, which could cause weird bugs (bug 374881 was an example of that).

Also, if this gets fixed, then I think the pagehide event should be used.
See bug 343282, comment 5, I think that comment might also (partially) apply here.

Comment 1

[Benjamin Smedberg]

I think we need to block on this to fix the "hold refs to the old document" issue at least.

Comment 2

[Martijn Wargers (dead)]

Michael, you happen to remember what this is about?
Wasn't this basically fixed with your last patch in bug 374881?

Comment 3

[Martijn Wargers (dead)]

Well, I guess it wasn't working when I last tested this, but this is worksforme now, with current trunk debug build.

Comment 4

[Justin Dolske [:Dolske]]

Reopening, as this doesn't workforme (see bug 397753). Also, I originally ran into this when working on bug 384230, where the lack of Unload firing is contributing to a memory leak.

Seems like this even might be fired for JS code, but not this C++ code.

Comment 5

[Justin Dolske [:Dolske]]

Hmm. In nsPIDOMWindow.h I see the comment:

 388   // These two variables are special in that they're set to the same
 389   // value on both the outer window and the current inner window. Make
 390   // sure you keep them in sync!
 391   nsCOMPtr<nsPIDOMEventTarget> mChromeEventHandler; // strong
 392   nsCOMPtr<nsIDOMDocument> mDocument; // strong

The interesting thing about this bug is that JS code (in content) is getting the Unload event which the C++ code is not. [All the other events we registered for on the chrome EL are working.] Might this imply that when a document is being unloaded, there's a period where the inner/outer chrome handlers are out of sync, and so the unload event isn't reaching us?

* - I'm not familiar with this code, so I'm kind of stabbing in the dark here...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I'll try to come up with some solution.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

The only case when I don't see unload event is when a tab/window is closed, 
and the reason for that is that nsFormFillController::RemoveWindowListeners
is called before unload. Perhaps ::RemoveWindowListeners should do the same thing
as ::Unload()
So not a DOM Event bug.
Justin or Martijn, want to take this bug? ;)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Um, this had blocking1.9+, but changing product/component removed that.
So asking blocking‑firefox3?.

Comment 9

[Justin Dolske [:Dolske]]

Ugh!

Well, seems the right thing to do here anyway is to remove the nsIDOMLoadListener interfaces, as Martijn started to do over in bug 397753. No point in having ::Unload around if it's not going to get called.

Since the failure to call ::Unload is expected behavior, I'm closing this as INVALID.

Comment 11

[Justin Dolske [:Dolske]]

(clearing blocking-ff3, since this is invalid)