474369 - get rid of nsVoidArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Everyone using nsVoidArray should use nsTArray<T*> instead.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or possibly nsTPtrArray<T> if they need SafeElementAt.

Comment 2

[Arpad Borsos [:Swatinem]]

Taking this bug, I have a patch in the works.

Comment 3

[Arpad Borsos [:Swatinem]]

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?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 5

[Arpad Borsos [:Swatinem]]

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

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 8

[Arpad Borsos [:Swatinem]]

Yes, I am on x86_64.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Karl is an x86_64 wizard who might be able to help.

Comment 10

[Arpad Borsos [:Swatinem]]

Attachment: get rid of nsVoidArray, layout part without tables

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?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 12

[Arpad Borsos [:Swatinem]]

Attachment: get rid of nsVoidArray, layout part without tables, v2

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

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 14

[Arpad Borsos [:Swatinem]]

Attachment: get rid of nsVoidArray, layout part without tables, v3

Using array[i] = nsnull; now.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 360066 [details] [diff] [review]
get rid of nsVoidArray, layout part without tables, v3

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

< 0

Comment 16

[Arpad Borsos [:Swatinem]]

Attachment: layout part, for checkin [Checkin: Comment 17]

hg exported patch with last nit fixed.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 360067 [details] [diff] [review]
layout part, for checkin
[Checkin: Comment 17]


http://hg.mozilla.org/mozilla-central/rev/a206aff7a9c6

Comment 18

[Arpad Borsos [:Swatinem]]

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.

Comment 19

[Karl Tomlinson (:karlt)]

(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?

Comment 20

[Arpad Borsos [:Swatinem]]

Attachment: get rid of nsVoidArray, layout/tables part

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

Comment 21

[[e-mail]]

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

Comment 22

[[e-mail]]

Attachment: Fix DOM Inspector [Checkin: Comment 26]

ReplaceElementsAt(n, 1, e); is horribly inefficient anyway, but the second call was wrong anyway as per my previous comment.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 360500 [details] [diff] [review]
Fix DOM Inspector
[Checkin: Comment 26]

oops, sorry

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 25

[Arpad Borsos [:Swatinem]]

Attachment: layout/tables part [Checkin: Comment 27]

hg exported patch with last nits fixed.

Comment 26

[[e-mail]]

Comment on attachment 360500 [details] [diff] [review]
Fix DOM Inspector
[Checkin: Comment 26]

Pushed changeset 8b72dc73cfa1 to mozilla-central.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed layout/tables part: http://hg.mozilla.org/mozilla-central/rev/9889b0189cd4

Comment 28

[Arpad Borsos [:Swatinem]]

Attachment: netwerk part

Comment 29

[Arpad Borsos [:Swatinem]]

Attachment: xpfe part

Comment 30

[Arpad Borsos [:Swatinem]]

Attachment: widget part [Checkin: Comment 41]

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.

Comment 31

[[e-mail]]

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?

Comment 32

[Arpad Borsos [:Swatinem]]

Attachment: xpfe part, v2

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.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe you could hop on IRC and see if anyone's around to help you with Windows.

Comment 35

[Arpad Borsos [:Swatinem]]

Attachment: gfx part [Checkin: Comment 42]

Comment 36

[Arpad Borsos [:Swatinem]]

Attachment: embedding part [Checkin: Comment 47]

Comment 37

[Arpad Borsos [:Swatinem]]

Attachment: toolkit part [Checkin: See comment 48]

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 39

[Arpad Borsos [:Swatinem]]

Attachment: netwerk part, v2 [Checkin: Comment 43]

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

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 41

[Serge Gautherie (:sgautherie)]

Comment on attachment 362578 [details] [diff] [review]
widget part
[Checkin: Comment 41]


http://hg.mozilla.org/mozilla-central/rev/a5b915087467

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 362713 [details] [diff] [review]
gfx part
[Checkin: Comment 42]


http://hg.mozilla.org/mozilla-central/rev/7366df357e91

Comment 43

[Serge Gautherie (:sgautherie)]

Comment on attachment 363287 [details] [diff] [review]
netwerk part, v2
[Checkin: Comment 43]


http://hg.mozilla.org/mozilla-central/rev/187775ec9ac5

Comment 44

[Arpad Borsos [:Swatinem]]

Attachment: content part 1

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.

Comment 45

[Arpad Borsos [:Swatinem]]

Attachment: content part 2

This is the working part of base and xul. Runs reftest and content mochitests fine.

Comment 46

[Arpad Borsos [:Swatinem]]

Attachment: content crasher

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 47

[Serge Gautherie (:sgautherie)]

Comment on attachment 362714 [details] [diff] [review]
embedding part
[Checkin: Comment 47]


http://hg.mozilla.org/mozilla-central/rev/24c37ac3bc3f

Comment 48

[Serge Gautherie (:sgautherie)]

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.

Comment 49

[Serge Gautherie (:sgautherie)]

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.

Comment 50

[Ben Combee (:bcombee)]

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

Comment 51

[Arpad Borsos [:Swatinem]]

Attachment: activex fixup [Checkin: Comment 55]

This fixes the problem reported in comment #50, sorry for that.

Comment 52

[Jonathan Watt [:jwatt]]

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.

Comment 53

[Arpad Borsos [:Swatinem]]

Lets get the ActiveX fixup in.

Comment 54

[[e-mail]]

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 55

[Serge Gautherie (:sgautherie)]

Comment on attachment 364275 [details] [diff] [review]
activex fixup
[Checkin: Comment 55]


http://hg.mozilla.org/mozilla-central/rev/21cd341f555c

Comment 56

[Arpad Borsos [:Swatinem]]

Attachment: xpfe part, for checkin [Checkin: Comment 57]

Updated after the appleevents remove, forwarding neils r+.

Comment 57

[Serge Gautherie (:sgautherie)]

Comment on attachment 364935 [details] [diff] [review]
xpfe part, for checkin
[Checkin: Comment 57]


http://hg.mozilla.org/mozilla-central/rev/6874f88f06a7

Comment 58

[Arpad Borsos [:Swatinem]]

Attachment: content part, v2

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

Comment 59

[Johnny Stenback (:jst)]

Comment on attachment 366181 [details] [diff] [review]
content part, v2

Sicking will deal with this one...

Comment 60

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 61

[Arpad Borsos [:Swatinem]]

Attachment: xpcom part

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.

Comment 62

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 63

[Arpad Borsos [:Swatinem]]

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

Comment 64

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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()

Comment 65

[Arpad Borsos [:Swatinem]]

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.

Comment 66

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 67

[Arpad Borsos [:Swatinem]]

Attachment: content part, v3

This patch has a lot more out-of-bounds checks. However, it still uses nsTArray as replacement for nsSmallVoidArray.

Comment 68

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 69

[Arpad Borsos [:Swatinem]]

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

Comment 70

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could you attach an interdiff? If not I'd have to start the review over from scratch. That is certainly doable if needed though...

Comment 71

[Arpad Borsos [:Swatinem]]

Attachment: interdiff

Comment 72

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 73

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 74

[Arpad Borsos [:Swatinem]]

Attachment: content part, v4 [pushed: comment 76]

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.

Comment 75

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 76

[Arpad Borsos [:Swatinem]]

Comment on attachment 368495 [details] [diff] [review]
content part, v4 [pushed: comment 76]

http://hg.mozilla.org/mozilla-central/rev/659355060532

Comment 77

[Peter Van der Beken [:peterv]]

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;
>+        }

Comment 78

[Arpad Borsos [:Swatinem]]

Attachment: content followup [pushed: comment 79]

As per comment 77.

Comment 79

[Arpad Borsos [:Swatinem]]

Comment on attachment 368501 [details] [diff] [review]
content followup [pushed: comment 79]

http://hg.mozilla.org/mozilla-central/rev/591d956f2f0a

Comment 80

[Arpad Borsos [:Swatinem]]

Attachment: bustage fix as checked in [pushed: comment 80]

As checked in: http://hg.mozilla.org/mozilla-central/rev/e29ec3dc42ee
Using an uninitialized iterator variable is not a good idea. :(

Comment 81

[Ryan VanderMeulen [:RyanVM]]

...\content\svg\content\src\nssvgnumberlist.cpp(388) : warning C4700: uninitialized local variable 'rv' used

Comment 82

[Arpad Borsos [:Swatinem]]

Attachment: uninitialized variable fix

Fix for comment 81.

Comment 83

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 84

[Arpad Borsos [:Swatinem]]

Attachment: uninitialized variable fix,v2 [pushed: comment 93]

Comment 85

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 86

[Arpad Borsos [:Swatinem]]

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?

Comment 87

[(dormant account)]

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

Comment 88

[Arpad Borsos [:Swatinem]]

Attachment: editor part

Comment 89

[Arpad Borsos [:Swatinem]]

Attachment: xpcom part, v2

I hat more problems with nsCOMArray than originally thought, so I left it alone.

Comment 90

[Arpad Borsos [:Swatinem]]

Attachment: js part [pushed: comment 97]

Comment 91

[Arpad Borsos [:Swatinem]]

Attachment: parser part

Comment 92

[Arpad Borsos [:Swatinem]]

Attachment: browser part [pushed: comment 120]

Comment 93

[Arpad Borsos [:Swatinem]]

Comment on attachment 368665 [details] [diff] [review]
uninitialized variable fix,v2 [pushed: comment 93]

http://hg.mozilla.org/mozilla-central/rev/d14de45019a0

Comment 94

[Brendan Eich [:brendan]]

Comment on attachment 369071 [details] [diff] [review]
js part [pushed: comment 97]

XPConnect != JS Engine.

/be

Comment 95

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 96

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 97

[Arpad Borsos [:Swatinem]]

Comment on attachment 369071 [details] [diff] [review]
js part [pushed: comment 97]

http://hg.mozilla.org/mozilla-central/rev/f35e58681951

Comment 98

[Arpad Borsos [:Swatinem]]

Attachment: parser part, v2 [pushed: comment 99]

As pushed: http://hg.mozilla.org/mozilla-central/rev/f35e58681951
with mObservers changed to nsCOMArray.

Comment 99

[Arpad Borsos [:Swatinem]]

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

Comment 100

[Arpad Borsos [:Swatinem]]

Attachment: rdf part

Comment 101

[Arpad Borsos [:Swatinem]]

Attachment: plugin part [pushed: comment 113]

The LL_ macros are deprecated and shouldn't be used any more, right?

Comment 102

[Arpad Borsos [:Swatinem]]

Attachment: docshell part

Comment 103

[Benjamin Smedberg]

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.

Comment 104

[Benjamin Smedberg]

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.

Comment 105

[Boris Zbarsky [:bzbarsky]]

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.

Comment 106

[Arpad Borsos [:Swatinem]]

Attachment: xpcom part, v3 [pushed: comment 114]

Now uses TArrays native sort API, missed that somehow the first time.

Comment 107

[Arpad Borsos [:Swatinem]]

Attachment: docshell part, v2

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

pre-increment is wrong here. I made that clear with a comment.

Comment 108

[Arpad Borsos [:Swatinem]]

Attachment: docshell part, v3

Sorry, missed another EnsureArray.

Comment 109

[Boris Zbarsky [: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".

Comment 110

[Arpad Borsos [:Swatinem]]

Attachment: docshell part, v4 [pushed: comment 115]

> I'd really prefer we just
> used a boolean flag for whether the array is "up to date".

This patch uses a bool now.

Comment 111

[Johnny Stenback (:jst)]

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.

Comment 112

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 370966 [details] [diff] [review]
docshell part, v4 [pushed: comment 115]

Looks good.

Comment 113

[Arpad Borsos [:Swatinem]]

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

http://hg.mozilla.org/mozilla-central/rev/ea9c7c17e0c2

Comment 114

[Arpad Borsos [:Swatinem]]

Comment on attachment 370885 [details] [diff] [review]
xpcom part, v3 [pushed: comment 114]

http://hg.mozilla.org/mozilla-central/rev/cd38a0fbe1eb

Comment 115

[Arpad Borsos [:Swatinem]]

Comment on attachment 370966 [details] [diff] [review]
docshell part, v4 [pushed: comment 115]

http://hg.mozilla.org/mozilla-central/rev/f2f8545e079b

Comment 116

[Arpad Borsos [:Swatinem]]

Attachment: editor part, v2

long to no progress :(
unbitrotted patch

Comment 117

[Arpad Borsos [:Swatinem]]

Attachment: rdf part, v2 [pushed: comment 126]

Comment 118

[[e-mail]]

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.

Comment 119

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 120

[Arpad Borsos [:Swatinem]]

Comment on attachment 369073 [details] [diff] [review]
browser part [pushed: comment 120]

http://hg.mozilla.org/mozilla-central/rev/d3a9f84b14d0

Comment 121

[Arpad Borsos [:Swatinem]]

Attachment: editor part, v3

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

Comment 122

[[e-mail]]

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?]

Comment 123

[Arpad Borsos [:Swatinem]]

Attachment: editor part, v4 [pushed: comment 124]

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

Comment 124

[Arpad Borsos [:Swatinem]]

Comment on attachment 375570 [details] [diff] [review]
editor part, v4 [pushed: comment 124]

http://hg.mozilla.org/mozilla-central/rev/42dd384c9c57

Comment 125

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 126

[Arpad Borsos [:Swatinem]]

Comment on attachment 375312 [details] [diff] [review]
rdf part, v2 [pushed: comment 126]

http://hg.mozilla.org/mozilla-central/rev/fb3b3f21f802

Comment 127

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts

This patch includes all the remaining parts, plus dbarons suggestion from comment 125.
This just leaves AutoLock, COMArray, SmallVoidArray and non-code parts.

Comment 128

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 129

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts, v2

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

Comment 130

[Boris Zbarsky [:bzbarsky]]

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

Comment 131

[Boris Zbarsky [:bzbarsky]]

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.

Comment 132

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts, v3

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.

Comment 133

[Boris Zbarsky [: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.

Comment 134

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts, v4 [pushed: comment 141]

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.

Comment 135

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 136

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Consistency? What's the benefit of using nsTPtrArray?

Comment 137

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 138

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 139

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 492587 on adding SafeElementAt to nsTArray.

Comment 140

[Boris Zbarsky [:bzbarsky]]

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.

Comment 141

[Arpad Borsos [:Swatinem]]

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

Comment 142

[Arpad Borsos [:Swatinem]]

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 :)

Comment 143

[[e-mail]]

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

Comment 144

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts, patch as checked in [pushed: comment 141]

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

Comment 145

[Dave Townsend [:mossop]]

I've backed this out due to the browser chrome test failures that it appeared to cause.

Comment 146

[Arpad Borsos [:Swatinem]]

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.

Comment 147

[Boris Zbarsky [:bzbarsky]]

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

Comment 148

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts, v5

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.

Comment 149

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 150

[Boris Zbarsky [:bzbarsky]]

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.

Comment 151

[Boris Zbarsky [:bzbarsky]]

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?

Comment 152

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts, v6, full patch [pushed: comment 156]

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

Comment 153

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts, v6, interdiff for easier review

Comment 154

[Boris Zbarsky [: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.

Comment 155

[Boris Zbarsky [:bzbarsky]]

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 156

[Arpad Borsos [:Swatinem]]

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.

Comment 157

[Mike Beltzner [:beltzner, not reading bugmail]]

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)

Comment 158

[Boris Zbarsky [:bzbarsky]]

Also a 3.3% Tp3 hit, at least on one of the Windows XP boxes.

Comment 159

[Arpad Borsos [:Swatinem]]

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.

Comment 160

[Benjamin Smedberg]

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.

Comment 161

[Arpad Borsos [:Swatinem]]

Attachment: remaining parts, v7

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

Comment 162

[Boris Zbarsky [:bzbarsky]]

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.

Comment 163

[Arpad Borsos [:Swatinem]]

Attachment: remove unused nsVoidArray includes

Not sure if you do reviews for treewide cleanup?

Comment 164

[Arpad Borsos [:Swatinem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17bc5713639c

I will file followups for the remaining uses.

Comment 165

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17bc5713639c

Comment 166

[Arpad Borsos [:Swatinem]]

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 :-)

Comment 167

[Randell Jesup [:jesup] (needinfo me)]

End of an Era..... :-(
Of course, nsTArray/etc is definitely more modern (and more C++-ey). :-)

Comment 168

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This warms my heart! Thanks Arpad!

Comment 169

[Randell Jesup [:jesup] (needinfo me)]

Sigh, finally.   Sniff.

Comment 170

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.