271823 - do_GetIOService should use nsGetServiceByCID

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Bernard Alleysson]

Let do_GetIOService() take advantage of the improvements after bug 264456

Comment 1

[Bernard Alleysson]

Attachment: use nsGetServiceByCID

Comment 2

[Bernard Alleysson]

Comment on attachment 167089 [details] [diff] [review]
use nsGetServiceByCID

Would you review the patch ? Just a few lines and it makes do_GetIOService()
use the improved nsGetServiceByCID

Comment 3

[Darin Fisher]

Comment on attachment 167089 [details] [diff] [review]
use nsGetServiceByCID

my only concern is that we're now duplicating the static variable.  is that
better or worse than always using nsGetServiceByCIDWithError?

Comment 4

[Darin Fisher]

How does this change impact codesize?

Comment 5

[Bernard Alleysson]

Attachment: remove static variable duplication

Well, static variable duplication is a problem with the use of
do_GetService(CID), CID constant structures are duplicated in many files.

There is less duplication with the use of contract IDs because they are string
constants and string constant duplicates are merged (per DLL/EXE) by the
compiler at -O1 (-Gf/-GF for VC98/VC7, -fmerge-constants for GCC). I guess that
CID constant structures would be merged by GCC if Mozilla was compiled with
-fmerge-all-constants.

This patch will only save a few bytes of code, you can resolve this bug as
wontfix if you don't want it :-)

Comment 6

[Darin Fisher]

Comment on attachment 168953 [details] [diff] [review]
remove static variable duplication

No, the intent of do_GetIOService is to invoke the IO service by ClassID.

Your patch includes a number of other changes that I like.  Any explicit
callers of do_GetService(kIOServiceCID) or the like can be switched to
do_GetIOService.

Comment 7

[Bernard Alleysson]

(In reply to comment #6)
> Any explicit callers of do_GetService(kIOServiceCID) or the like can be
> switched to do_GetIOService.

I can do that.

Do you mean inside of mozilla\netwerk or everywhere ?

There are quite a lot of do_GetService(NS_IO_CONTRACTID), or even
CallGetService(NS_IO_CONTRACTID, ...) around, should they be converted to
do_GetIOService() (invoking by CID instead of contract id) ?

In any case I won't include nsNetUtil.h or otherwise REQUIRES necko if it is not
available.

Comment 8

[Darin Fisher]

> Do you mean inside of mozilla\netwerk or everywhere ?

I was talking about inside netwerk, yes.  We could do it elsewhere, but perhaps
that is just unnecessary overhead.


> There are quite a lot of do_GetService(NS_IO_CONTRACTID), or even
> CallGetService(NS_IO_CONTRACTID, ...) around, should they be converted to
> do_GetIOService() (invoking by CID instead of contract id) ?

Hard to say.  It's likely that invoking by CID is slightly faster (fewer bytes
to compare), but I'm not sure the difference really matters in practice.

Some say that invoking by ContractID allows other implementations of that
ContractID to override the default one.  But, in practice you can do the same
thing by overriding by ClassID.  Technically, I get the difference (ClassID
indicates a particular class, and ContractID only indicates a set of supported
interfaces), but in fact, people usually override both ContractID and ClassID.

In short, I doubt optimizing do_GetIOService will save us much.  I think our
effort would be better spent on optimizing more frequently called code :-/

Comment 9

[Bernard Alleysson]

Attachment: cleanup do_GetXXX(id, &rv) when rv is not needed

(In reply to comment #6)
> Your patch includes a number of other changes that I like.

I've kept the other changes and I've done some cleanup of
do_GetService/do_QueryInterface usage inside mozilla\netwerk
Note that this has nothing to do with the original bug report but since
removing rv will save a few code bytes, it could be considered.

Comment 10

[Darin Fisher]

Comment on attachment 169368 [details] [diff] [review]
cleanup do_GetXXX(id, &rv) when rv is not needed

sr=darin (sorry for not reviewing this sooner)

Comment 11

[Bernard Alleysson]

Darin,
does this need r+ also ? Should I ask bryner for it ?

Comment 12

[Darin Fisher]

Comment on attachment 169368 [details] [diff] [review]
cleanup do_GetXXX(id, &rv) when rv is not needed

no need to get an additional review.  this is ready to check in.

Comment 13

[jag (Peter Annema)]

Comment on attachment 169368 [details] [diff] [review]
cleanup do_GetXXX(id, &rv) when rv is not needed

>Index: base/src/nsIOService.cpp
>===================================================================
> nsIOService::nsIOService()
>     : mOffline(PR_FALSE),
>       mOfflineForProfileChange(PR_FALSE)
> {
...
>-        rv = recyclingAllocator->Init(NS_NECKO_BUFFER_CACHE_COUNT,
>+        nsresult rv = recyclingAllocator->Init(NS_NECKO_BUFFER_CACHE_COUNT,
>                                       NS_NECKO_15_MINS, "necko");

One nit:

Could you line up the argument on the second line with the argument on the first?

Comment 14

[Darin Fisher]

-> reassign to default owner

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Attachment: Un-bitrotted patch v2

OK, I've un-bitrotted attachment 169368 [details] [diff] [review]. I have a few notes:

nsFileChannel.cpp - the affected code has moved around a bit since the patch was written. I'm not completely sure if the change is still safe to make in that spot.

nsFtpConnectionThread.cpp - I see a lot of other instances of the same type of pattern nsCOMPtr<something> foo = do_GetService(SOME_CID, &rv) with if (NS_FAILED(rv)) return rv; on the next line. Is there a good way to know which of those are safe to clean in the same way as the others here and which ones aren't?

nsHttpChannel.cpp - Same question as nsFtpConnectionThread.cpp.

I'm re-requesting review since the patch is over 3 years old and want to make sure all is still well with what it's trying to accomplish.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Attachment: Un-bitrotted patch v3

The patch from bug 411119 bitrotted part of this patch, so I did my best to make the changes in the spirit of this bug while maintaining the work from that patch. However, I really don't know my way around this code at all, so if you can look at it, David and biesi, I would be greatly appreciative.

Jeremy, can you apply this patch and verify that it hasn't regressed bug 411119? And biesi, if you can answer my questions in comment #15, I would be greatly appreciative!

The code compiles and I'm posting from a browser which uses it right now, so I believe everything is in order. The main thing I'm wondering about is whether or not there is room for further optimization within these files.

Comment 17

[David Luyer]

The nsSOCKSIOLayer.cpp change looks fine.  Yes there are opportunities for
further improvement in the SOCKS code, however the amount of use it gets
probably would not justify the effort, as there is scope for improvement
in much more common code paths.

For example, if you have an IPv4-only SOCKS proxy on localhost on a Mac,
the current code will attempt and fail an IPv6 connect to localhost every
time before succeeding on the IPv4 address.  It could be made smarter,
but this is most likely a rare corner case and for the vast majority of the
cases any overhead to deal with this case would just be overhead (as most
uses of localhost SOCKS would be SSH tunnels, and SSH supports v4 and v6).

(similar example applies to round-robin DNS SOCKS proxy collection with one
down)

Comment 18

[jag (Peter Annema)]

Comment on attachment 296083 [details] [diff] [review]
Un-bitrotted patch v3

Ryan, thanks for picking up this patch!

To answer your question: "I see a lot of other instances of the same type of
pattern nsCOMPtr<something> foo = do_GetService(SOME_CID, &rv) with if
(NS_FAILED(rv)) return rv; on the next line. Is there a good way to know which
of those are safe to clean in the same way as the others here and which ones
aren't?"

I suggest you just leave those alone. The ones that are "safe" to clean are the ones where the rv isn't returned or used other than to test whether the |do_GetService(...)| call succeeded. See my comments below.

I didn't really mean to do a complete review, but:

>Index: netwerk/base/src/nsIOService.cpp
>===================================================================
> nsresult
> nsIOService::Init()
> {
...
>-    mSocketTransportService = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
>-    if (NS_FAILED(rv)) {
>+    mSocketTransportService = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID);
>+    if (!mSocketTransportService)
>         NS_WARNING("failed to get socket transport service");
>-        return rv;
>-    }

With this change you warn, but you don't return early, with some kind of error, so you'll likely fail down the road (either here or later on when something expects |mSocketTransportService| to be non-null, since this didn't return an error). At the least return NS_ERROR_FAILURE, but really just undo this change.

>-    mDNSService = do_GetService(NS_DNSSERVICE_CONTRACTID, &rv);
>-    if (NS_FAILED(rv)) {
>+    mDNSService = do_GetService(NS_DNSSERVICE_CONTRACTID);
>+    if (!mDNSService)
>         NS_WARNING("failed to get DNS service");
>-        return rv;
>-    }

Same here.


>Index: netwerk/protocol/file/src/nsFileChannel.cpp
>===================================================================
...
>   if (isDir) {
>     rv = nsDirectoryIndexStream::Create(file, getter_AddRefs(stream));
>     if (NS_SUCCEEDED(rv) && !HasContentTypeHint())
>       contentType.AssignLiteral(APPLICATION_HTTP_INDEX_FORMAT);
>   } else {
>     rv = NS_NewLocalFileInputStream(getter_AddRefs(stream), file);
>     if (NS_SUCCEEDED(rv) && !HasContentTypeHint()) {
>       // Use file extension to infer content type
>-      nsCOMPtr<nsIMIMEService> mime = do_GetService("@mozilla.org/mime;1", &rv);
>-      if (NS_SUCCEEDED(rv)) {
>+      nsCOMPtr<nsIMIMEService> mime = do_GetService("@mozilla.org/mime;1");
>+      if (mime)
>         mime->GetTypeFromFile(file, contentType);
>-      }
>     }
>   }
>   return rv;
> }

This isn't right. Previously if for whatever reason there was a problem getting the mime service, |rv| would be have kind of error code, which it would return at the end of this function. Now it'll still have the success code from the |NS_NewLocalFileInputStream(...)| call, indicating success even in case of failure.

Here too just undo the change.


>Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp
>===================================================================
...
>     if (newDataConn) {
>         // now we know where to connect our data channel
>         nsCOMPtr<nsISocketTransportService> sts =
>-            do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
>+            do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID);

Wow. This should have something like:

          if (!sts)
              return FTP_ERROR;

Since you're here, could you add that?


>Index: netwerk/protocol/ftp/src/nsFtpControlConnection.cpp
>===================================================================
>     // build our own
>     nsresult rv;
>     nsCOMPtr<nsISocketTransportService> sts =
>-            do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
>-    if (NS_FAILED(rv))
>-        return rv;
>+            do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID);

Just undo this change.


>Index: netwerk/protocol/gopher/src/nsGopherChannel.cpp
>===================================================================
>     // Create socket tranport
>     nsCOMPtr<nsISocketTransportService> sts = 
>-             do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
>-    if (NS_FAILED(rv))
>-        return rv;
>+             do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID);
>+

Same here.


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp
>===================================================================
...
>+    nsCOMPtr<nsIURL> newURL = do_QueryInterface(newURI);
>+    if (newURL) {
>         nsCAutoString ref;
>         rv = newURL->GetRef(ref);
>         if (NS_SUCCEEDED(rv) && ref.IsEmpty()) {
>-            nsCOMPtr<nsIURL> baseURL( do_QueryInterface(mURI, &rv) );
>-            if (NS_SUCCEEDED(rv)) {
>+            nsCOMPtr<nsIURL> baseURL( do_QueryInterface(mURI) );

While you're here, could you remove the spaces around |do_QueryInterface(mURI)|?


>Index: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
>===================================================================
> PRBool
> mozTXTToHTMLConv::CheckURLAndCreateHTML(
>      const nsString& txtURL, const nsString& desc, const modetype mode,
>      nsString& outputHTML)
> {
>   // Create *uri from txtURL
>   nsCOMPtr<nsIURI> uri;
>-  nsresult rv = NS_OK;
>+  nsresult rv;
>   if (!mIOService)
>+  {
>     mIOService = do_GetService(kIOServiceCID, &rv);
>-  
>-  if (NS_FAILED(rv) || !mIOService)
>     return PR_FALSE;
>+  }

So this code used to lazily initialize mIOService and if that failed it would return PR_FALSE. You've changed it to always return PR_FALSE the first time through this code. You really want:

  // Lazily initialize mIOService
  if (!mIOService) {
    mIOService = do_GetService(kIOServiceCID);

    if (!mIOService)
      return PR_FALSE;
  }

Note that this variant skips the second null check if |mIOService| was non-null the first time around.


>Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp
>===================================================================

>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

What changed here? Looks like some UTF-8 character got inserted. Ah, EFBBBF. It's a UTF-8 BOM character. Please undo that?

sr- for now. Please attach a new patch with those changes made.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Attachment: Address jag's comments

jag, thanks for reviewing the patch. Attached is a patch making all the changes you requested.

Comment 20

[jag (Peter Annema)]

Comment on attachment 296604 [details] [diff] [review]
Address jag's comments

One more nit:

>Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp
>===================================================================
> nsresult
> nsIndexedToHTML::Init(nsIStreamListener* aListener) {
>-    nsresult rv = NS_OK;
>-
>     mListener = aListener;
> 
>-    mDateTime = do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID, &rv);
>+    mDateTime = do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID);

I guess do_CreateInstance() is unlikely to fail other than in an out-of-memory situation, but I would keep the rv and add |if (NS_FAILED(rv)) return rv;|

Please do attach a final patch, but wait for biesi's review.

sr=jag

Comment 21

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

Comment on attachment 296604 [details] [diff] [review]
Address jag's comments

this bug doesn't seem like the best place for a cleanup patch like this but whatever

+++ netwerk/streamconv/converters/mozTXTToHTMLConv.cpp	11 Jan 2008 22:53:47 -0000
+    mIOService = do_GetService(kIOServiceCID);

make this use do_GetIOService()?

Comment 22

[jag (Peter Annema)]

What's funny is that that calls nsGetServiceByContractIDWithError(NS_IOSERVICE_CONTRACTID, error).

You could add a version that doesn't take (and pass on) the error argument (make sure to drop the default value from the current one), and then call it.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Attachment: v5: Updated to jag's and biesi's comments

(In reply to comment #20)
> (From update of attachment 296604 [details] [diff] [review])
> One more nit:
> 
> I guess do_CreateInstance() is unlikely to fail other than in an out-of-memory
> situation, but I would keep the rv and add |if (NS_FAILED(rv)) return rv;|
> 

This code changed quite a bit from the last time I worked on this patch, but I think I did it correctly. Let me know if I messed up.

(In reply to comment #21)
> make this use do_GetIOService()?

I'm not familiar with this code, but I was able to piece together how to use it by looking at other code in the tree that calls it. I think I did it right (and things seem to be working OK while I'm using the browser), but let me know if something needs to be changed.

(In reply to comment #22)
> What's funny is that that calls
> nsGetServiceByContractIDWithError(NS_IOSERVICE_CONTRACTID, error).
> 
> You could add a version that doesn't take (and pass on) the error argument
> (make sure to drop the default value from the current one), and then call it.
> 

Followup bug?

Comment 24

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

Comment on attachment 330394 [details] [diff] [review]
v5: Updated to jag's and biesi's comments

mozTXTToHTMLConv.cpp
+    mIOService = do_GetIOService(&rv);

just do:
  mIOService = do_GetIOService();

+++ b/netwerk/streamconv/converters/nsIndexedToHTML.cpp
     nsresult rv = NS_OK;
+    if (NS_FAILED(rv))
+      return rv;

So, that's not what jag meant.

He meant adding this if after the mDateTime = line (and keeping the &rv in that line)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Attachment: [checked in] v6: Updated to biesi's comments

OK, so clearly assuming I knew what I was doing wasn't true :-). This fixes biesi's comments. Carrying over jag's previous sr+ now that I'm actually doing what he said to do properly.

Comment 26

[Rob Arnold [:robarnold]]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/811074481f5d

Comment 27

[Justin Dolske [:Dolske]]

netwerk/streamconv/converters/nsIndexedToHTML.cpp contained a UTF-8 BOM character in the first line, which seems to have broken the build. I pushed 4c2edd49e589 to remove it, you might want to take care with your editor. :)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Ah crap, I fixed that in an earlier patch and then forgot to do it in the final patch. Sorry for the bustage.

Comment 29

[[e-mail]]

Comment on attachment 331219 [details] [diff] [review]
[checked in] v6: Updated to biesi's comments

>             // Guessing causes crashes.
>             // (Of course, the parsing code should be more robust...)
>-            nsresult rv;
>-            nsCOMPtr<nsIStringBundleService> bundleService =
>-                    do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
>-            if (NS_FAILED(rv))
>+             nsCOMPtr<nsIStringBundleService> bundleService =
>+                do_GetService(NS_STRINGBUNDLE_CONTRACTID);
>+            if (!bundleService)
Indentation of nsCOMPtr seems to be off by one?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Attachment: [checked in] Fix the indentation error

Whoops, nice catch.

Comment 31

[Reed Loden [:reed]]

Pushed as 17114:568635438d36.