293386 - Audit UnbindFromTree codepaths to make sure teardown change is OK

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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".

Comment 1

[Boris Zbarsky [:bzbarsky]]

This should probably block b2 or b3.....

Comment 2

[Boris Zbarsky [:bzbarsky]]

The prototype-unhooking code at
http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLBinding.cpp#868
worries me...

Comment 3

[chris hofmann]

bryner will look at for b3.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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....

Comment 5

[Brendan Eich [:brendan]]

We should audit for b4, and we're gonna test in b3.

/be

Comment 6

[Brian Ryner (not reading)]

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.

Comment 7

[Brian Ryner (not reading)]

Ok, here's my analysis so far:

1) xbl binding teardown

The main thing that won't happen is that we won't unhook the bound element's
prototype from the prototype chain at document teardown.  As a testcase, I tried
holding a reference to a bound element in an iframe from the parent document,
changing the iframe src, then accessing an XBL-defined <property>.  In both
trunk and Firefox 1.0, the value from the property's getter is returned.  So
this seems to mean that either the prototype has never been unhooked correctly,
or that I'm misunderstanding when the code path in question is triggered.

2) xul element teardown

There's a comment that mentions a reference cycle with controller objects
implemented in Javascript.  I couldn't find any such objects in our tree (lxr'd
for Components.interfaces.nsIController), but even if they exist, the cycle
mentioned has already been broken by the call to SetNewDocument(null) on the window.

3) frame element teardown

Made a simple testcase where a reference to a nested iframe contentwindow is
obtained in the outermost document, then the outer iframe is navigated to a new
 url.  Tested what was returned by .frameElement at that point, and got null,
but it's because we're hitting the case in nsGlobalWindow::GetFrameElement where
the docshell has no parent.  The window itself still has mFrameElement pointing
to the old frame element, which I think is incorrect.  There are some callers
who use GetFrameElementInternal directly that could start accessing that element.

Also, nsDocShell::Destroy is no longer called from nsFrameLoader::Destroy.  This
is ok in the simple case because the docshell's last reference is released and
Destroy is called from the destructor.  However, if someone kept a reference to
the docshell, I think it would incorrectly exist in a non-destroyed state.

4) XTF element teardown

The main issue here is whether XTFElement implementations will be able to access
the window and docshell when ParentChanged and DocumentChanged are called at
teardown.  My feeling is that we should just document that these objects may not
be available, and check all of the in-tree implementations to make sure they
handle this correctly.  The other thing we might consider is adding a
pre-teardown notification before any of this happens.

So, to proceed:

#1/2: Boris, do you think this assessment makes sense?  If so, it seems like we
don't need to worry too much about these.

#3: I'll attempt a fix to maintain the teardown sequence
#4: I'll check the in-tree XTF implementations (which I think is just XForms),
and see how it looks.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 9

[Brian Ryner (not reading)]

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.

Comment 10

[Brian Ryner (not reading)]

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.

Comment 11

[Brian Ryner (not reading)]

Attachment: bindings file for xbl testcase

Comment 12

[Brian Ryner (not reading)]

Attachment: iframe for xbl testcase

Comment 13

[Brian Ryner (not reading)]

Attachment: xbl testcase

Comment 14

[Brian Ryner (not reading)]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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...

Comment 16

[Brian Ryner (not reading)]

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.