I should have realized this in bug 274784. Might have prevented the session history regression.... :( In any case, SetScriptGlobalObject had this comment: // XXX HACK ALERT! If the script context owner is null, the document // will soon be going away. So tell our content that to lose its // reference to the document. This has to be done before we actually // set the script context owner to null so that the content elements // can remove references to their script objects. My review comments said something about removing the comment if it's not relevant... That was a major mistake. Either we need to be sure that it's not relevant, or we need to restore this code in the preffed-off case. Some things that should especially bear watching (other than the aforementioned form state saving) when doing this audit: 1) XBL binding teardown 2) XUL element UnbindFromTree (controllers, event listeners, lots of fun here) Fwiw, I bet the leak we saw was in fact through the controllers. Do we have a bug on that? In any case, we really need to do this if we're going to have a hope of the "regression free" aspect of "preffed off".
This should probably block b2 or b3.....
The prototype-unhooking code at http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLBinding.cpp#868 worries me...
bryner will look at for b3.
I really think this should be done for any release that enables bfcache by default, but if drivers are absolutely sure they want to rescind the blocking+, it'd be nice to at least set the blocking? request for b4....
We should audit for b4, and we're gonna test in b3. /be
I came up with a couple more while looking at all of the UnbindFromTree implementations: 3) Frame elements destroy their FrameLoader in UnbindFromTree, and it may not be able to get at the domwindow to call SetFrameElementInternal, or at the docshell to call Destroy(). 4) XTF dispatches change notifications for the parent and document from UnbindFromTree, and listeners for those notifications may not be able to access the DOMWindow / docshell either. I didn't come up with anything else really... the other HTML and SVG element impls look ok.
> 1) xbl binding teardown Hmm... That's odd. We pretty definitely define the property getter and setter on the proto here. And using JSPROP_SHARED, so there should be no slots on the object itself. Brendan, any idea what's up here? > 2) xul element teardown As long as we're not leaking, all's good here.
So, for issue #3, maybe I've forgotten what I was concerned about or maybe something has changed in the last couple of weeks, but I can't reproduce the problem now where the docshell is not destroyed and the frame element is not nulled out. The FrameLoader holds its own reference to the docshell, so it doesn't depend on the document's ScriptGlobalObject to get ahold of it.
For issue #4, there's a problem with the way XForms currently does its model-destruct events. It does them via XTF's DocumentChange notification, which happens when UnbindFromTree is called... after the document has been detached from the window. That means that an event listener on the window won't be able to listen for these events. Given the way this event is defined to work, I think the most straightforward solution is to have XForms listen for unload and dispatch this event from there, when the window is still intact. This will have the effect of disabling fastback for documents containing XForms content. If that's no good, another solution would be to add the unload listener only if model-destruct listeners exist, but that could be quite involved. In any case, I'm not going to have time to make that change to XForms, so cc'ing doron and aaron. I think the semantics of the XTF notifications are probably still ok, just with the caveat that the window is inaccessible at document teardown. XTF implementations that need to deal with pagehide/unload can set up their own listeners for that... quite a bit cleaner IMO than dispatching some notification to every XTFElement in the document.
So, if you load attachment 197215 [details] and click "navigate" then "check property", I think this should test whether the prototype is still hooked up to the element. If you see "2", then it is. Given the teardown code, I'd think it would come back undefined, but I do indeed get "2", both on the trunk and in ff 1.0.x.
Oh, wow. It looks like prototype unhooking has actually been broken ever since XBL stopped creating elements for most things... :( There's never going to be an immediate child of the proto binding called "implementation" in our current setup. This should be checking whether the proto has a non-null mImplementation object instead...
I filed bug 310109 for the XForms problem, and bug 310107 for the XBL prototype unhooking problem. Closing this bug since we finished investigating the change.