1514936 - Get rid of support for aggregated components

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Kris Maglione [:kmag]]

There are only two classes that use the aggregated component machinery, nsProperties and nsLoadGroup. Neither of those, however, actually uses an outer class.

It's probably time to get rid of all of this machinery. It adds complexity all over the place, and very, very few people have any idea what it's actually supposed to do.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - push nullptr outer aggregates all the way down into factories

This commit is prep work for the real change in part 2 and is intended
to make part 2 somewhat easier to review.  This change also ensures that
we're not passing non-nullptr outer objects in anywhere.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - remove outer object support from xpcom creation functions

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - remove nsAgg.h

Now that nothing else uses it...

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 9032472 [details] [diff] [review]
part 1 - push nullptr outer aggregates all the way down into factories

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

Thanks for splitting this out.

::: xpcom/components/nsComponentManagerUtils.cpp
@@ +154,5 @@
>  #endif
>  
>  nsresult nsCreateInstanceByCID::operator()(const nsIID& aIID,
>                                             void** aInstancePtr) const {
> +  nsresult status = CallCreateInstance(mCID,  aIID, aInstancePtr);

nit: double space before aIID.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 9032473 [details] [diff] [review]
part 2 - remove outer object support from xpcom creation functions

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

Thanks for fixing this.

Can NS_ERROR_NO_AGGREGATION be removed? Here or in a followup.

You should post about this on dev-platform and firefox-dev. That'll at least give people writing JS a heads up as to why their new createInstance calls are failing weirdly.

You notify a TB person, because I could imagine that this will require a lot of fixes there. Hopefully they don't use aggregates.

::: netwerk/base/nsFileStreams.cpp
@@ -406,5 @@
>                              nsITellableStream, nsILineInputStream)
>  
> -nsresult nsFileInputStream::Create(nsISupports* aOuter, REFNSIID aIID,
> -                                   void** aResult) {
> -  NS_ENSURE_NO_AGGREGATION(aOuter);

This poor underutilized macro.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: remove NS_ERROR_NO_AGGREGATION

Part 4 followup!

Comment 7

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30) from comment #5)
> You notify a TB person, because I could imagine that this will require a lot
> of fixes there. Hopefully they don't use aggregates.

I checked when I filed the bug. Apparently RDF uses it, but hopefully it "uses" it the way nsLoadGroup did. If not, it shouldn't be much work to get the same behavior without the aggregate constructor stuff. They'll just have to create instances more directly.

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks, I'll keep an eye on it. de-RDF is actually progressing, just not fast enough. Since moving the RDF service to C-C we had a bit of bustage in it :-(

Comment 9

[Kris Maglione [:kmag]]

(In reply to Jorg K (GMT+1) from comment #8)
> Thanks, I'll keep an eye on it. de-RDF is actually progressing, just not
> fast enough. Since moving the RDF service to C-C we had a bit of bustage in
> it :-(

Might want to add a `MOZ_RELEASE_ASSERT(!aOuter);` here[1] and do a try run with it. If it doesn't trip, you can just remove all of the aggregation stuff from that class. If it does, you're going to need fix those callers to call the InMemoryDataSource constructor directly. And if you want to continue using the aggregation macros, you'll need to move nsAgg.h into comm-central.

[1]: https://searchfox.org/comm-central/rev/5a7cb6346ed51a400bc6910465b0cdf9c90af5f9/rdf/base/nsInMemoryDataSource.cpp#719

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:froydnj, could you have a look please?

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 12

[Andrew McCreight [:mccr8]]

Peter removed nsAgg already in bug 1632802, which presumably means that we're really not using the aggregated components stuff any more.

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 9032474 [details] [diff] [review]
part 3 - remove nsAgg.h

As I said, this is obsolete because it was already removed.

Comment 14

[Andrew McCreight [:mccr8]]

I have some updated patches for this.

Comment 15

[Andrew McCreight [:mccr8]]

One fun thing I uncovered that doesn't seem to be addressed in Nathan's old patches is that we apparently have a test that nsIFactory is binary compatible with COM's IClassFactory. From some light auditing of the code base and the fact that this is the only Windows-specific failure, we seem to not actually rely on this, so maybe I'll file a separate bug to explicitly remove this compatibility and get both XPCOM and MSCOM peer review on it, in case I'm missing something.

Comment 16

[Andrew McCreight [:mccr8]]

The remaining failure in my try push is in some Linux ASan Py3 mch test suite, which we don't seem to actually run normally on mozilla-central or autoland, so I'm going to assume that's a preexisting failure. (Maybe the ASan exit codes are weird and that's confusing the test?)

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: Bug 1514936, part 1 - Remove the outer argument to nsIFactory::createInstance.

This patch won't actually build, because a few bits of code are used
for both nsIFactory::createInstance and static components, and static
components are not fixed until the next patch.

The first place is nsLoadGroupConstructor, which uses an nsIFactory
macro to create a static component constructor. (This could be worked
around by expanding the macro to the state before this patch.)

The other issue is that nsAppShellConstructor is used in an nsIFactory
on OSX, but as a static component on all other platforms. This could
be worked around by wrapping nsAppShellConstructor in an adaptor that
passes in the extra null argument to nsAppShellConstructor.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: Bug 1514936, part 2 - Drop the outer arguments from static components.

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: Bug 1514936, part 3 - Remove NS_ERROR_NO_AGGREGATION which is no longer used.

Comment 20

[Andrew McCreight [:mccr8]]

Strings I searched for to track down all of these places: NS_ERROR_NO_AGGREGATION, aOuter, (note the trailing comma), createInstance(outer, createInstance(delegate

There are a few false positives with those, but not more than a dozen or so. Unfortunately you can't just search for "createInstance" as that mostly ends up with places calling it, not defining it.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b80ed974bdcc
part 1 - Remove the outer argument to nsIFactory::createInstance. r=xpcom-reviewers,preferences-reviewers,nika,Gijs
https://hg.mozilla.org/integration/autoland/rev/36444204bce1
part 2 - Drop the outer arguments from static components. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/2376830e18d5
part 3 - Remove NS_ERROR_NO_AGGREGATION which is no longer used. r=xpcom-reviewers,nika

Comment 22

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/b80ed974bdcc
https://hg.mozilla.org/mozilla-central/rev/36444204bce1
https://hg.mozilla.org/mozilla-central/rev/2376830e18d5