Status

()

defect
RESOLVED FIXED
11 years ago
Last year

People

(Reporter: roc, Assigned: Swatinem)

Tracking

(Depends on 3 bugs)

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(24 attachments, 33 obsolete attachments)

111.19 KB, patch
Swatinem
: review+
Swatinem
: superreview+
Details | Diff | Splinter Review
597 bytes, patch
roc
: review+
Details | Diff | Splinter Review
50.55 KB, patch
Swatinem
: review+
Swatinem
: superreview+
Details | Diff | Splinter Review
28.82 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.84 KB, patch
vlad
: review+
Details | Diff | Splinter Review
23.43 KB, patch
benjamin
: review+
benjamin
: superreview+
Details | Diff | Splinter Review
5.25 KB, patch
benjamin
: review+
benjamin
: superreview+
Details | Diff | Splinter Review
59.20 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
1.41 KB, patch
benjamin
: review+
benjamin
: superreview+
Details | Diff | Splinter Review
28.49 KB, patch
Swatinem
: review+
Swatinem
: superreview+
Details | Diff | Splinter Review
114.89 KB, patch
Swatinem
: review+
sicking
: superreview+
Details | Diff | Splinter Review
791 bytes, patch
sicking
: review+
sicking
: superreview+
Details | Diff | Splinter Review
1.10 KB, patch
Details | Diff | Splinter Review
1.13 KB, patch
sicking
: review+
sicking
: superreview+
Details | Diff | Splinter Review
11.28 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
17.69 KB, patch
vlad
: review+
Details | Diff | Splinter Review
4.98 KB, patch
Swatinem
: review+
Swatinem
: superreview+
Details | Diff | Splinter Review
13.81 KB, patch
jaas
: review+
Details | Diff | Splinter Review
18.49 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
7.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.73 KB, patch
benjamin
: review+
benjamin
: superreview+
Details | Diff | Splinter Review
56.27 KB, patch
Swatinem
: review+
Details | Diff | Splinter Review
72.14 KB, patch
Swatinem
: superreview+
Details | Diff | Splinter Review
8.19 KB, patch
Ehsan
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review
Everyone using nsVoidArray should use nsTArray<T*> instead.
Or possibly nsTPtrArray<T> if they need SafeElementAt.
Taking this bug, I have a patch in the works.
Assignee: nobody → arpad.borsos
Status: NEW → ASSIGNED
I'm kind of confused by this build error:
../mathml/base/src/libgkmathmlbase_s.a(nsMathMLmtableFrame.o):(.data.rel.ro._ZTV19nsMathMLmtableFrame[vtable for nsMathMLmtableFrame]+0x508): undefined reference to `nsTableFrame::InsertCells(nsTArray<nsTableCellFrame*>&, int, int)'
/usr/bin/ld: libgklayout.so: hidden symbol `nsTableFrame::InsertCells(nsTArray<nsTableCellFrame*>&, int, int)' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output
collect2: ld returned 1 exit status
make[5]: *** [libgklayout.so] Error 1

It builds fine with opt builds, the error only occurs in debug builds. What is wrong there?
First of all, if you are doing these changes by hand (rather than automatically using a script), please don't do this as one huge patch. It will be impossible to review.

As I'm sure you've noticed, the differences between nsTArray and nsVoidArray are actually quite big, apart from their names. For example nsVoidArray::ReplaceElementAt will automatically grow the array if an out-of-bounds insertion index is used. nsTArray::ReplaceElementAt will crash (I think without asserting even :( ).

There are also performance differences in that they use different algorithms for growing the allocated buffer. If we check in a mega-patch that regresses performance by 3%, then what?

I'd recommend creating a set of smaller patches that just change a few instances at a time over from nsVoidArray to nsTArray. This way we can also notice cases when we should use other template arguments, such as nsTArray<nsAutoPtr<T>> or even nsTArray<T>.


As for the compile error, not sure :(. Do you have a patch that we can look at?
Yes, I am doing this by hand, one module at a time. This is layout so far.
The Patch segfaults and as the debug build fails I wasn't able to track that down so far.
Yeah, it's exactly issues like that that is what I'm concerned about. I even forgot to mention the biggest difference between nsVoidArray and nsTArray:

nsVoidArray::ElementAt is out-of-bounds safe, whereas nsTArray::ElementAt is not, it just asserts and crashes.

Since the patch is fully in layout code, it is of course the layout folks call if they think the current patch is acceptable. But I'd really recommend splitting the patch up into much smaller patches. Such as doing just 2-3 arrays at a time.
I'll review a patch of almost any size if it has passed our tests.

In this case I suggest breaking it up to isolate the failures.

Is the build failure on x86-64 by any chance?
Yes, I am on x86_64.
Karl is an x86_64 wizard who might be able to help.
This patch includes everything in layout except layout/tables.
The patch does not regress reftest or mochitest-browser-chrome on my system.

> But I'd really recommend
> splitting the patch up into much smaller patches. Such as doing just 2-3 arrays

Wouldn't that be too much overkill?
Attachment #359085 - Attachment is obsolete: true
Attachment #359895 - Flags: superreview?(roc)
Attachment #359895 - Flags: review?(roc)
+      childList.AppendElement((nsIFrame*)nsnull);
+    mCurrentEventFrameStack.ReplaceElementsAt(i, 1, (nsIFrame*)nsnull);
+      mCurrentEventFrameStack.ReplaceElementsAt(i, 1, (nsIFrame*)nsnull);
+    gStretchyOperatorArray->ReplaceElementsAt(aIndex, 1, (OperatorData*)nsnull);

You don't need the casts in these places. At least I don't think you do. Does it not compile without these casts?

+  nsTArray<nsIFrame *> mLogicalFrames;
+  nsTArray<nsIFrame *> mVisualFrames;

No space between nsIFrame and *

-static int CompareByContentOrder(const void* aF1, const void* aF2, void* aDummy)
+static int CompareByContentOrder(const nsIFrame* f1, const nsIFrame* f2)

Keep the aF1/aF2 names for parameter naming style reasons.

+  PRBool Equals(const nsIFrame* a, const nsIFrame* b) const {
+    return CompareByContentOrder(a, b) == 0;
+  }
+  PRBool LessThan(const nsIFrame* a, const nsIFrame* b) const {
+    return CompareByContentOrder(a, b) == -1;
+  }

Likewise these need to be aA and aB or something.

But here, Equals should just check aA == aB, since in CompareByContentOrder any other return of 0 is an assertion failure.

+  StateEnumData enumData(aData);

We don't need enumData anymore here. enumData.data is just aData and enumData.change can be a local variable or just set directly into *aResult.

+  *_retval = (mRows[aIndex])->IsContainer();
+  *_retval = (mRows[aIndex])->IsOpen();
+  *_retval = (mRows[aIndex])->IsEmpty();
+  *_retval = (mRows[aIndex])->IsSeparator();
+  *_retval = (mRows[aRowIndex])->mParentIndex;
+  PRInt32 parentIndex = (mRows[aRowIndex])->mParentIndex;
+      if ((mRows[grandParentIndex])->IsOpen())
+    if ((mRows[i])->mContent == aContent) {

These unnecessary parens should be removed.

Some nice followup work would be to identify places where we're using nsTArray<T*> but the array really owns the T objects (i.e. when the array is deleted we iterate through the array doing 'delete' on each element). For those, we should use nsTArray<nsAutoPtr<T> > and avoid manual 'delete', or we may even be able to use nsTArray<T>.

I'm also pretty sure that mPrintDocList should be a direct nsTArray, not a pointer to a heap-allocated one.
(In reply to comment #11)
> You don't need the casts in these places. At least I don't think you do. Does
> it not compile without these casts?

Well those casts are really needed, otherwise the compile fails with this error:

../../dist/include/xpcom/nsTArray.h: In static member function 'static void nsTArrayElementTraits<E>::Construct(E*, const A&) [with A = int, E = nsIFrame*]':
../../dist/include/xpcom/nsTArray.h:776:   instantiated from 'void nsTArray<E>::AssignRange(PRUint32, PRUint32, const Item*) [with Item = int, E = nsIFrame*]'
../../dist/include/xpcom/nsTArray.h:554:   instantiated from 'E* nsTArray<E>::AppendElements(const Item*, PRUint32) [with Item = int, E = nsIFrame*]'
../../dist/include/xpcom/nsTArray.h:568:   instantiated from 'E* nsTArray<E>::AppendElement(const Item&) [with Item = int, E = nsIFrame*]'
/mnt/data/Coding/mozilla-central/layout/base/nsBidiPresUtils.cpp:891:   instantiated from here
../../dist/include/xpcom/nsTArray.h:193: error: invalid conversion from 'const int' to 'nsIFrame*'

> We don't need enumData anymore here. enumData.data is just aData and
> enumData.change can be a local variable or just set directly into *aResult.

StateEnumData is completely gone now.

> I'm also pretty sure that mPrintDocList should be a direct nsTArray, not a
> pointer to a heap-allocated one.

mPrintDocList is on nsPrintDatas stack now.

All the other nits are fixed as well.

> Some nice followup work would be to identify places where we're using
> nsTArray<T*> but the array really owns the T objects (i.e. when the array is
> deleted we iterate through the array doing 'delete' on each element). For
> those, we should use nsTArray<nsAutoPtr<T> > and avoid manual 'delete', or we
> may even be able to use nsTArray<T>.

I have already thought about this. But it's easier to do a straight port first.
Attachment #359895 - Attachment is obsolete: true
Attachment #359903 - Flags: superreview?(roc)
Attachment #359903 - Flags: review?(roc)
Attachment #359895 - Flags: superreview?(roc)
Attachment #359895 - Flags: review?(roc)
(In reply to comment #12)
> Well those casts are really needed, otherwise the compile fails with this
> error:

OK, I see the problem now.

For the ReplaceElementsAt cases, I think you can just write array[i] = nsnull instead. That only leaves the (nsIFrame*)nsnull case, which is not worth worrying about.
Using array[i] = nsnull; now.
Attachment #359903 - Attachment is obsolete: true
Attachment #360066 - Flags: superreview?(roc)
Attachment #360066 - Flags: review?(roc)
Attachment #359903 - Flags: superreview?(roc)
Attachment #359903 - Flags: review?(roc)
Comment on attachment 360066 [details] [diff] [review]
get rid of nsVoidArray, layout part without tables, v3

+    return CompareByContentOrder(aA, aB) == -1;

< 0
Attachment #360066 - Flags: superreview?(roc)
Attachment #360066 - Flags: superreview+
Attachment #360066 - Flags: review?(roc)
Attachment #360066 - Flags: review+
hg exported patch with last nit fixed.
Attachment #360066 - Attachment is obsolete: true
Attachment #360067 - Flags: superreview+
Attachment #360067 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment on attachment 360067 [details] [diff] [review]
layout part, for checkin
[Checkin: Comment 17]


http://hg.mozilla.org/mozilla-central/rev/a206aff7a9c6
Attachment #360067 - Attachment description: layout part, for checkin → layout part, for checkin [Checkin: Comment 17]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
Thanks for checkin Serge, but this is not fixed yet. There is a lot more to do than just layout. I will start porting other modules when I have time.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
(In reply to comment #3)
> I'm kind of confused by this build error:
> ../mathml/base/src/libgkmathmlbase_s.a(nsMathMLmtableFrame.o):(.data.rel.ro._ZTV19nsMathMLmtableFrame[vtable
> for nsMathMLmtableFrame]+0x508): undefined reference to
> `nsTableFrame::InsertCells(nsTArray<nsTableCellFrame*>&, int, int)'
> /usr/bin/ld: libgklayout.so: hidden symbol
> `nsTableFrame::InsertCells(nsTArray<nsTableCellFrame*>&, int, int)' isn't
> defined
> /usr/bin/ld: final link failed: Nonrepresentable section on output
> collect2: ld returned 1 exit status
> make[5]: *** [libgklayout.so] Error 1
> 
> It builds fine with opt builds, the error only occurs in debug builds. What is
> wrong there?

(In reply to comment #5)
> Created an attachment (id=359085) [details]
> get rid of nsVoidArray, layout part
> 
> Yes, I am doing this by hand, one module at a time. This is layout so far.
> The Patch segfaults and as the debug build fails I wasn't able to track that
> down so far.

I applied attachment 359085 [details] [diff] [review], but only needed the two small changes for a successful debug build with gcc-4.1.2 and gcc-4.2.4:

layout/generic/nsFrame.cpp:
-      aRules.InsertElementAt(&aRule, ruleX);
+      aRules.InsertElementAt(ruleX, &aRule);
layout/tables/nsCellMap.cpp:
-  printf(" cols in cache %d\n", mTableFrame.GetColCache().Count());
+  printf(" cols in cache %d\n", mTableFrame.GetColCache().Length());

Are you still seeing the "isn't defined" error with layout/tables changes?
(In reply to comment #19)
> layout/generic/nsFrame.cpp:
> -      aRules.InsertElementAt(&aRule, ruleX);
> +      aRules.InsertElementAt(ruleX, &aRule);

This change was already included in the checked-in patch.

> layout/tables/nsCellMap.cpp:
> -  printf(" cols in cache %d\n", mTableFrame.GetColCache().Count());
> +  printf(" cols in cache %d\n", mTableFrame.GetColCache().Length());

Thanks for spotting this one. Compiles fine, and I was able to track down the crasher. The attached patch now runs reftest and mochitest-browser-chrome without regressing.
Attachment #360488 - Flags: superreview?(roc)
Attachment #360488 - Flags: review?(roc)
Comment on attachment 360067 [details] [diff] [review]
layout part, for checkin
[Checkin: Comment 17]

>-  nsVoidArray list(kidCount);
>+  nsTArray<inDOMViewNode*> list(kidCount);
nsVoidArray's constructor sets its length.
nsTArray's constructor only sets its capactity.
This causes the replacements to fail...
ReplaceElementsAt(n, 1, e); is horribly inefficient anyway, but the second call was wrong anyway as per my previous comment.
Attachment #360500 - Flags: superreview?(roc)
Attachment #360500 - Flags: review?(roc)
Comment on attachment 360500 [details] [diff] [review]
Fix DOM Inspector
[Checkin: Comment 26]

oops, sorry
Attachment #360500 - Flags: superreview?(roc)
Attachment #360500 - Flags: superreview+
Attachment #360500 - Flags: review?(roc)
Attachment #360500 - Flags: review+
Attachment #360488 - Flags: superreview?(roc)
Attachment #360488 - Flags: superreview+
Attachment #360488 - Flags: review?(roc)
Attachment #360488 - Flags: review+
Comment on attachment 360488 [details] [diff] [review]
get rid of nsVoidArray, layout/tables part

+      cellChildren.AppendElement((nsTableCellFrame*)childFrame);

static_cast

A nice followup bug would be to replace IS_TABLE_CELL and the cast with do_QueryFrame.

Similar for the cast to nsTableRowFrame*.
hg exported patch with last nits fixed.
Attachment #360488 - Attachment is obsolete: true
Attachment #360685 - Flags: superreview+
Attachment #360685 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment on attachment 360500 [details] [diff] [review]
Fix DOM Inspector
[Checkin: Comment 26]

Pushed changeset 8b72dc73cfa1 to mozilla-central.
Keywords: checkin-needed
Whiteboard: [needs landing]
Posted patch netwerk part (obsolete) — Splinter Review
Attachment #362563 - Flags: superreview?(cbiesinger)
Attachment #362563 - Flags: review?(cbiesinger)
Posted patch xpfe part (obsolete) — Splinter Review
Attachment #362577 - Flags: superreview?(neil)
Attachment #362577 - Flags: review?(neil)
This patch may need some testing on alternative platforms. I'm only building and testing on linux, where this patch and the other two above do not regress reftest or mochitest-browser-chrome.
The patch changes some stuff in the widget implementation of windows and some exotic platforms (beos, os/2). It would be very nice if someone could confirm that this patch does not break those platforms.
Attachment #362578 - Flags: superreview?(roc)
Attachment #362578 - Flags: review?(roc)
Comment on attachment 362577 [details] [diff] [review]
xpfe part

You managed to fix the trailing whitespace on 11 of the lines you changed, but it looks as if 5 escaped your purge ;-)

>-  while (++index < mEnumeratorList.Count()) 
>-    ((nsAppShellWindowEnumerator*)mEnumeratorList[index])->WindowRemoved(inInfo);
>+  while (++index < PRInt32(mEnumeratorList.Length()))
>+    mEnumeratorList[index]->WindowRemoved(inInfo);
I would prefer it if you could change index/count/length variables from PRInt32 to PRUint32 to match the nsTArray API, rather than adding casts.

>diff -r 81f3ad533aab -r d22be52dc84d xpfe/bootstrap/appleevents/nsDocLoadObserver.cpp
I don't actually have a Mac, so would you mind getting an additional review?
Attachment #362577 - Flags: superreview?(neil)
Attachment #362577 - Flags: superreview+
Attachment #362577 - Flags: review?(neil)
Attachment #362577 - Flags: review+
Posted patch xpfe part, v2 (obsolete) — Splinter Review
This patch has more whitespace cleanup and fixes the explicit casts. There are however still a lot of implicit casts all around the tree. Some are needed for iterating from the top down. I left the implicit casts in. Honestly, I don't want to chase down un/signed warnings all day...

Could you please assign r? accordingly? I don't know which of the xpfe peers are running macs.
Attachment #362577 - Attachment is obsolete: true
+    nsTPtrArray<nsCString> mDataFlavors;

Mmm, that looks like a good candidate for conversion to nsTArray<nsCString>. Later.

+    nsTArray<nsString*> mDataFlavors;

That too!

+    nsTPtrArray<IDataObject> mDataObjects;

I guess this could become nsTArray<nsRefPtr<IDataObject> > perhaps.
Attachment #362578 - Flags: superreview?(roc)
Attachment #362578 - Flags: superreview+
Attachment #362578 - Flags: review?(roc)
Attachment #362578 - Flags: review+
Maybe you could hop on IRC and see if anyone's around to help you with Windows.
Attachment #362713 - Flags: superreview?(vladimir)
Attachment #362713 - Flags: review?(vladimir)
Attachment #362714 - Flags: superreview?(benjamin)
Attachment #362714 - Flags: review?(benjamin)
Attachment #362715 - Flags: superreview?(benjamin)
Attachment #362715 - Flags: review?(benjamin)
Attachment #362713 - Flags: superreview?(vladimir)
Attachment #362713 - Flags: superreview+
Attachment #362713 - Flags: review?(vladimir)
Attachment #362713 - Flags: review+
Attachment #362563 - Flags: superreview?(cbiesinger)
Attachment #362563 - Flags: review?(cbiesinger)
Attachment #362563 - Flags: review-
Comment on attachment 362563 [details] [diff] [review]
netwerk part

Very nice!

General comment on the patch: prefer ++i over i++


+++ b/netwerk/base/src/nsLoadGroup.cpp	Mon Feb 16 12:32:43 2009 +0100
+AppendRequestsToArray(PLDHashTable *table, PLDHashEntryHdr *hdr,
                           PRUint32 number, void *arg)

incorrect indentation on the second line

+            nsISupports *s = static_cast<nsISupports *>(requests[i]);

why not just store this in an nsIRequest* and avoid the cast? That said... maybe the array should store an nsCOMPtr<nsIRequest> instead?

(Next time can you diff with more context? And adding the -p option would be nice too)

+++ b/netwerk/base/src/nsProtocolProxyService.cpp	Mon Feb 16 12:32:43 2009 +0100
+            PRUint32 i, len = mHostFiltersArray.Length();

Not really a fan of mixing initialized and uninitialized variables in the same declaration. How about:

  PRUint32 len = mHostFiltersArray.Length();
  for (PRUint32 i = ...)

+                if (elem)
+                    delete elem;

don't need the if, delete NULL doesn't crash.

You should use nsTArray<nsAutoPtr<HostInfo> > though, to avoid having to call delete yourself

+        PRUint32 i, len = mHostFiltersArray.Length();

see above

+++ b/netwerk/protocol/http/src/nsHttpAuthCache.cpp	Mon Feb 16 12:32:43 2009 +0100

mList should be a nsTArray<nsAutoPtr<nsHttpAuthEntry> >

+++ b/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp	Mon Feb 16 12:32:43 2009 +0100
-            pendingQ.InsertElementAt(trans, i+1);
+            pendingQ.InsertElementsAt(i+1, 1, trans);
and
-    pendingQ.InsertElementAt(trans, 0);
+    pendingQ.InsertElementsAt(0, 1, trans);


Why? nsTArray does have an InsertElementAt function.

-                ent->mPendingQ.InsertElementAt(trans, i);
+                ent->mPendingQ.InsertElementsAt(i, 1, trans);

here too

+++ b/netwerk/streamconv/converters/nsTXTToHTMLConv.h	Mon Feb 16 12:32:43 2009 +0100
+    nsTArray<convToken*>            mTokens;   // list of tokens to search for

should also be an nsAutoPtr
> why not just store this in an nsIRequest* and avoid the cast? That said...
> maybe the array should store an nsCOMPtr<nsIRequest> instead?

I don't seem to get nsCOMPtr working correctly. I removed the casts however.
RequestMapEntry::mKey is a nsCOMPtr<nsIRequest> already.
Attachment #362563 - Attachment is obsolete: true
Attachment #363287 - Flags: review?(cbiesinger)
Comment on attachment 363287 [details] [diff] [review]
netwerk part, v2
[Checkin: Comment 43]

nsLoadGroup.cpp
+            NS_RELEASE(requests[i]);

Note that NS_RELEASE has the side-effect of setting the pointer to NULL (so this is not equivalent to the previous version), but that's probably a good thing here anyway.
Attachment #363287 - Flags: review?(cbiesinger) → review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #360500 - Attachment description: Fix DOM Inspector → Fix DOM Inspector [Checkin: Comment 26]
Attachment #360685 - Attachment description: layout/tables part, for checkin → layout/tables part [Checkin: Comment 27]
Attachment #362578 - Attachment description: widget part → widget part [Checkin: Comment 41]
Comment on attachment 362713 [details] [diff] [review]
gfx part
[Checkin: Comment 42]


http://hg.mozilla.org/mozilla-central/rev/7366df357e91
Attachment #362713 - Attachment description: gfx part → gfx part [Checkin: Comment 42]
Comment on attachment 363287 [details] [diff] [review]
netwerk part, v2
[Checkin: Comment 43]


http://hg.mozilla.org/mozilla-central/rev/187775ec9ac5
Attachment #363287 - Attachment description: netwerk part, v2 → netwerk part, v2 [Checkin: Comment 43]
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #362715 - Flags: superreview?(benjamin)
Attachment #362715 - Flags: superreview+
Attachment #362715 - Flags: review?(benjamin)
Attachment #362715 - Flags: review+
Attachment #362714 - Flags: superreview?(benjamin)
Attachment #362714 - Flags: superreview+
Attachment #362714 - Flags: review?(benjamin)
Attachment #362714 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Posted patch content part 1 (obsolete) — Splinter Review
This is the first part of content minus base and xul. Runs reftest fine.
The base and/or xul part has a crasher in there somewhere which I need to nail down before attaching another patch.
Also requesting review from jwatt for the svg parts.
Attachment #364061 - Flags: superreview?(peterv)
Attachment #364061 - Flags: review?(peterv)
Attachment #364061 - Flags: review?(jwatt)
Posted patch content part 2 (obsolete) — Splinter Review
This is the working part of base and xul. Runs reftest and content mochitests fine.
Attachment #364083 - Flags: superreview?(peterv)
Attachment #364083 - Flags: review?(peterv)
Posted patch content crasher (obsolete) — Splinter Review
This is the part that crashes.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f3e827e5780 (LWP 27429)]
0x00007f3e70306c25 in nsXULDocument::AddBroadcastListenerFor (this=0xf56170, 
    aBroadcaster=0x10c6c10, aListener=0x1096390, aAttr=@0x7fff8a811930)
    at /home/swatinem/Coding/mozilla-central/content/xul/document/src/nsXULDocument.cpp:839
839	        bl = entry->mListeners[i];
(gdb) p entry->mListeners.Length()
$8 = 14994240

I'm not quite sure what is going on there. Maybe there is something wrong with the in-place allocation?
About an hour ago I also got a crash in nsXULDocument::GetElementById inside the CallQueryInterface(content, aReturn) call, that suggested a problem with nsIdentifierMapEntry::mIdContentList. But I can't reproduce that crash right now.
Comment on attachment 362714 [details] [diff] [review]
embedding part
[Checkin: Comment 47]


http://hg.mozilla.org/mozilla-central/rev/24c37ac3bc3f
Attachment #362714 - Attachment description: embedding part → embedding part [Checkin: Comment 47]
Attachment #362715 - Attachment description: toolkit part → toolkit part [Checkin: Comment 48]
Comment on attachment 362715 [details] [diff] [review]
toolkit part
[Checkin: See comment 48]


http://hg.mozilla.org/mozilla-central/rev/2603878ec5fc

after removing
{
patching file toolkit/components/downloads/src/nsDownloadManager.cpp
Hunk #1 FAILED at 62
1 out of 1 hunks FAILED
}
which another bug already did.
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #362596 - Flags: review?(neil)
Comment on attachment 362596 [details] [diff] [review]
xpfe part, v2

See
(In reply to comment #32)
> Could you please assign r? accordingly? I don't know which of the xpfe peers
> are running macs.
Attachment #362715 - Attachment description: toolkit part [Checkin: Comment 48] → toolkit part [Checkin: See comment 48]
After the 2-25-2009 embedding checkin for this, Fennec on Win32 doesn't build.  There's two references to sBrowserList.Count() in /embedding/browser/activex/src/control/PromptService.cpp that need to be changed to .Length() -- see http://mxr.mozilla.org/mozilla-central/search?string=sBrowserList.Count
This fixes the problem reported in comment #50, sorry for that.
Attachment #364275 - Flags: superreview?(benjamin)
Attachment #364275 - Flags: review?(benjamin)
Attachment #364061 - Flags: review?(jwatt) → review+
Comment on attachment 364061 [details] [diff] [review]
content part 1

r=jwatt for the SVG changes, with the following comments.

>-  nsAutoVoidArray mLengths;
>+  nsAutoTArray<nsISVGLength*, 8> mLengths;

Use 1 instead of 8 here. For lengths, the majority of the time there will only be one value in the list, and when there's more than one, it will likely be more than 8.

>-  if (count<=0) return NS_OK;
>+  if (count==0) return NS_OK;

Can you put spaces around the operator while you're here? Same in nsSVGNumberList.cpp.

>-  if (!NS_FAILED(rv = mNumbers.InsertElementAt((void*)aElement, index)))
>+  if (mNumbers.InsertElementAt(index, aElement) == nsnull)

I prefer:

  +  if (!mNumbers.InsertElementAt(index, aElement))

Same in nsSVGTransformList.cpp.
Attachment #364275 - Flags: superreview?(benjamin)
Attachment #364275 - Flags: superreview+
Attachment #364275 - Flags: review?(benjamin)
Attachment #364275 - Flags: review+
Lets get the ActiveX fixup in.
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #362596 - Flags: superreview+
Attachment #362596 - Flags: review?(neil)
Attachment #362596 - Flags: review+
Comment on attachment 362596 [details] [diff] [review]
xpfe part, v2

So, it turns out that the appleevents stuff is being removed in bug 363747 anyway, so the rest of this patch can land as per the previous r+sr.

Thanks for looking at some of those variable types, it's always good to avoid introducing new compiler warnings.

>+  PRUint32 index = 0;
>+  while (index < mEnumeratorList.Length()) {
>+    mEnumeratorList[index]->WindowRemoved(inInfo);
>+    index++;
>+  }
[This could probably have been written as a for loop, couldn't it?]
Comment on attachment 364275 [details] [diff] [review]
activex fixup
[Checkin: Comment 55]


http://hg.mozilla.org/mozilla-central/rev/21cd341f555c
Attachment #364275 - Attachment description: activex fixup → activex fixup [Checkin: Comment 55]
Keywords: checkin-needed
Whiteboard: [needs landing]
Updated after the appleevents remove, forwarding neils r+.
Attachment #362596 - Attachment is obsolete: true
Attachment #364935 - Flags: superreview+
Attachment #364935 - Flags: review+
Blocks: 481881
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment on attachment 364935 [details] [diff] [review]
xpfe part, for checkin
[Checkin: Comment 57]


http://hg.mozilla.org/mozilla-central/rev/6874f88f06a7
Attachment #364935 - Attachment description: xpfe part, for checkin → xpfe part, for checkin [Checkin: Comment 57]
Keywords: checkin-needed
Whiteboard: [needs landing]
Posted patch content part, v2 (obsolete) — Splinter Review
In another bug I learned to be more careful with classes that have pointers to themselves.
Looks like the hashtables moved the nsAutoTArray around which then broke.
Fixed that, integrated jwatts comments, and folded the content patches together to ease review.

forwarded jwatts r+.
Attachment #364061 - Attachment is obsolete: true
Attachment #364083 - Attachment is obsolete: true
Attachment #364086 - Attachment is obsolete: true
Attachment #366181 - Flags: superreview?(jst)
Attachment #366181 - Flags: review+
Attachment #364061 - Flags: superreview?(peterv)
Attachment #364061 - Flags: review?(peterv)
Attachment #364083 - Flags: superreview?(peterv)
Attachment #364083 - Flags: review?(peterv)
Comment on attachment 366181 [details] [diff] [review]
content part, v2

Sicking will deal with this one...
Attachment #366181 - Flags: superreview?(jst) → superreview?(jonas)
Comment on attachment 366181 [details] [diff] [review]
content part, v2

General comment:

There seems to be quite a few places where you could take advantage of nsTArray.IsEmpty, rather than using .Length() == 0 or .Length() > 0.

>diff --git a/content/base/src/nsContentIterator.cpp b/content/base/src/nsContentIterator.cpp
>@@ -627,9 +628,9 @@ nsContentIterator::GetNextSibling(nsINod
> 
>   if (aIndexes)
>   {
>-    NS_ASSERTION(aIndexes->Count() > 0, "ContentIterator stack underflow");
>+    NS_ASSERTION(aIndexes->Length() > 0, "ContentIterator stack underflow");
>     // use the last entry on the Indexes array for the current index
>-    indx = NS_PTR_TO_INT32((*aIndexes)[aIndexes->Count()-1]);
>+    indx = (*aIndexes)[aIndexes->Length()-1];
>   }
>   else
>     indx = mCachedIndex;

Are you sure this will never be passed an empty array? The old code would just assert and return 0. The new code will crash exploitably. There's plenty of this pattern in this class.


>diff --git a/content/base/src/nsDocument.h b/content/base/src/nsDocument.h
>@@ -330,7 +330,7 @@ private:
> 
>   // The single element ID_NOT_IN_DOCUMENT, or empty to indicate we
>   // don't know what element(s) have this key as an ID
>-  nsSmallVoidArray mIdContentList;
>+  nsTPtrArray<nsIContent> mIdContentList;

Ugh, actually this would be a performance/memory hit. nsSmallVoidArray has an optimization that allows us to not do any allocations when there's only one item in the array, which is the by far most common case here.

So until we have an nsTArray variant with a similar optimization you probably don't want to change this :(


>diff --git a/content/base/src/nsHTMLContentSerializer.cpp b/content/base/src/nsHTMLContentSerializer.cpp
>@@ -101,13 +101,13 @@ nsHTMLContentSerializer::nsHTMLContentSe
> 
> nsHTMLContentSerializer::~nsHTMLContentSerializer()
> {
>-  NS_ASSERTION(mOLStateStack.Count() == 0, "Expected OL State stack to be empty");
>-  if (mOLStateStack.Count() > 0){
>-    for (PRInt32 i = 0; i < mOLStateStack.Count(); i++){
>-      olState* state = (olState*)mOLStateStack[i];
>+  NS_ASSERTION(mOLStateStack.Length() == 0, "Expected OL State stack to be empty");
>+  if (mOLStateStack.Length() > 0){
>+    for (PRUint32 i = 0; i < mOLStateStack.Length(); i++){
>+      olState* state = mOLStateStack[i];
>       delete state;

just do |delete mOLStateStack[i];|

>-      mOLStateStack.RemoveElementAt(i);
>     }
>+    mOLStateStack.Clear();
>   }
> }

Do you even need this Clear() call? Or even better, could you make this into

nsAutoTArray<olState, 8> mOLStateStack;

That would avoid all manual memory management. Feel free to leave this for later


>diff --git a/content/base/src/nsTreeWalker.h b/content/base/src/nsTreeWalker.h
>@@ -169,8 +169,7 @@ private:
>     void SetChildIndex(PRInt32 aIndexPos, PRInt32 aChildIndex)
>     {
>         if (aIndexPos != -1)
>-            mPossibleIndexes.ReplaceElementAt(NS_INT32_TO_PTR(aChildIndex),
>-                                              aIndexPos);
>+            mPossibleIndexes.ElementAt(aIndexPos) = aChildIndex;

These are not equivalent. nsVoidArray::ReplaceElementAt will grow the array if the index is greater than the current length, and pad with null. IIRC this function relies on that.

Apart from the bound safetyness this is the biggest difference between the nsTArray API and the nsVoidArray API. I hope you haven't made this same error elsewhere.

It's really a pity that nsTArray doesn't assert more for out-of-boundness, we should add that.


>diff --git a/content/base/src/nsXMLNameSpaceMap.cpp b/content/base/src/nsXMLNameSpaceMap.cpp
>@@ -101,7 +100,7 @@ nsXMLNameSpaceMap::AddPrefix(nsIAtom *aP
>     foundEntry = new nsNameSpaceEntry(aPrefix);
>     NS_ENSURE_TRUE(foundEntry, NS_ERROR_OUT_OF_MEMORY);
> 
>-    if (!mNameSpaces.AppendElement(foundEntry)) {
>+    if (mNameSpaces.AppendElement(foundEntry) == nsnull) {
>       delete foundEntry;
>       return NS_ERROR_OUT_OF_MEMORY;
>     }

I think you can make mNameSpaces into an nsTArray<nsNameSpaceEntry> to avoid the manual memory management. Feel free to leave this for later


>diff --git a/content/html/document/src/nsHTMLDocument.h b/content/html/document/src/nsHTMLDocument.h
>@@ -342,7 +343,7 @@ protected:
>   // finishes processing that script.
>   PRUint32 mWriteLevel;
> 
>-  nsSmallVoidArray mPendingScripts;
>+  nsAutoTArray<nsIScriptElement*, 1> mPendingScripts;

These aren't really equivalent as part of the point with the former is that it's just 1 word big. The latter is 4. Size isn't really a concern here though as this class is huge anyway, so this change is fine.

>diff --git a/content/svg/content/src/nsSVGValue.cpp b/content/svg/content/src/nsSVGValue.cpp
>@@ -52,27 +52,26 @@ void
> void
> nsSVGValue::ReleaseObservers()
> {
>-  PRInt32 count = mObservers.Count();
>-  PRInt32 i;
>+  PRUint32 count = mObservers.Length();
>+  PRUint32 i;
>   for (i = 0; i < count; ++i) {
>-    nsIWeakReference* wr = static_cast<nsIWeakReference*>(mObservers.ElementAt(i));
>+    nsIWeakReference* wr = mObservers.ElementAt(i);
>     NS_RELEASE(wr);
>   }
>-  while (i)
>-    mObservers.RemoveElementAt(--i);
>+  mObservers.Clear();
> }

Seems like mObservers should be an nsCOMArray<nsIWeakReference>, that will avoid the manual refcounting. Feel free to leave this for later


>diff --git a/content/svg/content/src/nsSVGValue.h b/content/svg/content/src/nsSVGValue.h
>@@ -80,7 +80,7 @@ private:
> private:
>   virtual void OnDidModify(){} // hook that will be called before observers are notified
>   
>-  nsSmallVoidArray mObservers;
>+  nsAutoTArray<nsIWeakReference*, 1> mObservers;
>   PRInt32 mModifyNestCount;
> };

Again, these are not equivalent since the latter is 3 words bigger. And I don't know how many nsSVGValues there usually are so I don't know if this is an issue.

jwatt should be able to advice.

>diff --git a/content/xslt/src/xslt/txInstructions.h b/content/xslt/src/xslt/txInstructions.h
>--- a/content/xslt/src/xslt/txInstructions.h
>+++ b/content/xslt/src/xslt/txInstructions.h
>@@ -304,7 +305,7 @@ public:
>         nsAutoPtr<Expr> mCaseOrderExpr;
>     };
>     
>-    nsVoidArray mSortKeys;
>+    nsTArray<SortKey*> mSortKeys;

You can make this into nsTArray<SortKey> instead. Feel free to leave this for later


>diff --git a/content/xul/document/src/nsXULDocument.cpp b/content/xul/document/src/nsXULDocument.cpp

>@@ -827,17 +824,16 @@ nsXULDocument::AddBroadcastListenerFor(n
> 
>         entry->mBroadcaster = aBroadcaster;
> 
>-        // N.B. placement new to construct the nsSmallVoidArray object
>-        // in-place
>-        new (&entry->mListeners) nsSmallVoidArray();
>+        // N.B. placement new to construct the nsTArray object in-place
>+        new (&entry->mListeners) nsTArray<BroadcastListener*>();

Shouldn't this be nsTArray<BroadcastListener>()?  (note, no '*').

>diff --git a/content/xul/document/src/nsXULDocument.h b/content/xul/document/src/nsXULDocument.h
>@@ -103,7 +103,7 @@ public:
>   PRBool RemoveContent(nsIContent* aContent);
> 
> private:
>-  nsSmallVoidArray mRefContentList;
>+  nsTPtrArray<nsIContent> mRefContentList;

here too I'm a little worried about memory consumption since I don't know how many instances of these we have.

>diff --git a/content/xul/templates/src/nsTemplateRule.h b/content/xul/templates/src/nsTemplateRule.h
>@@ -277,7 +276,7 @@ class nsTemplateQuerySet
> class nsTemplateQuerySet
> {
> protected:
>-    nsVoidArray mRules; // rules owned by nsTemplateQuerySet
>+    nsTArray<nsTemplateRule*> mRules; // rules owned by nsTemplateQuerySet

This one could be converted to nsTArray<nsTemplateRule> too, though it's quite a bit of work so probably better for a separate patch.


You don't need to convert any of the classes to use inline storage (I.e. the ones with 'leave for later' comments). However I'd like to see another patch with the rest fixed.
Attachment #366181 - Flags: superreview?(jonas) → superreview-
Posted patch xpcom part (obsolete) — Splinter Review
This patch most notably changes nsCOMArray_base::mArray to nsTPtrArray<nsISupports>
As discussed on IRC, it may be worth looking into replacing nsCOMArray<T> with nsTArray<nsRefPtr<T> > or nsTArray<nsCOMPtr<T> >.
But I'm not sure what that would mean for performance/memory consumption/code size.
Attachment #366611 - Flags: superreview?(benjamin)
Attachment #366611 - Flags: review?(benjamin)
> This patch most notably changes nsCOMArray_base::mArray to
> nsTPtrArray<nsISupports>
> As discussed on IRC, it may be worth looking into replacing nsCOMArray<T> with
> nsTArray<nsRefPtr<T> > or nsTArray<nsCOMPtr<T> >.
> But I'm not sure what that would mean for performance/memory consumption/code
> size.

Have you checked that all the users of nsCOMArray is bounds-safe? Otherwise you probably need to add bounds checks in the nsCOMArray implementation.
(In reply to comment #62)
> Have you checked that all the users of nsCOMArray is bounds-safe? Otherwise you
> probably need to add bounds checks in the nsCOMArray implementation.

nsCOMArray_base::ObjectAt() used nsVoidArray::FastElementAt() which was not bounds safe, so I believe thats ok.
I've added a bounds-check in nsCOMArray_base::ReplaceObjectAt additionally to the one already in RemoveObjectAt(). I believe that are the only cases where it is needed.
Ah, good point. However InsertObject(s)At used to be bounds safe but is no longer. And ReplaceElementAt used to act different for indexes > .Length()
Attachment #366611 - Attachment is obsolete: true
Attachment #366611 - Flags: superreview?(benjamin)
Attachment #366611 - Flags: review?(benjamin)
Comment on attachment 366611 [details] [diff] [review]
xpcom part

Ah I see, so InsertObject(s)At should fail with out-of-bounds instead of automatically resizing the array, and ReplaceElementAt should try to resize the array first before failing.
I will upload an updated patch shortly.
(In reply to comment #65)
> (From update of attachment 366611 [details] [diff] [review])
> Ah I see, so InsertObject(s)At should fail with out-of-bounds instead of
> automatically resizing the array,

Note that nsTArray::InsertObject(s)At doesn't resize on out-of-bounds, it crashes with a buffer overflow which is probably exploitable.
Posted patch content part, v3 (obsolete) — Splinter Review
This patch has a lot more out-of-bounds checks. However, it still uses nsTArray as replacement for nsSmallVoidArray.
Attachment #366181 - Attachment is obsolete: true
Attachment #366833 - Flags: superreview?(jonas)
Attachment #366833 - Flags: review+
Where did you add bounds checks? Wanna avoid looking over the full patch again. Or did you only modify places that I mentioned in the initial review?
(In reply to comment #68)
> Where did you add bounds checks? Wanna avoid looking over the full patch again.
> Or did you only modify places that I mentioned in the initial review?

Those and some others where it was obvious that a non existent index can be passed to ElementAt. Not sure I have caught all cases though.
Could you attach an interdiff? If not I'd have to start the review over from scratch. That is certainly doable if needed though...
Posted patch interdiff (obsolete) — Splinter Review
Comment on attachment 366833 [details] [diff] [review]
content part, v3

>diff --git a/content/base/src/nsContentIterator.cpp b/content/base/src/nsContentIterator.cpp
>@@ -623,13 +624,12 @@ nsContentIterator::GetNextSibling(nsINod
>   if (!parent)
>     return nsnull;
> 
>-  PRInt32 indx;
>+  PRInt32 indx = 0;
> 
>-  if (aIndexes)
>+  if (aIndexes && !aIndexes->IsEmpty())
>   {
>-    NS_ASSERTION(aIndexes->Count() > 0, "ContentIterator stack underflow");
>     // use the last entry on the Indexes array for the current index
>-    indx = NS_PTR_TO_INT32((*aIndexes)[aIndexes->Count()-1]);
>+    indx = (*aIndexes)[aIndexes->Length()-1];
>   }

Please add
NS_ASSERTION(!aIndexes || !aIndexes->IsEmpty(),
             "ContentIterator stack underflow");

so that we still assert if that happens.

>diff --git a/content/base/src/nsTreeWalker.h b/content/base/src/nsTreeWalker.h
>@@ -168,9 +167,10 @@ private:
>      */
>     void SetChildIndex(PRInt32 aIndexPos, PRInt32 aChildIndex)
>     {
>-        if (aIndexPos != -1)
>-            mPossibleIndexes.ReplaceElementAt(NS_INT32_TO_PTR(aChildIndex),
>-                                              aIndexPos);
>+        if (aIndexPos > 0) {
>+            mPossibleIndexes.EnsureLengthAtLeast(aIndexPos-1);
>+            mPossibleIndexes.ElementAt(aIndexPos) = aChildIndex;

Shouldn't that be EnsureLengthAtLeast(aIndexPos+1) ?


You can remove the bounds checks on txStack since I'm quite sure that never goes out-of-bounds.

sr=me with that fixed.

You could possibly do the same on nsContentIterator, but you'd have to check with someone from the layout team (roc, dbaron, bz) since I'm not sure how that class is called. But I'm fine with landing as is, that class could do with cleanup anyway.
Attachment #366833 - Flags: superreview?(jonas) → superreview+
Comment on attachment 366833 [details] [diff] [review]
content part, v3

Wait, forgot about the nsSmallVoidArray changes.

The change to nsIdentifierMapEntry (in nsDocument.h) to change mIdContentList I'm pretty sure is not acceptable. This one is also harder to measure since it depends on the web page.

The changes to BroadcasterMapEntry (in nsXULDocument.cpp) and nsRefMapEntry (in nsXULDocument.h) I'm less sure about, but I'd prefer to err on the side of caution unless there is data. It should be pretty trivial to get some data though, just see how many instances of those classes are created if you start the browser, create a couple of tabs, and then shut down.


As for nsIdentifierMapEntry, I think we'll want to create an nsTSmallPtrArray. It should be pretty trivial, though possibly better done in a separate bug.
Attachment #366833 - Flags: superreview+ → superreview-
So this is without the changes to the mentioned SmallVoidArrays.

> You can remove the bounds checks on txStack since I'm quite sure that never
> goes out-of-bounds.

I believe those could be useful together with the asserts that are already in there, so I left those in.

The other nits are fixed.
Attachment #366833 - Attachment is obsolete: true
Attachment #366997 - Attachment is obsolete: true
Attachment #368495 - Flags: superreview?(jonas)
Attachment #368495 - Flags: review+
Comment on attachment 368495 [details] [diff] [review]
content part, v4 [pushed: comment 76]

I'm not a big fan of having known to be unnecessary code in the tree, but I'd really like to see this patch land, and txStack isn't very heavily used.
Attachment #368495 - Flags: superreview?(jonas) → superreview+
Attachment #368495 - Attachment description: content part, v4 → content part, v4 [pushed: comment 76]
Comment on attachment 368495 [details] [diff] [review]
content part, v4 [pushed: comment 76]

>diff --git a/content/base/src/nsTreeWalker.h b/content/base/src/nsTreeWalker.h

>@@ -168,9 +167,10 @@ private:
>      */
>     void SetChildIndex(PRInt32 aIndexPos, PRInt32 aChildIndex)
>     {
>-        if (aIndexPos != -1)
>-            mPossibleIndexes.ReplaceElementAt(NS_INT32_TO_PTR(aChildIndex),
>-                                              aIndexPos);
>+        if (aIndexPos > 0) {
>+            mPossibleIndexes.EnsureLengthAtLeast(aIndexPos+1);

Shouldn't this be:
        if (aIndexPos >= 0 && mPossibleIndexes.EnsureLengthAtLeast(aIndexPos+1);
?

>+            mPossibleIndexes.ElementAt(aIndexPos) = aChildIndex;
>+        }
As per comment 77.
Attachment #368501 - Flags: superreview?
Attachment #368501 - Flags: review?
Attachment #368501 - Flags: superreview?(jonas)
Attachment #368501 - Flags: superreview?
Attachment #368501 - Flags: review?(jonas)
Attachment #368501 - Flags: review?
Attachment #368501 - Flags: superreview?(jonas)
Attachment #368501 - Flags: superreview+
Attachment #368501 - Flags: review?(jonas)
Attachment #368501 - Flags: review+
Attachment #368501 - Attachment description: content followup → content followup [pushed: comment 79]
As checked in: http://hg.mozilla.org/mozilla-central/rev/e29ec3dc42ee
Using an uninitialized iterator variable is not a good idea. :(
...\content\svg\content\src\nssvgnumberlist.cpp(388) : warning C4700: uninitialized local variable 'rv' used
Posted patch uninitialized variable fix (obsolete) — Splinter Review
Fix for comment 81.
Attachment #368664 - Flags: superreview?(jonas)
Attachment #368664 - Flags: review?(jonas)
Comment on attachment 368664 [details] [diff] [review]
uninitialized variable fix

>@@ -382,8 +382,10 @@ nsSVGNumberList::InsertElementAt(nsIDOMS
>   // list':
>   //  aElement->SetListOwner(this);
>   
>-  if (mNumbers.InsertElementAt(index, aElement))
>+  if (mNumbers.InsertElementAt(index, aElement)) {
>     NS_ADD_SVGVALUE_OBSERVER(aElement);
>+    rv = NS_OK;
>+  }
>   DidModify();
>   return rv;
> }

I'm really not a fan of functions that end by returning whatever value "happen" to be left in rv. This tends to lead to uninteded changes when code is inserted or removed, exactly like what you did. For example if someone inserted code before DidModify() that did |rv = Foo()|, but sometimes was able to recover from a returned error.

IMHO a better pattern here would be to use

if(!mNumbers.InsertElementAt(...)) {
  return NS_ERROR_OUT_OF_MEMORY;
}
...
return NS_OK;

values in rv should IMHO be treated as very short lived since its so heavily reused.
Attachment #368664 - Attachment is obsolete: true
Attachment #368665 - Flags: superreview?(jonas)
Attachment #368665 - Flags: review?(jonas)
Attachment #368664 - Flags: superreview?(jonas)
Attachment #368664 - Flags: review?(jonas)
Attachment #368665 - Flags: superreview?(jonas)
Attachment #368665 - Flags: superreview+
Attachment #368665 - Flags: review?(jonas)
Attachment #368665 - Flags: review+
Comment on attachment 368665 [details] [diff] [review]
uninitialized variable fix,v2 [pushed: comment 93]

Looks great. Sorry to be a pain in the ass about nits, but since I'm hoping for more patches from you, I'd rather comment on these things early on to have less comments later.
Looks like the patch introduced two new PRBool violations. Those look like false positives to me however.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsPlainTextSerializer.cpp#244
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsPlainTextSerializer.cpp#271

aStack is a nsTArray<PRPackedBool> and aStack.ElementAt returns a PRPackedBool&.
I don't know why prcheck complains here. Taras: can you explain to me whats wrong there?
(In reply to comment #86)
> Looks like the patch introduced two new PRBool violations. Those look like
> false positives to me however.
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsPlainTextSerializer.cpp#244
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsPlainTextSerializer.cpp#271
> 
> aStack is a nsTArray<PRPackedBool> and aStack.ElementAt returns a
> PRPackedBool&.
> I don't know why prcheck complains here. Taras: can you explain to me whats
> wrong there?

prcheck doesn't deal well with arrays, sorry for the false positive.
Posted patch editor part (obsolete) — Splinter Review
Attachment #368858 - Flags: superreview?(daniel)
Attachment #368858 - Flags: review?(daniel)
Posted patch xpcom part, v2 (obsolete) — Splinter Review
I hat more problems with nsCOMArray than originally thought, so I left it alone.
Attachment #368865 - Flags: superreview?(benjamin)
Attachment #368865 - Flags: review?(benjamin)
Attachment #369071 - Flags: superreview?(brendan)
Attachment #369071 - Flags: review?(brendan)
Posted patch parser part (obsolete) — Splinter Review
Attachment #369072 - Flags: superreview?(mrbkap)
Attachment #369072 - Flags: review?(mrbkap)
Attachment #369073 - Flags: superreview?
Attachment #369073 - Flags: review?
Attachment #369073 - Flags: superreview?(sspitzer)
Attachment #369073 - Flags: superreview?
Attachment #369073 - Flags: review?(sspitzer)
Attachment #369073 - Flags: review?
Comment on attachment 368665 [details] [diff] [review]
uninitialized variable fix,v2 [pushed: comment 93]

http://hg.mozilla.org/mozilla-central/rev/d14de45019a0
Attachment #368665 - Attachment description: uninitialized variable fix,v2 → uninitialized variable fix,v2 [pushed: comment 93]
Attachment #368520 - Attachment description: bustage fix as checked in → bustage fix as checked in [pushed: comment 80]
Comment on attachment 369071 [details] [diff] [review]
js part [pushed: comment 97]

XPConnect != JS Engine.

/be
Attachment #369071 - Flags: superreview?(mrbkap)
Attachment #369071 - Flags: superreview?(brendan)
Attachment #369071 - Flags: review?(mrbkap)
Attachment #369071 - Flags: review?(brendan)
Attachment #369071 - Flags: superreview?(mrbkap)
Attachment #369071 - Flags: superreview+
Attachment #369071 - Flags: review?(mrbkap)
Attachment #369071 - Flags: review+
Attachment #369072 - Flags: superreview?(mrbkap)
Attachment #369072 - Flags: superreview+
Attachment #369072 - Flags: review?(mrbkap)
Attachment #369072 - Flags: review+
Comment on attachment 369072 [details] [diff] [review]
parser part

>+  nsTArray<nsCOMPtr<nsIElementObserver> >* mObservers[NS_HTML_TAG_MAX + 1];

Seems like this would be better put as nsCOMArray<nsIElementObserver>* mObservers[NS_HTML_TAG_MAX + 1] to remove a level of indirection. In practice there aren't many observers, so losing the ability to say nsAutoTArray<..., 8> doesn't seem terrible to me.

r+sr=mrbkap with that.
Btw, I'm weakly in favor of keeping existing nsCOMArray users using nsCOMArray. But make nsCOMArray be based on nsTArray. The reason is that I suspect that we can make nsCOMArray<T> produce more optimized code than nsTArray<nsCOMPtr<T>>.

Though, I would also be ok with typedef-ing nsCOMArray<T> to nsTArray<nsCOMPtr<T>>. If it turns out that we can optimize the implementation, we can always do that later.
Comment on attachment 369071 [details] [diff] [review]
js part [pushed: comment 97]

http://hg.mozilla.org/mozilla-central/rev/f35e58681951
Attachment #369071 - Attachment description: js part → js part [pushed: comment 97]
As pushed: http://hg.mozilla.org/mozilla-central/rev/f35e58681951
with mObservers changed to nsCOMArray.
Attachment #369072 - Attachment is obsolete: true
Attachment #369241 - Flags: superreview+
Attachment #369241 - Flags: review+
Comment on attachment 369241 [details] [diff] [review]
parser part, v2 [pushed: comment 99]

The changeset is actually http://hg.mozilla.org/mozilla-central/rev/803af1db1238 sorry
Attachment #369241 - Attachment description: parser part, v2 [pushed: comment 98] → parser part, v2 [pushed: comment 99]
Posted patch rdf part (obsolete) — Splinter Review
Attachment #369713 - Flags: superreview?(axel)
Attachment #369713 - Flags: review?(axel)
The LL_ macros are deprecated and shouldn't be used any more, right?
Attachment #369813 - Flags: superreview?(joshmoz)
Attachment #369813 - Flags: review?(joshmoz)
Posted patch docshell part (obsolete) — Splinter Review
Attachment #369817 - Flags: superreview?(benjamin)
Attachment #369817 - Flags: review?(benjamin)
Attachment #369813 - Flags: superreview?(joshmoz) → superreview?(jst)
Attachment #369813 - Flags: review?(joshmoz) → review+
Comment on attachment 368865 [details] [diff] [review]
xpcom part, v2

>diff --git a/xpcom/base/nsTraceRefcntImpl.cpp b/xpcom/base/nsTraceRefcntImpl.cpp

>-  const PRInt32 count = entries.Count();
>+  const PRInt32 count = entries.Length();
> 
>   if (!gLogLeaksOnly || leaked) {
>     // Sort the entries alphabetically by classname.
>     PRInt32 i, j;
>     for (i = count - 1; i >= 1; --i) {
>       for (j = i - 1; j >= 0; --j) {

There's a lot of signed/unsigned mismatch here... can you make everything unsigned, count up instead of down, and sort the array in the opposite direction?

>diff --git a/xpcom/glue/nsAutoLock.cpp b/xpcom/glue/nsAutoLock.cpp

Please don't touch this code; Chris Jones is rewriting it in a separate bug.

>diff --git a/xpcom/threads/TimerThread.cpp b/xpcom/threads/TimerThread.cpp

>-    PRBool rv = timers.AppendElements(mTimers);
>+#ifdef DEBUG
>+    nsTimerImpl** rv =
>+#endif
>+      timers.AppendElements(mTimers);
>     NS_ASSERTION(rv, "Could not copy timers array, remaining timers will not be released");

Skip the DEBUG and NS_ASSERTION blocks here: it will only fail on OOM, which is not really assertion-worthy.

r=me with nits fixed, except I'd like to see the nsTraceRefcntImpl.cpp changes again.
Attachment #368865 - Flags: superreview?(benjamin)
Attachment #368865 - Flags: review?(benjamin)
Attachment #368865 - Flags: review+
Comment on attachment 369817 [details] [diff] [review]
docshell part

The docshell enumerator changes at least need review by bz: I'm not sure what invariants are changing on mCurItem.
Attachment #369817 - Flags: superreview?(benjamin)
Attachment #369817 - Flags: review?(bzbarsky)
Attachment #369817 - Flags: review?(benjamin)
Comment on attachment 369817 [details] [diff] [review]
docshell part

>+++ b/docshell/base/nsDocShellEnumerator.cpp
>+    nsIDocShellTreeItem* thisItem = mItemArray[mCurIndex];
>     rv = thisItem->QueryInterface(NS_GET_IID(nsISupports), (void **)outCurItem);
>     if (NS_FAILED(rv)) return rv;

Hey, as long as you're here how about making replacing those three lines with:

  mCurIndex++;
  return CallQueryInterface(mItemArray[mCurIndex], outCurItem);

And maybe better yet, make the whole body of this function after we ensured the array:

  if (mCurIndex >= mItemArray.Length()) {
    return NS_ERROR_FAILURE;
  }

  mCurIndex++;
  return CallQueryInterface(mItemArray[mCurIndex], outCurItem);

>-nsresult nsDocShellEnumerator::EnsureDocShellArray()
>-{
>-  if (!mItemArray)
>-  {

This is bad.  With your change, every GetNext() call will rebuild the array.  That seems undesirable.  I think you want to keep the distinction between "Ensure" and "Build"...

The rest looks fine.
Attachment #369817 - Flags: review?(bzbarsky) → review-
Now uses TArrays native sort API, missed that somehow the first time.
Attachment #368865 - Attachment is obsolete: true
Attachment #370885 - Flags: review?(benjamin)
Posted patch docshell part, v2 (obsolete) — Splinter Review
>   mCurIndex++;
>   return CallQueryInterface(mItemArray[mCurIndex], outCurItem);

pre-increment is wrong here. I made that clear with a comment.
Attachment #369817 - Attachment is obsolete: true
Attachment #370892 - Flags: review?(bzbarsky)
Posted patch docshell part, v3 (obsolete) — Splinter Review
Sorry, missed another EnsureArray.
Attachment #370892 - Attachment is obsolete: true
Attachment #370894 - Flags: review?(bzbarsky)
Attachment #370892 - Flags: review?(bzbarsky)
That still does extra work in the case when there just happen to be no docshells to enumerate for our type in our subtree.  I'd really prefer we just used a boolean flag for whether the array is "up to date".
Attachment #370885 - Flags: review?(benjamin) → review+
> I'd really prefer we just
> used a boolean flag for whether the array is "up to date".

This patch uses a bool now.
Attachment #370894 - Attachment is obsolete: true
Attachment #370966 - Flags: review?(bzbarsky)
Attachment #370894 - Flags: review?(bzbarsky)
Attachment #369813 - Flags: superreview?(jst) → superreview+
Comment on attachment 369813 [details] [diff] [review]
plugin part [pushed: comment 113]

- In nsNPAPIPluginInstance::PopPopupsEnabledState():

-  PopupControlState oldState =
-    (PopupControlState)NS_PTR_TO_INT32(mPopupStates[last]);
+  PopupControlState &oldState = mPopupStates[last];

I guess I don't see why this should be a reference here, rather than a PopupControlState. It's an int under the covers.
 
Anyways, sr=jst either way.
Attachment #370966 - Flags: review?(bzbarsky) → review+
Comment on attachment 370966 [details] [diff] [review]
docshell part, v4 [pushed: comment 115]

Looks good.
Comment on attachment 369813 [details] [diff] [review]
plugin part [pushed: comment 113]

http://hg.mozilla.org/mozilla-central/rev/ea9c7c17e0c2
Attachment #369813 - Attachment description: plugin part → plugin part [pushed: comment 113]
Comment on attachment 370885 [details] [diff] [review]
xpcom part, v3 [pushed: comment 114]

http://hg.mozilla.org/mozilla-central/rev/cd38a0fbe1eb
Attachment #370885 - Attachment description: xpcom part, v3 → xpcom part, v3 [pushed: comment 114]
Comment on attachment 370966 [details] [diff] [review]
docshell part, v4 [pushed: comment 115]

http://hg.mozilla.org/mozilla-central/rev/f2f8545e079b
Attachment #370966 - Attachment description: docshell part, v4 → docshell part, v4 [pushed: comment 115]
Posted patch editor part, v2 (obsolete) — Splinter Review
long to no progress :(
unbitrotted patch
Attachment #368858 - Attachment is obsolete: true
Attachment #375311 - Flags: superreview?(neil)
Attachment #375311 - Flags: review?(neil)
Attachment #368858 - Flags: superreview?(daniel)
Attachment #368858 - Flags: review?(daniel)
Attachment #369713 - Attachment is obsolete: true
Attachment #375312 - Flags: superreview?(benjamin)
Attachment #375312 - Flags: review?(benjamin)
Attachment #369713 - Flags: superreview?(axel)
Attachment #369713 - Flags: review?(axel)
Attachment #369073 - Flags: superreview?(vladimir)
Attachment #369073 - Flags: superreview?(sspitzer)
Attachment #369073 - Flags: review?(vladimir)
Attachment #369073 - Flags: review?(sspitzer)
Comment on attachment 375311 [details] [diff] [review]
editor part, v2

>-      res = SetCSSProperty(theElement, (nsIAtom *)cssPropertyArray.ElementAt(index),
>+      res = SetCSSProperty(theElement, cssPropertyArray.ElementAt(index),
>                            cssValueArray[index], aSuppressTransaction);
I think you should use [] where possible (cssValueArray is also an nsTArray).

>+          spanCellList.AppendElement(cell.get());
I think the get() is only here because you can't cast an nsCOMPtr to a void*, but you can cast it to an nsIDOMElement*, so you shouldn't need the .get()

>-static PRBool IndexNotTested(nsVoidArray *aArray, PRInt32 aIndex)
>+static PRBool IndexNotTested(nsTArray<PRInt32> *aArray, PRInt32 aIndex)
> {
>-  if (aArray)
>+  if (aArray && aArray->Contains(aIndex))
Consider
static PRBool IndexNotTested(nsTArray<PRInt32> &aArray, PRInt32 aIndex)
{
  return !aArray->Contains(aIndex);
}

>-          newEntry = (OffsetEntry *)mOffsetTable[i+1];
>+          newEntry = &mOffsetTable[i+1];
>           newEntry->mNodeOffset = entry->mNodeOffset;
The problem here is that newEntry was created by expanding the array, potentially memmoving the data, making entry stale. I don't know a good fix.
Attachment #375311 - Flags: superreview?(neil)
Attachment #375311 - Flags: superreview-
Attachment #375311 - Flags: review?(neil)
> >-static PRBool IndexNotTested(nsVoidArray *aArray, PRInt32 aIndex)
> >+static PRBool IndexNotTested(nsTArray<PRInt32> *aArray, PRInt32 aIndex)
> > {
> >-  if (aArray)
> >+  if (aArray && aArray->Contains(aIndex))
> Consider
> static PRBool IndexNotTested(nsTArray<PRInt32> &aArray, PRInt32 aIndex)
> {
>   return !aArray->Contains(aIndex);
> }

Or even better, remove the funktion entirely and call Contains directly.
Attachment #369073 - Flags: superreview?(vladimir)
Attachment #369073 - Flags: superreview+
Attachment #369073 - Flags: review?(vladimir)
Attachment #369073 - Flags: review+
Comment on attachment 369073 [details] [diff] [review]
browser part [pushed: comment 120]

http://hg.mozilla.org/mozilla-central/rev/d3a9f84b14d0
Attachment #369073 - Attachment description: browser part → browser part [pushed: comment 120]
Posted patch editor part, v3 (obsolete) — Splinter Review
> The problem here is that newEntry was created by expanding the array,
> potentially memmoving the data, making entry stale. I don't know a good fix.

I changed this back to having OffsetEntry allocated on heap, seems to be the safest thing to do even if it wastes some memory/allocations.

Other nits fixed.
Attachment #375311 - Attachment is obsolete: true
Attachment #375455 - Flags: review?(neil)
Comment on attachment 375455 [details] [diff] [review]
editor part, v3

>+          if ((i+1) < PRInt32(mOffsetTable.Length()))
[These casts are annoying but it's presumably infeasible to change the declaration of i?]
Attachment #375455 - Flags: review?(neil) → review+
(In reply to comment #122)
> [These casts are annoying but it's presumably infeasible to change the
> declaration of i?]

After looking at things carefully again, I just found one place where I could change i to PRUint32.
In the other cases i is used to iterate from top down. Or it would require other variables to be cast to avoid un/signed comparison warnings.

I'll land the patch when I have more time to watch the tree.
Attachment #375455 - Attachment is obsolete: true
Attachment #375570 - Flags: review+
Comment on attachment 375570 [details] [diff] [review]
editor part, v4 [pushed: comment 124]

http://hg.mozilla.org/mozilla-central/rev/42dd384c9c57
Attachment #375570 - Attachment description: editor part, v4 → editor part, v4 [pushed: comment 124]
Attachment #375312 - Flags: superreview?(benjamin)
Attachment #375312 - Flags: superreview+
Attachment #375312 - Flags: review?(benjamin)
Attachment #375312 - Flags: review+
Comment on attachment 375570 [details] [diff] [review]
editor part, v4 [pushed: comment 124]

>diff --git a/editor/libeditor/base/nsSelectionState.cpp b/editor/libeditor/base/nsSelectionState.cpp

>@@ -70,8 +69,7 @@ nsSelectionState::SaveSelection(nsISelec
>     PRInt32 count = rangeCount-arrayCount;
>     for (i=0; i<count; i++)
>     {
>-      item = new nsRangeStore;
>-      mArray.AppendElement(item);
>+      mArray.AppendElement();
>     }
>   }
>   
>@@ -80,8 +78,6 @@ nsSelectionState::SaveSelection(nsISelec
>   {
>     for (i = arrayCount-1; i >= rangeCount; i--)
>     {
>-      item = (nsRangeStore*)mArray.ElementAt(i);
>-      delete item;
>       mArray.RemoveElementAt(i);
>     }
>   }

Seems like you could replace this entire 18-line chunk (including the little bits of context omitted above) with one line:

  mArray.SetLength(rangeCount);
Comment on attachment 375312 [details] [diff] [review]
rdf part, v2 [pushed: comment 126]

http://hg.mozilla.org/mozilla-central/rev/fb3b3f21f802
Attachment #375312 - Attachment description: rdf part, v2 → rdf part, v2 [pushed: comment 126]
Posted patch remaining parts (obsolete) — Splinter Review
This patch includes all the remaining parts, plus dbarons suggestion from comment 125.
This just leaves AutoLock, COMArray, SmallVoidArray and non-code parts.
Attachment #376255 - Flags: superreview?(dbaron)
Attachment #376255 - Flags: review?(jonas)
Attachment #376255 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 376255 [details] [diff] [review]
remaining parts

In nsJavaXPTCStub, seems like you want nsTPtrArray rather than 
nsTArray?

In nsSystemPrefService, shouldn't both mObservers just be 
nsAutoTArray rather than nsAutoTArray* ?  Or is there a reason for 
the opposite?

In nsSystemPrefService, you should remove the function 
gconfDeleteObserver which you make unused.

In nsSystemPrefService, are you really ok converting those arrays to
storing objects rather than pointers?  It looks like the addresses 
of the entries in both mObservers arrays are stored elsewhere, which
makes me think you're not.  If you think otherwise, could you 
explain?

In nsDocLoader::FireOnProgressChange, maybe there was a reason for 
the SafeElementAt?  Should you re-get the length of the array each 
time through the loop?  i.e., make it
  while (!mListeneRInfoList.IsEmpty()) {
    nsListenerInfo &info = mListenerInfoList[mListenerInfoList.Length() - 1];
etc.?  Likewise for FireOnStateChange, etc.  That said, if that was
really the motivation, we'd really be much better off copying the 
listeners array before notifying.  You should probably ask bzbarsky
about this.

Shouldn't gViewManagers also be a nsTPtrArray?

Same for nsXPITriggerInfo.h?

sr=dbaron with those changes, although some of them are substantial
Posted patch remaining parts, v2 (obsolete) — Splinter Review
> In nsSystemPrefService, shouldn't both mObservers just be 
> nsAutoTArray rather than nsAutoTArray* ?  Or is there a reason for 
> the opposite?
> 
> In nsSystemPrefService, you should remove the function 
> gconfDeleteObserver which you make unused.

done.

> In nsSystemPrefService, are you really ok converting those arrays to
> storing objects rather than pointers?  It looks like the addresses 
> of the entries in both mObservers arrays are stored elsewhere, which
> makes me think you're not.  If you think otherwise, could you 
> explain?

I changed that back to heap-allocated objects.
Also using nsAutoPtr to avoid manual delete.

> In nsJavaXPTCStub, seems like you want nsTPtrArray rather than 
> nsTArray?

This also uses nsAutoPtr now.

> In nsDocLoader::FireOnProgressChange, maybe there was a reason for 
> the SafeElementAt?  Should you re-get the length of the array each 
> time through the loop?  i.e., make it
>   while (!mListeneRInfoList.IsEmpty()) {
>     nsListenerInfo &info = mListenerInfoList[mListenerInfoList.Length() - 1];
> etc.?  Likewise for FireOnStateChange, etc.  That said, if that was
> really the motivation, we'd really be much better off copying the 
> listeners array before notifying.  You should probably ask bzbarsky
> about this.

The comments above those code (whose wording I also fixed) says it should be safe here. bzbarsky should give it another look/review though.

> Shouldn't gViewManagers also be a nsTPtrArray?
> 
> Same for nsXPITriggerInfo.h?

I don't see any added benefit by using nsTPtrArray here.
Attachment #376255 - Attachment is obsolete: true
Attachment #376391 - Flags: superreview+
Attachment #376391 - Flags: review?(bzbarsky)
Attachment #376255 - Flags: review?(jonas)
> The comments above those code (whose wording I also fixed) says it should be
> safe here.

What that comment _should_ be saying is that the back-to-front iteration order guarantees that if a listener removes some listeners (possibly including itself) from the list, that won't cause us to skip notifying other listeners that wanted the notification.  That's all it guarantees, really: adding listeners while not removing means that the added listeners won't get the notifications, while adding and removing together means that the new listeners might or might not get it.  In any case, listeners can perform arbitrary mutation of the listener array, so the patch in question (which nixes the "safe" call) can lead to crashes.

Note that just making a copy of the array means breaking compat (for a frozen interface, at that, though the behavior isn't documented in the interface, of course).  Ideally we'd use an observer array here...  In fact, is there a reason to not do that?  This is what it's designed for.
Note that I didn't look at the non-docloader changes; let me know if you want me to do that.  The above applies to all the On*Change stuff in docloader.
Posted patch remaining parts, v3 (obsolete) — Splinter Review
Using nsTObserverArray with EndLimitedIterator now. This should have the same behaviour as the old code, as in not notifying any new observers.

One problem with nsTObserverArray is that it does not have a Compact() method. The old code used that a lot to make the Array take as little memory as possible. I'm not sure what impact this has.

If you don't mind, you could also review the other parts of the patch, I changed quite a lot after dbarons comments so I would feel more comfortable if those parts are looked over again.
Attachment #376391 - Attachment is obsolete: true
Attachment #376572 - Flags: superreview+
Attachment #376572 - Flags: review?(bzbarsky)
Attachment #376391 - Flags: review?(bzbarsky)
> One problem with nsTObserverArray is that it does not have a Compact() method.

Why not just add one?  The nsTObserverArray API had just enough of nsTArray-like stuff exposed to be useful; there's no reason to not add more.

It probably makes more sense to keep have whoever reviewed the patch before review the updates too, since they've already looked at it...  I'm happy to review the updated docloader changes and whatever else hasn't been reviewed by someone else or particularly needs my review.
This adds a Compact() method to ObserverArray.

dbaron: do you mind looking over it again? However, as I said in comment 129 I don't see any benefit in using TPtrArray instead of TArray in some cases you mentioned.
Attachment #376572 - Attachment is obsolete: true
Attachment #376614 - Flags: superreview?(dbaron)
Attachment #376614 - Flags: review?(bzbarsky)
Attachment #376572 - Flags: review?(bzbarsky)
(In reply to comment #134)
> dbaron: do you mind looking over it again? However, as I said in comment 129 I
> don't see any benefit in using TPtrArray instead of TArray in some cases you
> mentioned.

What's the benefit of using nsTArray rather than nsTPtrArray?
Consistency? What's the benefit of using nsTPtrArray?
Comment on attachment 376614 [details] [diff] [review]
remaining parts, v4 [pushed: comment 141]

I thought nsTPtrArray had some features that were better for arrays of pointers.  If that's not the case, then maybe we should get rid of it?

+  nsTArray<nsAutoPtr<nsJavaXPTCStub> > mChildren; // weak references (cleared by the children)

You should fix that comment.

>+NS_SPECIALIZE_TEMPLATE
>+class nsDefaultComparator <class nsDocLoader::nsListenerInfo, class nsDocLoader::nsListenerInfo> {
>+  public:
>+    PRBool Equals(const nsDocLoader::nsListenerInfo& aA,
>+                  const nsDocLoader::nsListenerInfo& aB) const {
>+      return aA.mWeakListener == aB.mWeakListener
>+             && aA.mNotifyMask == aB.mNotifyMask;
>+    }
> };

What's this for?  It doesn't look like it's any different from the default operator== behavior.
Attachment #376614 - Flags: superreview?(dbaron) → superreview+
nsTPtrArray provides SafeElementAt(index), that's all it does. I do think we should keep the class since I want to encourage people to use SafeElementAt unless there is a good reason not to (generally performance and/or documentation).

If you don't need SafeElementAt(index) it doesn't really matter if you use nsTPtrArray or nsTArray.

We could use some trickery and template specialization to make SafeElementAt(index) appear on nsTArray though. But that's fodder for a separate bug IMHO (this one is huge as it is).
Filed bug 492587 on adding SafeElementAt to nsTArray.
Attachment #376614 - Flags: review?(bzbarsky) → review+
Comment on attachment 376614 [details] [diff] [review]
remaining parts, v4 [pushed: comment 141]

Please add the lines:

  unified = 8
  showfunc = true  

to the [diff] section of your ~/.hgrc.  It'll make your patches a _lot_ easier to review.....

This diff seems to be missing nsCommandGroup.cpp, no?  Or did you change the comment in the header but not the actual code?  If so, let's not do that.  ;)

>+++ b/uriloader/base/nsDocLoader.cpp
>+NS_SPECIALIZE_TEMPLATE
>+class nsDefaultComparator <class nsDocLoader::nsListenerInfo, class nsDocLoader::nsListenerInfo> {

I too do not understand why you need this.

It might be nice to have a macro that takes a flag-set and some code and then basically does the whole "iterate the array, skip infos that don't have these flags, call this code on the others" thing.  That should be a followup bug, though.


> nsDocLoader::GetListenerInfo(nsIWebProgressListener *aListener)
>+    info = &mListenerInfoList.ElementAt(i);

I really dislike depending on that implementation detail of nsTArray.  Please file a followup bug to add ElementAt() signatures that return elem_type* and const elem_type*?  Or just use |mListenerInfoList.Elements() + i| here?

>+++ b/uriloader/base/nsDocLoader.h
>@@ -216,11 +222,11 @@ protected:
>-    nsVoidArray                mListenerInfoList;
>+    nsTObserverArray<nsListenerInfo> mListenerInfoList;

The old behavior was to init the size to 8 in the ctor.  Should this be an nsAutoTObserverArray<nsListenerInfo, 8>?

>+    // Every nsListenerInfo 
>+    PRUint32 GetListenerIndex(nsListenerInfo &aInfo) {
>+    
>+    }

This seems like leftovers from... something.  Just get rid of it?

r=bzbarsky on the docloader changes, with the above nits picked and followup bugs filed as needed.
Blocks: 493701
Comment on attachment 376614 [details] [diff] [review]
remaining parts, v4 [pushed: comment 141]

> This diff seems to be missing nsCommandGroup.cpp, no?  Or did you change the
> comment in the header but not the actual code?  If so, let's not do that.  ;)

I already fixed that in another patch, just missed the comment.

> I really dislike depending on that implementation detail of nsTArray.  Please
> file a followup bug to add ElementAt() signatures that return elem_type* and
> const elem_type*?  Or just use |mListenerInfoList.Elements() + i| here?

> >+++ b/uriloader/base/nsDocLoader.cpp
> >+NS_SPECIALIZE_TEMPLATE
> >+class nsDefaultComparator <class nsDocLoader::nsListenerInfo, class nsDocLoader::nsListenerInfo> {
> 
> I too do not understand why you need this.

Ok, I added a Elements() function to ObserverArray and used that. I also replaced RemoveElement (which needed DefaultComparator) with RemoveElementAt(&elem-array.Elements())
I hope thats ok.

> The old behavior was to init the size to 8 in the ctor.  Should this be an
> nsAutoTObserverArray<nsListenerInfo, 8>?

That seems to work well with the Compact() calls, making sure it always uses the internal buffer if the elements fit in it.

Pushed as http://hg.mozilla.org/mozilla-central/rev/461d728271d1
Attachment #376614 - Attachment description: remaining parts, v4 → remaining parts, v4 [pushed: comment 141]
Depends on: 493708
Depends on: 493711
I filed followups on nsSmallVoidArray and nsCOMArray. nsAutoLock should die soon anyway. There should only be non-code parts left that I just left alone.

Thank you all for helping and reviewing :)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Comment on attachment 376614 [details] [diff] [review]
remaining parts, v4 [pushed: comment 141]

>+    // Every nsListenerInfo 
>+    PRUint32 GetListenerIndex(nsListenerInfo &aInfo) {
>+    
>+    }
This doesn't look quite right...
(In reply to comment #143)
> (From update of attachment 376614 [details] [diff] [review])
> >+    // Every nsListenerInfo 
> >+    PRUint32 GetListenerIndex(nsListenerInfo &aInfo) {
> >+    
> >+    }
> This doesn't look quite right...

I have removed that from the final patch.
This is the final patch as pushed to m-c, with all nits fixed.
Attachment #376614 - Attachment is obsolete: true
Attachment #378294 - Flags: superreview+
Attachment #378294 - Flags: review+
I've backed this out due to the browser chrome test failures that it appeared to cause.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I suspect the usage of EndLimitedIterator has caused the tests to fail. I have pushed a patch using ForwardIterator to the tryserver and will attach it once it passes the tests.
> Ok, I added a Elements() function to ObserverArray and used that

Oh, this is not an nsTArray....

I'm not so happy adding Elements() to nsTObserverArray.  Why exactly does RemoveElement need the comparator thing?  Would it make sense to move away from passing around a pointer there instead of looking for hacky ways to get at the pointer?

It's probably worth having a typedef for nsAutoTArray<ListenerInfo,8> so you don't have to keep repeating it in the code.

Just using ForwardIterator would be a significant behavior change.  I don't think we want to do that, but if you can explain to me why that won't break things I might be willing to change my mind....
Posted patch remaining parts, v5 (obsolete) — Splinter Review
As discussed on IRC, the test failures were due to changing the iteration order. I added a nsTObserverArray::BackwardIterator and that seems to have fixed browser-chrome for me. I have also pushed the patch to tryserver.
I also added a nsDocLoader::RemoveEmptyListeners() method. RemoveElement() is forward iterating and may leave empty listeners at the end of the array.
Attachment #378294 - Attachment is obsolete: true
Attachment #378413 - Flags: superreview+
Attachment #378413 - Flags: review?(bzbarsky)
Yes, I really don't want to add .Elements() to nsTObserverArray. The whole purpose of that class is that it's supposed to deal with mutations safely and once you start handing out references to the elements array people are definitely going to iterate it unsafely.

Why do you need .Elements()?

Similarly, making RemoveElement not work correctly when there are ReverseIterators seems to violate the purpose of the class since we no longer gracefully deal with mutations at all points. It'd very likely also violate the assumptions people make about the class.

Is there a reason you're using a nsTObserverArray? Couldn't you just use a simple nsTArray since apparently you don't need to deal with mutations?

Alternatively, you can implement a ReverseIterator properly. We actually used to have that. See:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/Attic/nsTObserverArray.h&rev=3.1&root=/cvsroot
I thought we'd decided to not do the Elements() thing, on irc.

And yes, RemoveElement needs to work, period.  I did mention on IRC that a good starting point is copying out old impl.

> Is there a reason you're using a nsTObserverArray?

Yes.  Notifying these objects will run arbitrary script and sometimes remove or add them; the array needs to deal.
OK, I was looking at this again, but I'd _really_ appreciate an interdiff here; it'd help a good bit with sorting out which parts I really want to review.

One comment so far: BackwardIterator as implemented doesn't correctly traverse an element prepended to the array while notifying the first element of the array...

Also, since the Elements() you added is not used and Jonas and I are not happy with it, please take it out?
> One comment so far: BackwardIterator as implemented doesn't correctly traverse
> an element prepended to the array while notifying the first element of the
> array...

As I don't see an easy way to fix that without also modifying AdjustIterators or the other Iterators, I added a comment to make that behaviour clear.
Prepending was broken anyway, I also fixed that now.
Attachment #378413 - Attachment is obsolete: true
Attachment #381910 - Flags: superreview+
Attachment #381910 - Flags: review?(bzbarsky)
Attachment #378413 - Flags: review?(bzbarsky)
Comment on attachment 381910 [details] [diff] [review]
remaining parts, v6, full patch [pushed: comment 156]

r=me based on that interdiff.  I find it very sad that nsTObserverArray got broken like that.  Thanks for fixing it!  For the record, it was a regression from bug 407442.
Attachment #381910 - Flags: review?(bzbarsky) → review+
Arpad, I added a unit test for nsTObserverArray; it's xpcom/tests/TestObserverArray.cpp.  The test that fails without your patch is commented out near the bottom; uncomment it as part of your landing?  And maybe add some tests for the new iterator type you're adding?
Comment on attachment 381910 [details] [diff] [review]
remaining parts, v6, full patch [pushed: comment 156]

Pushed as http://hg.mozilla.org/mozilla-central/rev/691dd3dcfbca
Tests proposed in comment 155 were added as part of bug 493701.
Attachment #381910 - Attachment description: remaining parts, v6, full patch → remaining parts, v6, full patch [pushed: comment 156]
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Regression: DHTML increase 0.98% on XP Firefox:
http://graphs-new.mozilla.org/graph.html#tests=[{%22machine%22:55,%22test%22:18,%22branch%22:1},{%22machine%22:56,%22test%22:18,%22branch%22:1},{%22machine%22:57,%22test%22:18,%22branch%22:1},{%22machine%22:71,%22test%22:18,%22branch%22:1},{%22machine%22:108,%22test%22:18,%22branch%22:1},{%22machine%22:109,%22test%22:18,%22branch%22:1},{%22machine%22:110,%22test%22:18,%22branch%22:1},{%22machine%22:111,%22test%22:18,%22branch%22:1}]&sel=1244728380,1244901180

Previous results: 1083.47 from build 20090612062022 of revision 04ed36482fa9 at 2009-06-12 06:20:00 on qm-pxp-fast03 run # 0

New results: 1094.12 from build 20090612065328 of revision 2f2586790f32 at 2009-06-12 06:53:00 on qm-pxp-trunk01 run # 0

(this and bug 493701 fall into the suspected regression range)
Also a 3.3% Tp3 hit, at least on one of the Windows XP boxes.
I backed out the patch. I will probably split it up into smaller chunks to see which one is causing the performance regressions. I really don't know why it only hit the windows boxes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
There was a new warning from one of these patches: http://office.smedbergs.us:8080/warning?signature=hg%3Auriloader%2Fbase%2FnsDocLoader.cpp%3Ab8e531a6c961%3A122%3Adeclaration+%27struct+nsDocLoade


uriloader/base/nsDocLoader.cpp:122: declaration 'struct nsDocLoader::nsListenerInfo' does not declare anything

I think the extra class keyword there is unnecessary and should be removed.
Long time no progress, sorry about that.
I split out the docloader changes into bug 493701, forwarding sr=dbaron for the remaining parts, as you have reviewed those parts and the docloader changes bz has reviewed are now in another bug.

So one of these patches is causing a ~1% DHTML regression on windows. Details show that the "scrolling" test is suffering a slowdown while other tests stay the same or even improve a little.
I submitted the patches separately to tryserver, unfortunately the dhtml tests are not run there. I've tried reproducing the slowdown on vista with those builds but I wasn't able to do so. So I'm kind of stuck there. :-(
Attachment #381910 - Attachment is obsolete: true
Attachment #381911 - Attachment is obsolete: true
Attachment #385615 - Flags: superreview+
Hmm.  We also saw a regression on "scrolling" that we can't reproduce locally from a frame construction change....

You might want to just land this patch in pieces until you find the specific part (if any) that causes this regression.
Depends on: 503784
Depends on: 520621
Depends on: 522779
Depends on: 563616
Depends on: 485693
Not sure if you do reviews for treewide cleanup?
Attachment #8406841 - Flags: review?(Ms2ger)
Attachment #8406841 - Flags: review?(ehsan)
Attachment #8406841 - Flags: review?(Ms2ger)
Attachment #8406841 - Flags: feedback+
Attachment #8406841 - Flags: review?(ehsan) → review+
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/17bc5713639c

I will file followups for the remaining uses.
Depends on: 1004933
Depends on: 1007604
The only things still using VoidArray are in network/cache, which is dead code and will go away eventually and the things depending on nsSmallVoidArray, which is tracked in Bug 493708.

So lets finally close this :-)
Status: REOPENED → RESOLVED
Closed: 10 years ago5 years ago
Resolution: --- → FIXED
End of an Era..... :-(
Of course, nsTArray/etc is definitely more modern (and more C++-ey). :-)
This warms my heart! Thanks Arpad!
Sigh, finally.   Sniff.
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.