1404198 - DeCOMtaminate some common constructors

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Kris Maglione [:kmag]]

This has been nagging me for a while now. do_CreateInstance is almost hilariously expensive, but we use it all over the place to create extremely common objects on hot code paths.

There are some other constructors we should probably do this for (nsIArray, nsISupportsCString, nsISimpleURL, nsIStandardURL come to mind), but I'm starting with nsITimer and nsIObjectInputStream/nsIObjectOutputStream because those are the ones that I constantly see showing up in profiles.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1404198: Part 1 - Add non-virtual constructor for nsIObject(Input|Output)Stream and update existing callers.

Review commit: https://reviewboard.mozilla.org/r/184926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184926/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1404198: Part 2 - Add non-virtual constructor for nsITimer and update existing callers.

Review commit: https://reviewboard.mozilla.org/r/184928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184928/

Comment 3

[Nathan Froyd [:froydnj]]

Did you actually test the nsITimer change on try?  The last time I tried doing that, ISTR there were a *ton* of places that took failure to construct nsITimers as an indication that we were in shutdown, and making those calls non-fatal in such cases wreaked all kinds of havoc.

deCOM'ing nsITimer is also bug 683785.

Comment 4

[Nathan Froyd [:froydnj]]

I should make clear that I am 100% in favor of doing this, but the nsITimer gotchas make me skeptical that it's always a search-and-replace.

We have de-COM'd functions for nsIArray:

http://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsArray.h#33

The naming is not great, though.

deCOM'ing nsISupportsCString sounds like a great idea.  nsI*URL might need some input from the Necko team.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Did you actually test the nsITimer change on try?  The last time I tried
> doing that, ISTR there were a *ton* of places that took failure to construct
> nsITimers as an indication that we were in shutdown, and making those calls
> non-fatal in such cases wreaked all kinds of havoc.

Yes, I did a full try run with both of these patches, and the only issue I found was that I accidentally fixed a broken nsIObjectOutputStream constructor.

I can add checks for shutdown to the timer constructors if necessary, though. I left all of the existing null checks in place, even though the constructors should technically be infallible now.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > Did you actually test the nsITimer change on try?  The last time I tried
> > doing that, ISTR there were a *ton* of places that took failure to construct
> > nsITimers as an indication that we were in shutdown, and making those calls
> > non-fatal in such cases wreaked all kinds of havoc.
> 
> Yes, I did a full try run with both of these patches, and the only issue I
> found was that I accidentally fixed a broken nsIObjectOutputStream
> constructor.

That's excellent!  Carry on, then!

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8913550 [details]
Bug 1404198: Part 1 - Add non-virtual constructor for nsIObject(Input|Output)Stream and update existing callers.

https://reviewboard.mozilla.org/r/184926/#review190400

r=me with the comments below addressed.

::: dom/cache/FileUtils.cpp:424
(Diff revision 1)
>    nsCOMPtr<nsIOutputStream> outputStream;
>    rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), file);
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
>    nsCOMPtr<nsIBinaryOutputStream> binaryStream =
> -    do_CreateInstance("@mozilla.org/binaryoutputstream;1");
> +    NS_NewObjectOutputStream(outputStream);

Change this to `nsCOMPtr<nsIObjectOutputStream> objectStream`.

::: dom/cache/FileUtils.cpp:744
(Diff revision 1)
>    nsCOMPtr<nsIInputStream> bufferedStream;
>    rv = NS_NewBufferedInputStream(getter_AddRefs(bufferedStream), stream, 512);
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
>    nsCOMPtr<nsIBinaryInputStream> binaryStream =
> -    do_CreateInstance("@mozilla.org/binaryinputstream;1");
> +    NS_NewObjectInputStream(bufferedStream);

ditto

::: dom/quota/ActorsParent.cpp:2147
(Diff revision 1)
> -    do_CreateInstance("@mozilla.org/binaryoutputstream;1");
> +    return NS_ERROR_UNEXPECTED;
> -  if (NS_WARN_IF(!binaryStream)) {
> -    return NS_ERROR_FAILURE;
>    }
>  
> -  rv = binaryStream->SetOutputStream(outputStream);
> +  nsCOMPtr<nsIObjectOutputStream> binaryStream =

`binaryStream` is a misleading name. Call it `objectOutputStream`?

::: dom/xul/nsXULPrototypeCache.cpp:405
(Diff revision 1)
>      bool found = mOutputStreamTable.Get(uri, getter_AddRefs(storageStream));
>      if (found) {
> -        objectOutput = do_CreateInstance("mozilla.org/binaryoutputstream;1");
> -        if (!objectOutput) return NS_ERROR_OUT_OF_MEMORY;
> +        // Setting an output stream here causes crashes on Windows. The previous
> +        // version of this code always returned NS_ERROR_OUT_OF_MEMORY here,
> +        // because it used a mistyped contract ID to create its object stream.
> +        return NS_ERROR_NOT_IMPLEMENTED;

yikes

::: xpcom/io/nsBinaryStream.cpp:49
(Diff revision 1)
>  
> +already_AddRefed<nsIObjectOutputStream>
> +NS_NewObjectOutputStream()
> +{
> +  return do_AddRef(new nsBinaryOutputStream());
> +}

Feels like this should be called NS_NewBinaryOutputStream(), because that would be more general, given that nsIBinaryOutputStream is the superclass of nsIObjectOutputStream.

Having said that, AFAICT this function isn't used anywhere so I think you should just remove it.

::: xpcom/io/nsBinaryStream.cpp:65
(Diff revision 1)
> +
> +already_AddRefed<nsIObjectInputStream>
> +NS_NewObjectInputStream()
> +{
> +  return do_AddRef(new nsBinaryInputStream());
> +}

ditto

::: xpcom/io/nsIObjectInputStream.idl:39
(Diff revision 1)
>  };
>  
>  %{C++
>  
> +already_AddRefed<nsIObjectInputStream>
> +NS_NewObjectInputStream();

Which means this decl can be removed.

::: xpcom/io/nsIObjectOutputStream.idl:62
(Diff revision 1)
>      [notxpcom] void    putBuffer(in charPtr aBuffer, in uint32_t aLength);
>  };
>  
>  %{C++
> +already_AddRefed<nsIObjectOutputStream>
> +NS_NewObjectOutputStream();

ditto

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8913551 [details]
Bug 1404198: Part 2 - Add non-virtual constructor for nsITimer and update existing callers.

https://reviewboard.mozilla.org/r/184928/#review190420

The heavy overloading of NS_NewTimer() makes this hard to read. I like the descriptiveness of the existing Init\*() functions. What do you think about NS_NewTimerAndInitWithCallback(), NS_NewTimerAndInitWithNamedFuncCallback(), etc.?

::: xpcom/threads/nsTimerImpl.cpp:88
(Diff revision 1)
> +  nsCOMPtr<nsITimer> timer;
> +  MOZ_TRY(NS_NewTimer(getter_AddRefs(timer),
> +                      aObserver,
> +                      aDelay,
> +                      aType,
> +                      nullptr));

This function doesn't use `aTarget`. Should it pass it along instead of `nullptr`?

Comment 9

[Andrew McCreight [:mccr8]]

FYI, bug 1286082 was filed on the typo, but it looks like it never got fixed because of crashes.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8913551 [details]
Bug 1404198: Part 2 - Add non-virtual constructor for nsITimer and update existing callers.

https://reviewboard.mozilla.org/r/184928/#review190420

I considered that. I went with this pattern because that's what we do for other similar constructors (e.g., NS_NewChannel), but I don't really have a strong opinion. I'm not a big fan of the `AndInit`, because I think the init function names are already kind of unreadable, but `NS_NewTimerWithCallback` sounds fine to me.

> This function doesn't use `aTarget`. Should it pass it along instead of `nullptr`?

Oops. Yes, I used a macro to generate the Result versions. I think I had `nsIEventTarget* aTarget = nullptr` in the arguments list initially, and it got mangled.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight (PTO-ish Oct 1 - 12) [:mccr8] from comment #9)
> FYI, bug 1286082 was filed on the typo, but it looks like it never got fixed
> because of crashes.

Thanks. I was going to file a bug for it, but that saves me the trouble (I ran into the same crashes when I accidentally fixed it...)

Comment 12

[Nicholas Nethercote [inactive]]

> I'm not a big fan of the `AndInit`, because I think the init
> function names are already kind of unreadable, but `NS_NewTimerWithCallback`
> sounds fine to me.

Sounds fine to me too -- more concise than what I suggested, but still informative. How about you update the patch to use names of those forms and then I'll do the review? Thanks.

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8913551 [details]
Bug 1404198: Part 2 - Add non-virtual constructor for nsITimer and update existing callers.

https://reviewboard.mozilla.org/r/184928/#review191650

I've cleared the r? in anticipation of a new patch with the updated names.

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8913550 [details]
Bug 1404198: Part 1 - Add non-virtual constructor for nsIObject(Input|Output)Stream and update existing callers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184926/diff/1-2/

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8913551 [details]
Bug 1404198: Part 2 - Add non-virtual constructor for nsITimer and update existing callers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184928/diff/1-2/

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8913551 [details]
Bug 1404198: Part 2 - Add non-virtual constructor for nsITimer and update existing callers.

https://reviewboard.mozilla.org/r/184928/#review191720

I'd prefer if there was less repeating of some of the names. See the comments near the end of the patch.

For the failure checking, AFAICT you tried to preserve the existing behaviour, whether checks were present or not? Seems reasonable, though it's depressing how inconsistent the code is with checking.

When it comes to landing this, if I were doing it I would split it into numerous separate patches, so that if there are any problems bisection is likely to be helpful.

::: dom/plugins/base/nsPluginInstanceOwner.cpp:1292
(Diff revision 2)
>    return mEventModel;
>  }
>  
>  #define DEFAULT_REFRESH_RATE 20 // 50 FPS
>  
> -nsCOMPtr<nsITimer>               *nsPluginInstanceOwner::sCATimer = nullptr;
> +StaticRefPtr<nsITimer>            nsPluginInstanceOwner::sCATimer;

Is StaticRefPtr ok to use with an nsI type? I see there are quite a few other such cases, so I guess it must be...

::: dom/plugins/base/nsPluginInstanceOwner.cpp
(Diff revision 2)
>    sCARefreshListeners->RemoveElement(this);
>  
>    if (sCARefreshListeners->Length() == 0) {
>      if (sCATimer) {
> -      (*sCATimer)->Cancel();
> +      sCATimer->Cancel();
> -      delete sCATimer;

We were calling `delete` on an nsCOMPtr??

::: layout/base/PresShell.cpp:9256
(Diff revision 2)
>    NS_PRECONDITION(!mObservingLayoutFlushes, "Shouldn't get here");
>    ASSERT_REFLOW_SCHEDULED_STATE();
>  
>    if (!mReflowContinueTimer) {
> -    mReflowContinueTimer = do_CreateInstance("@mozilla.org/timer;1");
> -    mReflowContinueTimer->SetTarget(
> +    nsresult rv;
> +    rv = NS_NewTimerWithFuncCallback(getter_AddRefs(mReflowContinueTimer),

Can combine the declaration and assignment.

::: netwerk/cache/nsCacheService.cpp:1640
(Diff revision 2)
> -    if (NS_SUCCEEDED(rv)) {
> +                                 new nsSetDiskSmartSizeCallback(),
> -        rv = mSmartSizeTimer->InitWithCallback(new nsSetDiskSmartSizeCallback(),
> -                                               1000*60*3,
> +                                 1000*60*3,
> -                                               nsITimer::TYPE_ONE_SHOT);
> +                                 nsITimer::TYPE_ONE_SHOT);
> -        if (NS_FAILED(rv)) {
> +    if (NS_FAILED(rv)) {
> -            NS_WARNING("Failed to post smart size timer");
> +        NS_WARNING("Failed to post smart size timer");

"Failed to create or post smart size timer"?

::: netwerk/cache/nsDiskCacheMap.cpp:1287
(Diff revision 2)
> -    if (NS_SUCCEEDED(rv)) {
> +    if (mCleanCacheTimer) {
> -        mCleanCacheTimer->SetTarget(nsCacheService::GlobalInstance()->mCacheIOThread);
>          rv = ResetCacheTimer();
> +    } else {
> +        rv = NS_ERROR_OUT_OF_MEMORY;
>      }

You could use ?: here.

::: netwerk/protocol/ftp/nsFtpProtocolHandler.cpp:316
(Diff revision 2)
> -
>      timerStruct* ts = new timerStruct();
>      if (!ts)
>          return NS_ERROR_OUT_OF_MEMORY;
>  
> -    rv = timer->InitWithNamedFuncCallback(
> +    nsresult rv;

You could combine this with the assignment below.

::: netwerk/protocol/websocket/WebSocketChannel.cpp:1430
(Diff revision 2)
> -  }
> -
> -  rv = mOpenTimer->InitWithCallback(this, mOpenTimeout,
> -                                    nsITimer::TYPE_ONE_SHOT);
> +                               nsITimer::TYPE_ONE_SHOT);
>    if (NS_FAILED(rv)) {
>      LOG(("WebSocketChannel::BeginOpenInternal: cannot initialize open "

"cannot create or initialize open timer"?

::: uriloader/exthandler/nsExternalHelperAppService.cpp:2535
(Diff revision 2)
>  
>        // Now close the old window.  Do it on a timer so that we don't run
>        // into issues trying to close the window before it has fully opened.
>        NS_ASSERTION(!mTimer, "mTimer was already initialized once!");
> -      mTimer = do_CreateInstance("@mozilla.org/timer;1");
> -      if (!mTimer) {
> +      NS_NewTimerWithCallback(getter_AddRefs(mTimer),
> +                              this, 0, nsITimer::TYPE_ONE_SHOT);

You removed the failure test.

::: xpcom/ds/nsExpirationTracker.h:464
(Diff revision 2)
>        this,
>        mTimerPeriod,
>        nsITimer::TYPE_REPEATING_SLACK_LOW_PRIORITY,
> -      mName);
> +      mName,
> +      target);
>      return NS_OK;

Redundant return statement.

::: xpcom/threads/nsTimerImpl.cpp:154
(Diff revision 2)
> +                                  aType,
> +                                  aTarget));
> +  return Move(timer);
> +}
> +nsresult
> +NS_NewTimerWithCallback(nsITimer** aTimer,

NS_NewTimerHighResolutionWithCallback?

::: xpcom/threads/nsTimerImpl.cpp:189
(Diff revision 2)
> +                                      aNameString,
> +                                      aTarget));
> +  return Move(timer);
> +}
> +nsresult
> +NS_NewTimerWithFuncCallback(nsITimer** aTimer,

NS_NewTimerWithNamedFuncCallback?

::: xpcom/threads/nsTimerImpl.cpp:228
(Diff revision 2)
> +                                      aNameCallback,
> +                                      aTarget));
> +  return Move(timer);
> +}
> +nsresult
> +NS_NewTimerWithFuncCallback(nsITimer** aTimer,

NS_NewTimerWithNameableFuncCallback?

::: xpfe/components/directory/nsDirectoryViewer.cpp:896
(Diff revision 2)
>      mNodeList->AppendElement(child, /*weak = */ false);
>  
>  	if (!mTimer)
>  	{
> -		mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> -		NS_ASSERTION(NS_SUCCEEDED(rv), "unable to create a timer");
> +        NS_NewTimerWithFuncCallback(getter_AddRefs(mTimer),
> +                                    nsHTTPIndex::FireTimer,

The old code checked for failure, the new code should too. You can just do `return NS_NewTimerWithFuncCallback(...)`.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8913551 [details]
Bug 1404198: Part 2 - Add non-virtual constructor for nsITimer and update existing callers.

https://reviewboard.mozilla.org/r/184928/#review191720

> Is StaticRefPtr ok to use with an nsI type? I see there are quite a few other such cases, so I guess it must be...

Yeah, RefPtr types work fine with `nsI*` types, they just don't support do_QueryInterface, and store their original pointer rather than casting to nsISupports.

> We were calling `delete` on an nsCOMPtr??

No, we were calling `delete` on a `nsCOMPtr*`. Which is really hard to make sense of. Which is why I changed this to a `StaticRefPtr`.

> "Failed to create or post smart size timer"?

The creation part is currently infallible, though I suppose we could change that.

> You removed the failure test.

The failure test was only for timer creation. Failure to initialize was always ignored. Though it probably makes sense to return on either failure.

> NS_NewTimerHighResolutionWithCallback?

The `HighResolution` variant isn't special except in terms of the type it takes for the the delay. The other variant just casts its integer argument to a TimeDuration, so I don't think the naming distinction is really necessary.

> NS_NewTimerWithNamedFuncCallback?

I thought about it, but nearly every other place we take a name when creating a runnable from a function, the naming is implicit (e.g., NewRunnableMethod), and the longer these constructor names get, the harder I find them to read.

> NS_NewTimerWithNameableFuncCallback?

Same as above. I don't think it adds anything to give these variants separate names. It's pretty clear at the call location that the name argument is either a name string or a name callback.

> The old code checked for failure, the new code should too. You can just do `return NS_NewTimerWithFuncCallback(...)`.

The old code only checked for failure to create the timer, which is infallible now. Although admittedly now we don't set the mTimer value if either part fails, so returning failure probably does make sense.

Comment 18

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f6ff8f40729503c749f1e7f966182807fa7993f
Bug 1404198: Part 1 - Add non-virtual constructor for nsIObject(Input|Output)Stream and update existing callers. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1fbe9a79c6e59cb31ff0a08cdf9a5c04f1c5d3
Bug 1404198: Part 2a - Add non-virtual constructors for nsITimer. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f1ce79ba5cd667c098fe188853a3dbbba2c15e
Bug 1404198: Part 2b - Switch to NS_NewTimer* in xpcom. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa53cb4191939e1d66446f5e45175e0079197082
Bug 1404198: Part 2c - Switch to NS_NewTimer* in necko. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c3876d6d2f31f137dd9d62c870ed6320963770
Bug 1404198: Part 2d - Switch to NS_NewTimer* in widget. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/474d24f4545fb096b91a6eafec3b76e528ef1092
Bug 1404198: Part 2e - Switch to NS_NewTimer* in layout. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/08f1f4f0aa5399f25fbe71ccd4900582e4cda039
Bug 1404198: Part 2f - Switch to NS_NewTimer* in toolkit. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/71117dd0686c940af0e41834895c86a5abf1cc4d
Bug 1404198: Part 2g - Switch to NS_NewTimer* in media. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/247d3966c7ac1220c39a86b31b8a98bfbc4895fe
Bug 1404198: Part 2h - Switch to NS_NewTimer* in gfx. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/e405a4ec473c3d510047fff16599a5f73a8d600b
Bug 1404198: Part 2i - Switch to NS_NewTimer* in dom. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/33b50e69601405149cb7100a8bd774a2bad035a6
Bug 1404198: Part 2j - Switch to NS_NewTimer* everywhere else. r=njn

Comment 19

[Nicholas Nethercote [inactive]]

Thank you for splitting up part 2.

Comment 20

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53e535e09300f296159e21e695b43ae31b11b25d
Bug 1404198: Follow-up: Fix OS-X build bustage. r=bustage

Comment 21

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f08de0f71a04f5d1fe025df06ac1fd109d37404
Bug 1404198: Follow-up: Fix more OS-X build bustage on a CLOSED TREE r=bustage

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4f6ff8f40729
https://hg.mozilla.org/mozilla-central/rev/1e1fbe9a79c6
https://hg.mozilla.org/mozilla-central/rev/e1f1ce79ba5c
https://hg.mozilla.org/mozilla-central/rev/fa53cb419193
https://hg.mozilla.org/mozilla-central/rev/e7c3876d6d2f
https://hg.mozilla.org/mozilla-central/rev/474d24f4545f
https://hg.mozilla.org/mozilla-central/rev/08f1f4f0aa53
https://hg.mozilla.org/mozilla-central/rev/71117dd0686c
https://hg.mozilla.org/mozilla-central/rev/247d3966c7ac
https://hg.mozilla.org/mozilla-central/rev/e405a4ec473c
https://hg.mozilla.org/mozilla-central/rev/33b50e696014
https://hg.mozilla.org/mozilla-central/rev/53e535e09300
https://hg.mozilla.org/mozilla-central/rev/7f08de0f71a0