1154877 - Content process shouldn't use the Places Database

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Experimenting with e10s, it looks like we instantiate the Places Database from the content process. That's not expected, and I'm not even sure that it is safe to use the same database from two processes.

To reproduce the bug, add an assertion
 MOZ_ASSERT(XRE_GetProcessType() != GeckoProcessType_Content);
in Database::Database (toolkit/components/places/Database.cpp).

Comment 1

[Marco Bonardo [:mak]]

do you have a stack?

Comment 2

[Marco Bonardo [:mak]]

fwiw, it's safe to use the db from multiple processes. But we shouldn't really.

Comment 3

[Marco Bonardo [:mak]]

09:33:30 INFO - #01: mozilla::places::Database::GetSingleton() [xpcom/base/nsRefPtr.h:92]
09:33:30 INFO - #02: nsFaviconService::Init() [mfbt/AlreadyAddRefed.h:109]
09:33:30 INFO - #03: nsFaviconService::GetSingleton() [toolkit/components/places/nsFaviconService.cpp:70]
09:33:30 INFO - #04: nsFaviconServiceConstructor [mfbt/AlreadyAddRefed.h:109]
09:33:30 INFO - #05: nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) [xpcom/components/nsComponentManager.cpp:1232]
09:33:30 INFO - #06: nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) [xpcom/components/nsComponentManager.cpp:1593]
09:33:30 INFO - #07: nsGetServiceByContractID::operator()(nsID const&, void**) const [xpcom/glue/nsComponentManagerUtils.cpp:281]
09:33:30 INFO - #08: nsCOMPtr<mozIAsyncFavicons>::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) [xpcom/glue/nsCOMPtr.h:1085]
09:33:30 INFO - #09: CopyFavicon [docshell/base/nsDocShell.cpp:9445]
09:33:30 INFO - #10: nsDocShell::InternalLoad(nsIURI*, nsIURI*, unsigned int, nsISupports*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) [docshell/base/nsDocShell.cpp:10162]
09:33:30 INFO - #11: nsDocShell::OnLinkClickSync(nsIContent*, nsIURI*, char16_t const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, nsIDocShell**, nsIRequest**) [xpcom/string/nsString.h:55]
09:33:30 INFO - #12: OnLinkClickEvent::Run() [docshell/base/nsDocShell.cpp:13318]

Comment 4

[Marco Bonardo [:mak]]

It's CopyFavicon, it should not directly instanciate the favicon service, it should instead send an async message and let the main process do that.

And once that is fixed, we should put a big assert in Database, as you did, so nobody tries again to init Places in the wrong process... this bug has both performance and memory implications, plus it might do fancy things at shutdown.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

I can confirm the fancy things at shutdown.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Bob, you might want to be aware of this for sandboxing.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1

sort-of the patch, didn't clean it up yet.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f6d0b3920be

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Bill, could you please review this or suggest someone who could?

Comment 9

[Marco Bonardo [:mak]]

the new try https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c93f2da0b8e

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Added a message to moz_assert.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8597598 [details] [diff] [review]
patch v1.2

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

I don't like how you're duplicating the code from nsDocShell. Can you move that code to somewhere in Places and have both clients call it?

::: toolkit/components/places/Database.cpp
@@ +28,5 @@
>  #include "mozilla/Services.h"
>  #include "prtime.h"
>  
> +#include "mozilla/dom/ContentChild.h"
> +#include "mozilla/dom/ContentParent.h"

Not sure what you need these for. nsXULAppAPI.h has the XRE_GetProcessType() stuff.

Comment 12

[Marco Bonardo [:mak]]

(In reply to Bill McCloskey (:billm) from comment #11)
> Comment on attachment 8597598 [details] [diff] [review]
> patch v1.2
> 
> Review of attachment 8597598 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't like how you're duplicating the code from nsDocShell. Can you move
> that code to somewhere in Places and have both clients call it?

This is just docshell code using Places, it's not Places internals. I don't know if there's a shared file for the docshell, indeed most of the Send/Recv methods duplicate code already :/

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Generally we try to have the Recv methods call into the same function that did the Send call in the child process.

Could you add CopyFavicon to AsyncFaviconHelpers.cpp/h? If that doesn't sit well with you, maybe we could make it a static method on nsDocShell.

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.3

This solves the code duplication using a static method.

I didn't move this to Places for 3 reasons:
1. As I said, it's consumer code, not internal, I usually don't like to mixup unless the utils are used by more than one consumer. We also prefer to not expose APIs for things that can be done in a few lines of code by using the existing ones.
2. Places doesn't currently export any public header, apart from interfaces (and we are moving more and more to js so we try to expose as few as possible to cpp)
3. CopyFavicon can be implemented by other favicons implementations, for example by Firefox for Android or B2G, the Places part is only one possible implementation (The current position of the ifdef in this patch should clarify that), so at a maximum I could move the callback implementation but not CopyFavicon

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8598465 [details] [diff] [review]
patch v1.3

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

Thanks for fixing this.

::: docshell/base/nsDocShell.cpp
@@ +9460,5 @@
> +nsDocShell::CopyFavicon(nsIURI* aOldURI,
> +                        nsIURI* aNewURI,
> +                        bool aInPrivateBrowsing)
> +{
> +

Looks like an extra newline snuck in here.

@@ +9484,3 @@
>    }
>  #endif
> +  return false;

This will cause the content process to crash if MOZ_PLACES is false (since returning false from any IPC handler kills the content process). I don't think we want to do that. Is it really necessary to return anything from this function? Not copying the favicon isn't the end of the world.

::: dom/ipc/ContentParent.cpp
@@ +4549,5 @@
>  
>  bool
> +ContentParent::RecvCopyFavicon(const URIParams& aOldURI,
> +                               const URIParams& aNewURI,
> +                               const bool& aInPrivateBrowsing) {

Brace on its own line.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Bill McCloskey (:billm) from comment #15)
> @@ +9484,3 @@
> >    }
> >  #endif
> > +  return false;
> 
> This will cause the content process to crash if MOZ_PLACES is false (since
> returning false from any IPC handler kills the content process).

I didn't know, thanks for pointing that out.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/46f29b35aefe

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46f29b35aefe

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Changes from this patch appear to be related to B2G desktop and Mulet build bustage on a try push I did recently, see https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbfa5f340d13 - not sure what's going on there...

Comment 20

[Ryan VanderMeulen [:RyanVM]]

It's happening in production trees too. Some sort of Taskcluster issue I guess.

Comment 21

[Marco Bonardo [:mak]]

b2g doesn't use Places and it's not defining MOZ_PLACES so most of the code is not even built, there's only some boilerplate, I can't see a relation...

Comment 22

[Marco Bonardo [:mak]]

ah I see
: 'virtual bool mozilla::dom::ContentParent::RecvCopyFavicon(const URIParams&, const URIParams&, const bool&)' marked override, but does not override

Maybe it needs some sort of clobber?

Comment 23

[Kartikaya Gupta (email:[e-mail])]

I filed bug 1160508 for it, it looks like taskcluster is doing a bad incremental build maybe.