1308317 - Clean up nsSupportsArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

+++ This bug was initially created as a clone of Bug #1308036 +++

As follow up to bug 1308036 I'd like to clean up nsISupportsArray. It's usage is deprecated so trimming out unused parts and possibly updating gecko code that uses it to something else would be a good idea.

We can also probably replace all the array management logic with an nsAutoTArray or something similar.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Remove debug code

MozReview-Commit-ID: 7TQG7flcJkj

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Remove nsSupportsArray::MoveElement

|MoveElement| is not scriptable and unused in our codebase.

MozReview-Commit-ID: CBe8WZHG1JG

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 3: Remove nsSupportsArray::SizeTo

|SizeTo| is not scriptable and unused in our codebase.

MozReview-Commit-ID: 1DrTm46qbar

Comment 4

[Eric Rahm [:erahm]]

Attachment: Part 4: Remove nsSupportsArray::Equals

|Equals| is not scriptable and unused in our codebase.

MozReview-Commit-ID: BsbJIuR9fSk

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 5: Remove nsSupportsArray::DeleteLastElement

|DeleteLastElement| is scriptable, but a search of our add-on repo turned up
no hits and there were no references in gecko code. This also allows us to
remove the non-scriptable |RemoveLastElement| which was only called by
|DeleteLastElement|.

MozReview-Commit-ID: 20FXBrosacA

Comment 6

[Eric Rahm [:erahm]]

Attachment: Part 6: Remove nsSupportsArray::GetIndexOfStartingAt

This removes the scriptable method |GetIndexOfStartingAt| which is unused in
our codebase and turns up no references in the plugins repo. This allows to
remove the non-scriptable |IndexOfStartingAt| which is folded into |IndexOf|.

MozReview-Commit-ID: 2ADz5mLIvMU

Comment 7

[Eric Rahm [:erahm]]

Attachment: Part 7: Remove nsSupportsArray::RemoveElementsAt

MozReview-Commit-ID: H3A3gxckw5o

Comment 8

[Eric Rahm [:erahm]]

Attachment: Part 8: Just use an nsTAutoArray

This switches over from using a half-baked auto array to nsTAutoArray.

MozReview-Commit-ID: 6FR2SjOhoZR

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8798691 [details] [diff] [review]
Part 6: Remove nsSupportsArray::GetIndexOfStartingAt

Review of attachment 8798691 [details] [diff] [review]:
-----------------------------------------------------------------

IndexOf should really always take a start index, but if it hasn't been useful thus far with nsISupportsArray, it probably won't be useful in the future.  Plus it'd be a real pain to provide in this case.

Comment 10

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=306d9b7d6bd9

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8798693 [details] [diff] [review]
Part 8: Just use an nsTAutoArray

Review of attachment 8798693 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.  Apologies for the delay.  r=me

::: xpcom/ds/nsSupportsArray.cpp
@@ +50,5 @@
>  NS_IMPL_ISUPPORTS(nsSupportsArray, nsISupportsArray, nsICollection,
>                    nsISerializable)
>  
>  NS_IMETHODIMP
>  nsSupportsArray::Read(nsIObjectInputStream* aStream)

Oh, we probably use this in some terrible RDF code or something, don't we?  Ugh.

@@ -132,5 @@
> -      newArraySize = mArraySize;
> -    } else {
> -      nsISupports** array = new nsISupports*[newArraySize];
> -      if (mArray != mAutoArray) {
> -        delete[] mArray;

Uh, we (used to) just discard all the refs that mArray held?  Waaat.

@@ +75,3 @@
>    }
>  
> +  // Clear out any existing refs.

While I grant that this data structure should just go away, WDYT about making this a little more robust:

  nsTArray<nsISupports*> temp;
  if (!temp.AppendElements(newArraySize)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }
  temp.SetLength(count);

  // Read elements into temp

  // Then clear existing refs once we've read everything

  mArray.SwapElements(temp);
  return NS_OK;

Comment 12

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d58f918c21f

Comment 13

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea82808b9abd16a3170b21b9785d0fbef04a863b
Bug 1308317 - Part 1: Remove debug code. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/63208c26bfc31dc3d448afc579183cd8d67dcb89
Bug 1308317 - Part 2: Remove nsSupportsArray::MoveElement. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8a0f01f28cc41a378b8585dae2e191da2b89d2
Bug 1308317 - Part 3: Remove nsSupportsArray::SizeTo. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb30697071b22bf2218f9062372f71afe80efc35
Bug 1308317 - Part 4: Remove nsSupportsArray::Equals. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/476a831ca87ed47159795eaa38a96dd45fcb1faf
Bug 1308317 - Part 5: Remove nsSupportsArray::DeleteLastElement. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2984643f74f33137675b00b95398a9116ea175
Bug 1308317 - Part 6: Remove nsSupportsArray::GetIndexOfStartingAt. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/928ad5e9337239c5c8effc00ac42fb1140351a71
Bug 1308317 - Part 7: Remove nsSupportsArray::RemoveElementsAt. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/96d1b7238832ebf3e7e03053db19a8e557f6ca42
Bug 1308317 - Part 8: Just use an nsTAutoArray. r=froydnj

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea82808b9abd
https://hg.mozilla.org/mozilla-central/rev/63208c26bfc3
https://hg.mozilla.org/mozilla-central/rev/8d8a0f01f28c
https://hg.mozilla.org/mozilla-central/rev/bb30697071b2
https://hg.mozilla.org/mozilla-central/rev/476a831ca87e
https://hg.mozilla.org/mozilla-central/rev/8c2984643f74
https://hg.mozilla.org/mozilla-central/rev/928ad5e93372
https://hg.mozilla.org/mozilla-central/rev/96d1b7238832

Comment 15

[Carsten Book [:Tomcat]]

backed this out since this might have caused pgo crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=5271518&repo=mozilla-central

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/9ab31bcd2d40
Backed out changeset 96d1b7238832 
https://hg.mozilla.org/mozilla-central/rev/080a3dc2c637
Backed out changeset 928ad5e93372 
https://hg.mozilla.org/mozilla-central/rev/fbbbdbd73b21
Backed out changeset 8c2984643f74 
https://hg.mozilla.org/mozilla-central/rev/f3c15a19c65a
Backed out changeset 476a831ca87e 
https://hg.mozilla.org/mozilla-central/rev/e30ffe1e3c1e
Backed out changeset bb30697071b2 
https://hg.mozilla.org/mozilla-central/rev/df460f90b0cf
Backed out changeset 8d8a0f01f28c 
https://hg.mozilla.org/mozilla-central/rev/aa6c80d37fce
Backed out changeset 63208c26bfc3 
https://hg.mozilla.org/mozilla-central/rev/6343523ec56d
Backed out changeset ea82808b9abd

Comment 17

[Eric Rahm [:erahm]]

Looks like just Windows in this case? I'm not sure how this change affects nsCOMArray at all...

Comment 18

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6746de4cec1d

Comment 19

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce990cc2da03

Comment 20

[Eric Rahm [:erahm]]

So this is crashing when releasing the previous object at the array location [1]. The crash address is 0xffffffffe5e5e5e5, indicating we've hit a chunk of uninitialized freed memory.

[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/xpcom/glue/nsCOMArray.cpp#209

Comment 21

[Girish Sharma [:Optimizer]]

So, are we backing out or releasing the fix? Is it worth respinning nightly updates for windows?

Comment 22

[Eric Rahm [:erahm]]

(In reply to Girish Sharma [:Optimizer] from comment #21)
> So, are we backing out or releasing the fix? Is it worth respinning nightly
> updates for windows?

See comment 16, further discussion in bug 1310189.

Comment 23

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef2d0956bc0a

Comment 24

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03035b1827fc

Comment 25

[Eric Rahm [:erahm]]

Attachment: Part 8: Just use an nsTAutoArray

This is an updated version that addresses previous review comments for the Clone method and adds a copy of the template specialization for |nsTArrayElementTraits<nsISupports*>| that nsCOMArray uses [1].

It appears this fixes crashes on Windows PGO builds where we were preallocating an array, it was expected to have all elements initialized to nullptr, but instead the elements were unitialized leading to a crash when they were addrefed. My best guess is that PGO on Windows decided that we didn't need two copies of |nsTArrayElementTraits<nsISupports*>| in xpcom and it chose the version that didn't initialize memory. Due to the usage in nsISupportsArray not initializing is fine, but of course this was not fine for nsCOMArray.

We could pursue a more complete solution of sharing the specialization or maybe converting nsISupportsArray to use nsCOMPtr but given we want to get rid of nsSupportsArray I don't think it's worth the effort.

MozReview-Commit-ID: 6FR2SjOhoZR

Comment 26

[Eric Rahm [:erahm]]

Attachment: Part 8: Just use an nsTAutoArray

(removing option that forced a PGO build)

Comment 27

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #24)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=03035b1827fc

This was a forced PGO build, the build failures are just for configurations that we don't normally do PGO for. Windows no longer insta-crashes so I feel pretty good about it.

Comment 28

[Eric Rahm [:erahm]]

Attachment: Part 8: Remove nsSupportsArray::LastIndexOf

This removes the scriptable method |GetLastIndexOf| which is unused in
our codebase and turns up no references in the plugins repo. This allows to
remove the non-scriptable |LastIndexOf|.

MozReview-Commit-ID: 54Ux7yZMh4F

Comment 29

[Eric Rahm [:erahm]]

Attachment: Part 9: Remove nsSupportsArray::Compact

This removes the scriptable method |Compact| which is unused in
our codebase and turns up no references in the plugins repo.

MozReview-Commit-ID: 5sJtO5COJpB

Comment 30

[Eric Rahm [:erahm]]

Attachment: Part 10: Just use an nsCOMArray

Hopefully the last bit of churn. This switches over to nsCOMArray which avoids dealing with traits weirdness.

Comment 31

[Nathan Froyd [:froydnj]]

Comment on attachment 8801417 [details] [diff] [review]
Part 10: Just use an nsCOMArray

Review of attachment 8801417 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thank you!

::: xpcom/ds/nsSupportsArray.cpp
@@ +129,5 @@
>  NS_IMETHODIMP_(int32_t)
>  nsSupportsArray::IndexOf(const nsISupports* aPossibleElement)
>  {
> +  // nsCOMArray takes a non-const param, but it just passes through to
> +  // nsTArray which takes a const param.

Can you file a follow up bug for fixing this detail in nsCOMArray?

Comment 32

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #31)
> Comment on attachment 8801417 [details] [diff] [review]
> Part 10: Just use an nsCOMArray
> 
> Review of attachment 8801417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, thank you!
> 
> ::: xpcom/ds/nsSupportsArray.cpp
> @@ +129,5 @@
> >  NS_IMETHODIMP_(int32_t)
> >  nsSupportsArray::IndexOf(const nsISupports* aPossibleElement)
> >  {
> > +  // nsCOMArray takes a non-const param, but it just passes through to
> > +  // nsTArray which takes a const param.
> 
> Can you file a follow up bug for fixing this detail in nsCOMArray?

Sure, filed bug 1311117.

Comment 33

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7f98ccd640c4f1941bbde8dec919c6d0229c78
Bug 1308317 - Part 1: Remove debug code. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/90cefb7a313ea8bb6dff2d3a6270b3102d7fbcee
Bug 1308317 - Part 2: Remove nsSupportsArray::MoveElement. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/05201b121db7ce935f260f99de4d817054594aeb
Bug 1308317 - Part 3: Remove nsSupportsArray::SizeTo. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/53360f42f5c1292ff5259884fbcad9fc673a5ea2
Bug 1308317 - Part 4: Remove nsSupportsArray::Equals. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/776fb871785f0fbc8f529f8345f8c5a8a467f11f
Bug 1308317 - Part 5: Remove nsSupportsArray::DeleteLastElement. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/da0288fce79d6cd8365538ff0ae2208c47013e50
Bug 1308317 - Part 6: Remove nsSupportsArray::GetIndexOfStartingAt. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f59500a7c9fdaca4057b29a5e59533b3d4a5e5
Bug 1308317 - Part 7: Remove nsSupportsArray::RemoveElementsAt. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/dea75d69f2f6627d6546e4e67a69f350963d239c
Bug 1308317 - Part 8: Remove nsSupportsArray::LastIndexOf. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/f84c2e169172ff89fe43850feaa5def5c9227072
Bug 1308317 - Part 9: Remove nsSupportsArray::Compact. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1f6ee2b22395a54b4b7624e2e4a189fa3ec459d
Bug 1308317 - Part 10: Just use an nsCOMArray. r=froydnj

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d7f98ccd640
https://hg.mozilla.org/mozilla-central/rev/90cefb7a313e
https://hg.mozilla.org/mozilla-central/rev/05201b121db7
https://hg.mozilla.org/mozilla-central/rev/53360f42f5c1
https://hg.mozilla.org/mozilla-central/rev/776fb871785f
https://hg.mozilla.org/mozilla-central/rev/da0288fce79d
https://hg.mozilla.org/mozilla-central/rev/b4f59500a7c9
https://hg.mozilla.org/mozilla-central/rev/dea75d69f2f6
https://hg.mozilla.org/mozilla-central/rev/f84c2e169172
https://hg.mozilla.org/mozilla-central/rev/a1f6ee2b2239