Content process shouldn't use the Places Database

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Yoric, Assigned: mak)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox40 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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).
Summary: Component process uses Database → Component process shouldn't use the Places Database
Summary: Component process shouldn't use the Places Database → Content process shouldn't use the Places Database
(Assignee)

Comment 1

4 years ago
do you have a stack?
(Assignee)

Comment 2

4 years ago
fwiw, it's safe to use the db from multiple processes. But we shouldn't really.
(Assignee)

Comment 3

4 years ago
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]
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Updated

4 years ago
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
OS: Mac OS X → All
Hardware: x86 → All
I can confirm the fancy things at shutdown.
Bob, you might want to be aware of this for sandboxing.
Blocks: 1063169
tracking-e10s: ? → +
(Assignee)

Comment 8

4 years ago
Created attachment 8597596 [details] [diff] [review]
patch v1.1

Bill, could you please review this or suggest someone who could?
Assignee: nobody → mak77
Attachment #8597269 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8597596 - Flags: review?(wmccloskey)
(Assignee)

Comment 10

4 years ago
Created attachment 8597598 [details] [diff] [review]
patch v1.2

Added a message to moz_assert.
Attachment #8597596 - Attachment is obsolete: true
Attachment #8597596 - Flags: review?(wmccloskey)
Attachment #8597598 - Flags: review?(wmccloskey)
Iteration: --- → 40.2 - 27 Apr
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.
Attachment #8597598 - Flags: review?(wmccloskey)
(Assignee)

Comment 12

4 years ago
(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 :/
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.
(Assignee)

Comment 14

4 years ago
Created attachment 8598465 [details] [diff] [review]
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
Attachment #8597598 - Attachment is obsolete: true
Attachment #8598465 - Flags: review?(wmccloskey)
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
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.
Attachment #8598465 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 16

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/46f29b35aefe
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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...
Flags: needinfo?(mak77)
It's happening in production trees too. Some sort of Taskcluster issue I guess.
(Assignee)

Comment 21

4 years ago
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...
Flags: needinfo?(mak77)
(Assignee)

Comment 22

4 years ago
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?
I filed bug 1160508 for it, it looks like taskcluster is doing a bad incremental build maybe.
You need to log in before you can comment on or make changes to this bug.