> someone is holding on to the nsJSContext(in this case a nsJSEventHandler), the That should have been nsJSEventListener not nsJSEventHandler.
Created attachment 90925 [details] [diff] [review] one line patch Any reason why this will not work?
Created attachment 90926 [details] [diff] [review] Another one-line patch I don't like changing how often we potentially do a GC during document destruction (even though the GC timer code should eliminate any GC's that the first patch would possibly add) so I'd rather see us make sure we schedule a GC when a window is torn down. This patch does that. Does this patch also fix the problem?
Er, s/destruction/transitions/, duh! (it's getting late, or something)
Yep, that should work as well. Thanks.
Well ... on thinking about it a bit ... Seems to me that the two patches are for all practical purposes, equivalent. We are doing gc on document transition anyway. If we have a document transition to a new document, SetNewDocument() will be called with a non-null aDocument. The current code triggers gc in that case anyway, and that doesn't change in anyway with my patch. Its only when aDocument is null that the my patch changes behaviour. And I believe that only happens when the window is being torn down. Right? Which leads me to ask: Why was the original code written such that you do have a gc when transitioning to some new document, but don't have one when you transition to no document? That was just a bit of wondering aloud though .... I don't have any favorites amongst the two patches. Either does of them does the job for me.
We call SetNewDocument(nsnull) during every document transition, i.e. when you go from page to page we first call SetNewDocument(nsnull) to clear out the old document and then we call SetNewDocument(aNewDocument) to insert the new document.
Oh. Of course. I hadn't realized that. Thats what ususally happens when I start "thinking a little bit" without looking at the code :-) I suppose I should shut up now.
No, not at all, you were right on track...
Ugh .. everyone seems to have forgotten about this one. Bug has a patch but no review. If this is going to go in, would it not be a good idea to get this in before the 1.2alpha freeze. Any chance of that happening?
Comment on attachment 90926 [details] [diff] [review] Another one-line patch r=caillon
16 years ago