378327 - minor cleanup of ForwardReferences code in nsXULDocument

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Nickolay_Ponomarev]

Attachment: patch

- move AddForwardReference and ResolveForwardReferences from nsIXULDocument to nsXULDocument, make them non-virtual.
- make mForwardReferences an nsTArray<nsAutoPtr<nsForwardReference> > instead of nsAutoVoidArray.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Nickolay, I'm not sure I'll be able to get to this in an expeditious manner.  You might want to ask another sr....

Comment 2

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

Comment on attachment 262378 [details] [diff] [review]
patch


>+nsresult
> nsXULDocument::AddForwardReference(nsForwardReference* aRef)
> {
>     if (mResolutionPhase < aRef->GetPhase()) {
>-        mForwardReferences.AppendElement(aRef);
>+        if (!mForwardReferences.AppendElement(aRef)) {
>+            delete aRef;
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        }

I'm not quite sure what happens if AppendElement fails.
Is there a nsAutoPtr<nsForwardReference> object already created, and
that has taken the ownership. In that case |delete aRef| would delete
already deleted object.

Comment 3

[Nickolay_Ponomarev]

My reading of http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsTArray.h#501 was that the allocation (which can fail) happens first. And the nsAutoPtrs are created only after allocation succeeds. I.e. "class Item" there is nsForwardReference, not nsAutoPtr<nsForwardReference> or something...

Comment 4

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

Comment on attachment 262378 [details] [diff] [review]
patch

OK, I think OOM handling is ok. It just took some time to read nsTArray.

>     const nsForwardReference::Phase* pass = nsForwardReference::kPasses;
>     while ((mResolutionPhase = *pass) != nsForwardReference::eDone) {
>         PRInt32 previous = 0;
>-        while (mForwardReferences.Count() && mForwardReferences.Count() != previous) {
>-            previous = mForwardReferences.Count();
>+        while (mForwardReferences.Length() &&
>+               mForwardReferences.Length() != previous) {
>+            previous = mForwardReferences.Length();
> 
>-            for (PRInt32 i = 0; i < mForwardReferences.Count(); ++i) {
>-                nsForwardReference* fwdref = NS_REINTERPRET_CAST(nsForwardReference*, mForwardReferences[i]);
>+            for (PRInt32 i = 0; i < mForwardReferences.Length(); ++i) {
>+                nsForwardReference* fwdref = mForwardReferences[i];


nsTArray<..>.Length() returns PRUint32, not PRInt32 like nsVoidArray.Count()

Comment 5

[Nickolay_Ponomarev]

Checked in with the PRUint32 fix, good catch!

mozilla/content/xul/document/public/nsIXULDocument.h  1.40
mozilla/content/xul/document/src/nsXULDocument.cpp    1.761
mozilla/content/xul/document/src/nsXULDocument.h      1.194