226692 - [FIXr]mXBLDocInfoWeak (on nsXBLPrototypeBinding) could probably just be a raw ptr

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

As it is, the code assumes that it'll be non-null (the retval of
GetXBLDocumentInfo is not really checked).  I don't see any circumstances under
which it could really become null, short of someone holding on to prototype
bindings or bindings outside XBL past document info destruction and then trying
to do something with them...

Comment 1

[Mike Kaply [:mkaply]]

Surprise.

We're seeing a crash in GKLAYOUT on the GCC OS/2 build.

In particular here:

http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLPrototypeBinding.cpp#332

332   nsCOMPtr<nsIXBLDocumentInfo> info = GetXBLDocumentInfo(nsnull);
333   
334   return info->DocumentURI();

info is null....

This is after going to  a particular preference page, changing a preference and
clicking OK.

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Patch to work around crash

Here's what the patch looks like that works around the crash.

I had to null check in two places.

Comment 3

[Boris Zbarsky [:bzbarsky]]

mkaply, which preference page and which preference?  That function should never
be returning null (or we need to rename it back and fix all its callers to
null-check (which they currently do not)).

Comment 4

[Mike Kaply [:mkaply]]

Here's the scenario (it seems to happen mostly with a new profile)

Go to preferences->Debug->Networking

Change Diretcory Listing format from HTML to XUL. Then click OK.

On the OS/2 GCC build, we then crash. 

Doesn't happen on the OS/2 VACPP build.

I can't figure out why this would be compiler specific, except GCC is more
sensitive about uninitialized variables.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ok, thanks.  I'll try to debug that when I get back (3 weeks or so).  Either I
will need to change some teardown order somewhere, or the ownership model here
will change.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I thought about this some more, and it's safest to change the ownership model as
follows:

1)  nsXBLDocumentInfo holds strong refs to the prototype bindings via
    mBindingTable
2)  The prototype bindings hold strong refs to the nsXBLDocumentInfo
3)  nsXBLDocumentInfo::Release checks whether the new refcount is equal to the
    number of objects in mBindingTable; if it is, it clears mBindingTable.

That should make sure things stay alive for the right length of time, and
shouldn't leak.

Comment 7

[David Hyatt]

I have a simpler model that I've implemented in Safari.  The simplification could be applied to Gecko.

(1) Instead of having an nsXBLDocInfo object in Safari, I just made an XBLDocument class that 
subclasses XMLDocument.  The XBLDocument is reference-counted.
(2) A memory cache can addref the XBLDocument to pin it.  This would be somewhat analogous to 
Gecko's chrome cache (which would cache XBL documents).
(3) The XBLDocument owns all the prototype bindings.  The prototype bindings are not reference 
counted.  They die when the XBLDocument dies.
(4) XBLBindings (real bindings, not prototypes) then add refs to the XBLDocument.

With this model, prototype bindings needn't be reference counted, and the XBLDocument can be pulled 
out of a memory cache safely even if XBL bindings are still referencing the document.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This is more or less hyatt's suggestion, except I kept nsIXBLDocumentInfo,
since we have no sane way to create different document types based on root
namespace.

So the model this patch implents is:

1)  nsXLBDocumentInfo owns all prototype bindings and deletes them in its
    destructor
2)  Everyone else just has weak refs to prototype bindings
3)  nsXBLBinding holds a weak ref to the proto binding and a strong ref to the
    document info
4)  The document info is what's stored in the chrome cache.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140790 [details] [diff] [review]
Proposed fix

Thoughts?  mkaply, does this fix your crash?

Comment 10

[Mike Kaply [:mkaply]]

Yes, this fixes the OS/2 crash.

Ganesh - we need this for the OS/2 IWB build.

Comment 11

[timeless]

Comment on attachment 140790 [details] [diff] [review]
Proposed fix

I think I flagged the unchecked new in nsXBLDocumentInfo::SetPrototypeBinding
as an OOM concern, but otherwise this looks fine :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140790 [details] [diff] [review]
Proposed fix

Could this possibly be approved for 1.7a?  Fixes some crashes by changing the
ownership model around	a bit...

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 140790 [details] [diff] [review]
Proposed fix

a=[e-mail] for 1.7a.

/be

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.7a

Comment 15

[Jerome Lacoste]

<OT>
Wouah. 
David (from Apple) proposes an implementation.
Michael (from IBM) helps investigation.
Boris (from MIT) fixes it.
Ian (from Opera) is QA contact.

This is sooooo open source. Thank you guys!

This was my first and last post off topic on that system, but I couldn't find a
better way to thank you all for your work.
</OT>