Closed Bug 1308317 Opened 3 years ago Closed 3 years ago

Clean up nsSupportsArray

Categories

(Core :: XPCOM, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(10 files, 3 obsolete files)

8.37 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.33 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.82 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.46 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.32 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
5.82 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.98 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.63 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.24 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
13.25 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
+++ 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.
MozReview-Commit-ID: 7TQG7flcJkj
Attachment #8798686 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
|MoveElement| is not scriptable and unused in our codebase.

MozReview-Commit-ID: CBe8WZHG1JG
Attachment #8798687 - Flags: review?(nfroyd)
|SizeTo| is not scriptable and unused in our codebase.

MozReview-Commit-ID: 1DrTm46qbar
Attachment #8798688 - Flags: review?(nfroyd)
|Equals| is not scriptable and unused in our codebase.

MozReview-Commit-ID: BsbJIuR9fSk
Attachment #8798689 - Flags: review?(nfroyd)
|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
Attachment #8798690 - Flags: review?(nfroyd)
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
Attachment #8798691 - Flags: review?(nfroyd)
MozReview-Commit-ID: H3A3gxckw5o
Attachment #8798692 - Flags: review?(nfroyd)
Attached patch Part 8: Just use an nsTAutoArray (obsolete) — Splinter Review
This switches over from using a half-baked auto array to nsTAutoArray.

MozReview-Commit-ID: 6FR2SjOhoZR
Attachment #8798693 - Flags: review?(nfroyd)
Attachment #8798686 - Flags: review?(nfroyd) → review+
Attachment #8798687 - Flags: review?(nfroyd) → review+
Attachment #8798688 - Flags: review?(nfroyd) → review+
Attachment #8798689 - Flags: review?(nfroyd) → review+
Attachment #8798690 - Flags: review?(nfroyd) → review+
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.
Attachment #8798691 - Flags: review?(nfroyd) → review+
Attachment #8798692 - Flags: review?(nfroyd) → review+
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;
Attachment #8798693 - Flags: review?(nfroyd) → review+
backed this out since this might have caused pgo crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=5271518&repo=mozilla-central
Status: RESOLVED → REOPENED
Flags: needinfo?(erahm)
Resolution: FIXED → ---
Looks like just Windows in this case? I'm not sure how this change affects nsCOMArray at all...
Flags: needinfo?(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
Target Milestone: mozilla52 → ---
So, are we backing out or releasing the fix? Is it worth respinning nightly updates for windows?
(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.
Attached patch Part 8: Just use an nsTAutoArray (obsolete) — Splinter Review
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
Attachment #8801379 - Flags: review?(nfroyd)
Attachment #8798693 - Attachment is obsolete: true
Attached patch Part 8: Just use an nsTAutoArray (obsolete) — Splinter Review
(removing option that forced a PGO build)
Attachment #8801381 - Flags: review?(nfroyd)
Attachment #8801379 - Attachment is obsolete: true
Attachment #8801379 - Flags: review?(nfroyd)
(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.
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
Attachment #8801415 - Flags: review?(nfroyd)
Attachment #8801381 - Attachment is obsolete: true
Attachment #8801381 - Flags: review?(nfroyd)
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
Attachment #8801416 - Flags: review?(nfroyd)
Hopefully the last bit of churn. This switches over to nsCOMArray which avoids dealing with traits weirdness.
Attachment #8801417 - Flags: review?(nfroyd)
Attachment #8801415 - Flags: review?(nfroyd) → review+
Attachment #8801416 - Flags: review?(nfroyd) → review+
Blocks: 1310040
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?
Attachment #8801417 - Flags: review?(nfroyd) → review+
(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.
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
You need to log in before you can comment on or make changes to this bug.