560095 - Make use of mozilla::services

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mitchell Field [:Mitch]]

Following the great work done in bug 516085 to provide easy access for common global services, we can now start replacing do_GetService() calls with mozilla::services::Get*Service().

In forthcoming patches I'm doing so with GetObserverService(), but I got a bit carried away and nuked hundreds of unnecessary {} brackets in the process.

Comment 1

[Mitchell Field [:Mitch]]

Attachment: /toolkit (observer service, v1)

Comment 2

[Mitchell Field [:Mitch]]

Attachment: layout (observer service, v1)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Our style guide says

> Always brace controlled statements, even a single-line consequent of an if else
> else. This is redundant typically, but it avoids dangling else bugs, so it's
> safer at scale than fine-tuning.

So removing those {}s is not right.

Comment 4

[Mitchell Field [:Mitch]]

Attachment: netwerk (observer service, v1)

Comment 5

[Mitchell Field [:Mitch]]

What a horrible style rule.

Comment 6

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

Comment on attachment 439763 [details] [diff] [review]
netwerk (observer service, v1)

+++ b/netwerk/cache/src/nsCacheService.cpp
+    if (!observerService)
+        return NS_ERROR_FAILURE;
     NS_ENSURE_ARG(observerService);

you don't need both of those checks

+    if (obs)
+        for (unsigned int i=0; i<NS_ARRAY_LENGTH(observerList); i++)
+             obs->RemoveObserver(this, observerList[i]);

please keep the {}, I find multi-line bodies easier to read if they're bracketed even when they're a single statement.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> What a horrible style rule.

It's there because not having it led to serious bugs, including security bugs, in the past.  Put the curlies back, please.

Comment 8

[Mitchell Field [:Mitch]]

Attachment: netwerk (observer service, v2)

Review comments addressed.

Comment 9

[Mitchell Field [:Mitch]]

Attachment: layout (observer service, v2)

Comment 10

[Mitchell Field [:Mitch]]

Attachment: toolkit (observer service, v2)

Note: I changed "obssvc" to "obsSvc" for a bit of consistency within one file.

Comment 11

[Mitchell Field [:Mitch]]

Attachment: toolkit (observer service, v3)

I didn't realise the toolkit component encompassed "chrome" and "profile" directories.

Comment 12

[Mitchell Field [:Mitch]]

Attachment: accessible (observer service, v1)

Comment 13

[Mitchell Field [:Mitch]]

Attachment: content (observer service, v1)

Comment 14

[Mitchell Field [:Mitch]]

Attachment: docshell (observer service, v1)

Comment 15

[Mitchell Field [:Mitch]]

Attachment: dom (observer service, v1)

Comment 16

[Mitchell Field [:Mitch]]

Attachment: embedding (observer service, v1)

Comment 17

[Mitchell Field [:Mitch]]

Attachment: gfx (observer service, v1)

Comment 18

[Mitchell Field [:Mitch]]

Attachment: intl (observer service, v1)

Comment 19

[Mitchell Field [:Mitch]]

Attachment: modules (observer service, v1)

Comment 20

[Mitchell Field [:Mitch]]

Attachment: parser (observer service, v1)

Comment 21

[Mitchell Field [:Mitch]]

Attachment: storage (observer service, v1)

Comment 22

[Mitchell Field [:Mitch]]

Attachment: widget (observer service, v1)

Comment 23

[Mitchell Field [:Mitch]]

Attachment: xpcom (observer service, v1)

Comment 24

[Mitchell Field [:Mitch]]

Attachment: xpinstall (observer service, v1)

Comment 25

[Mitchell Field [:Mitch]]

Sorry for all the bug spam. There has to be a better way to do this.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do we actually need to use nsCOMPtr to hold the service? I don't think we do.

Comment 27

[(dormant account)]

(In reply to comment #26)
> Do we actually need to use nsCOMPtr to hold the service? I don't think we do.

The services still need to be reference-counted as they are also accessed via JS. I'd rather not have someone accidentally hold on to a service until after it is shutdown.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #27)
> (In reply to comment #26)
> > Do we actually need to use nsCOMPtr to hold the service? I don't think we do.
> 
> The services still need to be reference-counted as they are also accessed via
> JS.

But isn't the mozilla::service layer holding a reference to the service? Why does layout etc need to add a reference?

Comment 29

[(dormant account)]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > Do we actually need to use nsCOMPtr to hold the service? I don't think we do.
> > 
> > The services still need to be reference-counted as they are also accessed via
> > JS.
> 
> But isn't the mozilla::service layer holding a reference to the service? Why
> does layout etc need to add a reference?

It holds one reference, until xpcom shutdown when it goes away. At which point any references via raw pointers will be bad.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK but most of the uses here are storing the service reference in an nsCOMPtr local variable. Those surely aren't needed.

Comment 31

[(dormant account)]

(In reply to comment #30)
> OK but most of the uses here are storing the service reference in an nsCOMPtr
> local variable. Those surely aren't needed.

Indeed, but I'd rather not risk having that leak(or have xpcom shutdown midmethod). As far as saving cpu time it's pointless, getservice-type things just don't get called enough for reference counting to make any measurable difference.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

XPCOM can shut down mid-method? I seriously hope not!

If the caller doesn't add a reference, it can't leak.

Comment 33

[(dormant account)]

(In reply to comment #32)
> XPCOM can shut down mid-method? I seriously hope not!

yes if the method initiates a shutdown.

> 
> If the caller doesn't add a reference, it can't leak.

Worried about dereferencing free()ed memory, not leaks. My original idea was to not ref count, but ref counting turned out to be safer.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, but XPCOM can't shut down without spinning a nested event loop, right?

Lots of code holds raw pointers across function calls because we assume/know that the callee can't spin a nested event loop. The same should be true here.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 439796 [details] [diff] [review]
toolkit (observer service, v3)

>diff --git a/toolkit/xre/nsNativeAppSupportUnix.cpp b/toolkit/xre/nsNativeAppSupportUnix.cpp

>@@ -323,24 +323,24 @@ static void OssoHardwareCallback(osso_hw

>   if (state->memory_low_ind) {
>       if (! ourState->memory_low_ind) {
>-      nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1");
>+      nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>       if (os)
>         os->NotifyObservers(nsnull, "memory-pressure", NS_LITERAL_STRING("low-memory").get());
>     }
>   }

Yeesh, what's going on here? Hate to scope creep, but this is so ugly I can't help it - can you make this a single conditional rather than nested if()s, and fix the indent?

Please undo all of the brace removals.

r=me with that.

Comment 36

[Mitchell Field [:Mitch]]

Attachment: toolkit (observer service, v4)

Comment 37

[Mitchell Field [:Mitch]]

Attachment: content (observer service, v2)

Restored some braces.

Comment 38

[Mitchell Field [:Mitch]]

Attachment: docshell (observer service, v2)

Restored some more braces.

Comment 39

[Shawn Wilsher :sdwilsh]

Comment on attachment 439806 [details] [diff] [review]
storage (observer service, v1)

r=sdwilsh

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();

This doesn't need to be an nsCOMPtr.

+    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();

Neither does this. etc...

I guess we can take these patches for now, but at some point --- perhaps when we have infrastructure for annotating/analyzing which methods can spin the event loop --- I would like to remove the nsCOMPtrs.

Comment 41

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

mozilla::services returns already_AddRefed objects, so you do need a comptr.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It shouldn't.

Comment 43

[(dormant account)]

(In reply to comment #41)
> mozilla::services returns already_AddRefed objects, so you do need a comptr.

You can do mozilla::services::GetIOService().get(), so technically you don't.

I think making nsCOMPtr<>s optional is an interesting project, that should be investigated as part of another bug. It would be easier to make that change after giving services explicit getters like this bug is doing.
Ridding services of nsCOMPtrs is not a low-hanging performance fruit, so didn't consider it as important as providing a way to bypass the massive GetService() slowness.
One was to accomplish a comptr-less operation would be to define a wrapper class that lets one dereference services, but not store them.
Ie allow mozilla::services::IOService->...., but not nsIIOService *io = mozilla::services::IOService.

Comment 44

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

(In reply to comment #43)
> You can do mozilla::services::GetIOService().get(), so technically you don't.

Well sure, but then you need to call release manually, so you might as well use a comptr.

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 439800 [details] [diff] [review]
dom (observer service, v1)

Please put back the braces to keep with the bracing stile of these files. (It also makes future patches easier to read)

Looks good otherwise.

Comment 46

[Mitchell Field [:Mitch]]

Attachment: dom (observer service, v2)

Comment 47

[Johnny Stenback (:jst)]

Comment on attachment 440438 [details] [diff] [review]
dom (observer service, v2)

Stealing review from sicking...

Comment 48

[Daniel Veditz [:dveditz]]

Comment on attachment 439810 [details] [diff] [review]
xpinstall (observer service, v1)

r=dvedit

Comment 49

[Mitchell Field [:Mitch]]

Attachment: netwerk (observer service, v3)

TryServer picked up a missing include. Carrying over r+.

Comment 50

[Mitchell Field [:Mitch]]

Attachment: widget (observer service, v2)

Ditto.

Comment 51

[Justin Wood (:Callek)]

Comment on attachment 439845 [details] [diff] [review]
toolkit (observer service, v4)

this patch failed a try-build, nsNativeAppSupportUnix.cpp doesn't have "mozilla/services.h" in scope. The other files in this patch that don't directly include it get it for free from NetUtils.h. I'm not sure if toolkit would want explicit #includes in any file that uses mozilla::services though; so re-pushed the patchset to try without this...

Comment 52

[Justin Wood (:Callek)]

Comment on attachment 439801 [details] [diff] [review]
embedding (observer service, v1)

mozilla::services is not externally linkable, therefore this fails to compile [and link] TestGtkEmbed.cpp please revert in this file.

Comment 53

[Mitchell Field [:Mitch]]

Attachment: embedding (observer service, v2)

Fixed. Thanks, Justin.

Comment 54

[Mitchell Field [:Mitch]]

Attachment: [for-checkin] (observer service)

This amalgamation of all above patches passes TryServer, and should be ready for check-in. Patch includes Hg changeset summary.

Comment 55

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/79cbadb26768