minor cleanup of ForwardReferences code in nsXULDocument

RESOLVED FIXED in mozilla1.9alpha5

Status

()

Core
XUL
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Nickolay_Ponomarev, Assigned: Nickolay_Ponomarev)

Tracking

Trunk
mozilla1.9alpha5
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
Created attachment 262378 [details] [diff] [review]
patch

- move AddForwardReference and ResolveForwardReferences from nsIXULDocument to nsXULDocument, make them non-virtual.
- make mForwardReferences an nsTArray<nsAutoPtr<nsForwardReference> > instead of nsAutoVoidArray.
Attachment #262378 - Flags: superreview?(bzbarsky)
Attachment #262378 - Flags: review?(Olli.Pettay)
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

11 years ago
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.
(Assignee)

Comment 3

11 years ago
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...
(Assignee)

Updated

11 years ago
Attachment #262378 - Flags: superreview?(bzbarsky) → superreview?(roc)

Comment 4

11 years ago
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()
Attachment #262378 - Flags: review?(Olli.Pettay) → review+
Attachment #262378 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 5

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Target Milestone: --- → mozilla1.9alpha5

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.