996996 - [ Feature Detection(hardware.memory) ] : Move fopen into non-sanboxed process

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Alphan Chen [:alchen]]

We should do the open() in a non-sandboxed process to prevent main-thread IO.
This is a follow up bug for bug 983502.

Comment 1

[(no longer active)]

Alphan, are you planning to take this on?

Comment 2

[Alphan Chen [:alchen]]

Hi Ehsan,
I should work on this task based on bug 983502, am I right?
In my opinion, Bug 996996 should depend on Bug 983502 not block it.

Comment 3

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #2)
> Hi Ehsan,
> I should work on this task based on bug 983502, am I right?

That would be great, yes!

> In my opinion, Bug 996996 should depend on Bug 983502 not block it.

Well, this bug fixes an implementation issue introduced in bug 983502, and I think we usually encode this relationship between bugs this way.  But if you feel strongly feel free to change it, I don't care much as long as the work happens here.  :-)

Comment 4

[Alphan Chen [:alchen]]

Hi Ehsan,
Do we need to rewrite this part?
I spend some time thinking about this work today, I think it might be over-engineering to do it.

List the reasons below:
1. As you said in Bug 983502 comment 61, the behavior of "fopen(/proc/meminfo)" is not a real IO and it is not blocking.
2. We only do it once.
3. The implementation won't be straight forward. Since the promise must be resolved or reject in main thread, we may need to dispatch fopen task into other thread first. After we retrieved the value, we need to dispatch the task of resolve or reject promise into main thread again.

Comment 5

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #4)
> Hi Ehsan,
> Do we need to rewrite this part?
> I spend some time thinking about this work today, I think it might be
> over-engineering to do it.
> 
> List the reasons below:
> 1. As you said in Bug 983502 comment 61, the behavior of
> "fopen(/proc/meminfo)" is not a real IO and it is not blocking.
> 2. We only do it once.

The issue is that with seccomp enabled, you won't be able to invoke the open syscall at all from the content process, it doesn't matter which file you're trying to open.  This is not a question of performance.  I think the simplest thing to do here is to add a new method to PContent.ipdl to send the request to the parent process, do all of the work there (opening the file, reading it and parsing the value out of it) and send the result back to the child process, and then resolve/reject the promise based on what the parent process returns.

CCing bent, billm and mrbkap who can tell you how the IPDL bits should work.  I'm not super familiar with it.

> 3. The implementation won't be straight forward. Since the promise must be
> resolved or reject in main thread, we may need to dispatch fopen task into
> other thread first. After we retrieved the value, we need to dispatch the
> task of resolve or reject promise into main thread again.

I'm not sure which two threads you're talking about here, the two threads involved are both the main threads one of the child and the other of the parent.

Comment 6

[Alphan Chen [:alchen]]

OK, understood.
Thanks for your explaination :)

Comment 7

[Alphan Chen [:alchen]]

Attachment: (0428) Move fopen into non-sanboxed process

In this patch, I add a new method to PContent.ipdl to do fopen in ContenParent.
Besides, I revise one part based on BUG 996474.
Please have a look.
Thanks.

Comment 8

[(no longer active)]

Comment on attachment 8413574 [details] [diff] [review]
(0428) Move fopen into non-sanboxed process

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

I think this is fine, but Blake, can you please check for IPDL stuff?  Thanks!

::: dom/base/Navigator.cpp
@@ +1443,5 @@
>  
>  #if defined(XP_LINUX)
>    if (aName.EqualsLiteral("hardware.memory")) {
> +
> +  static int memLevel = 1;

Nit: please fix the indentation in this code.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8413574 [details] [diff] [review]
(0428) Move fopen into non-sanboxed process

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

The IPC stuff looks OK, but I wonder if you might be able to share the implementation of RecvGetMemoryTotal instead of having the fopen/fscanf duplicated in two places?

Please file a followup bug on making the new IPC message an asynchronous message. We have this great async API, and we reward ourselves by blocking the content process anyway. We should avoid doing that.

Comment 10

[(no longer active)]

(In reply to comment #9)
> Comment on attachment 8413574 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8413574
> (0428) Move fopen into non-sanboxed process
> 
> Review of attachment 8413574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The IPC stuff looks OK, but I wonder if you might be able to share the
> implementation of RecvGetMemoryTotal instead of having the fopen/fscanf
> duplicated in two places?

Yes, good point.  Can you do this please, Alphan?

> Please file a followup bug on making the new IPC message an asynchronous
> message. We have this great async API, and we reward ourselves by blocking the
> content process anyway. We should avoid doing that.

Is that too much work?  If not, I'd prefer to do that here before landing the patch.

Comment 11

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #10)
> (In reply to comment #9)
> > Comment on attachment 8413574 [details] [diff] [review]
> >   --> https://bugzilla.mozilla.org/attachment.cgi?id=8413574
> > (0428) Move fopen into non-sanboxed process
> > 
> > Review of attachment 8413574 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The IPC stuff looks OK, but I wonder if you might be able to share the
> > implementation of RecvGetMemoryTotal instead of having the fopen/fscanf
> > duplicated in two places?
> 
> Yes, good point.  Can you do this please, Alphan?
Okay, I will do it.
> 
> > Please file a followup bug on making the new IPC message an asynchronous
> > message. We have this great async API, and we reward ourselves by blocking the
> > content process anyway. We should avoid doing that.
> 
> Is that too much work?  If not, I'd prefer to do that here before landing
> the patch.

I can replace sync to async.
The reason why I don't use async is "fopen("/proc/meminfo", "r")" should not be a blocking behavior.
But I can do so if reviewer think it would be better.

@dom/ipc/PContent.ipdl
+    sync GetMemoryTotal()
+        returns (uint32_t memTotal);

Comment 12

[(no longer active)]

(In reply to comment #11)
> > > Please file a followup bug on making the new IPC message an asynchronous
> > > message. We have this great async API, and we reward ourselves by blocking the
> > > content process anyway. We should avoid doing that.
> > 
> > Is that too much work?  If not, I'd prefer to do that here before landing
> > the patch.
> 
> I can replace sync to async.
> The reason why I don't use async is "fopen("/proc/meminfo", "r")" should not be
> a blocking behavior.
> But I can do so if reviewer think it would be better.

The issue here is blocking the *content process* on the parent process while the IPC delivery and responding to the message in the parent process happens.

Comment 13

[Alphan Chen [:alchen]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #12)
> (In reply to comment #11)
> > > > Please file a followup bug on making the new IPC message an asynchronous
> > > > message. We have this great async API, and we reward ourselves by blocking the
> > > > content process anyway. We should avoid doing that.
> > > 
> > > Is that too much work?  If not, I'd prefer to do that here before landing
> > > the patch.
> > 
> > I can replace sync to async.
> > The reason why I don't use async is "fopen("/proc/meminfo", "r")" should not be
> > a blocking behavior.
> > But I can do so if reviewer think it would be better.
> 
> The issue here is blocking the *content process* on the parent process while
> the IPC delivery and responding to the message in the parent process happens.

I know what will happen when using sync.
But I think we can use sync in this case.
If we cannot use sync when doing a non-blocking behavior, when can we use sync call?

Comment 14

[(no longer active)]

(In reply to Alphan Chen[:Alphan] from comment #13)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #12)
> > (In reply to comment #11)
> > > > > Please file a followup bug on making the new IPC message an asynchronous
> > > > > message. We have this great async API, and we reward ourselves by blocking the
> > > > > content process anyway. We should avoid doing that.
> > > > 
> > > > Is that too much work?  If not, I'd prefer to do that here before landing
> > > > the patch.
> > > 
> > > I can replace sync to async.
> > > The reason why I don't use async is "fopen("/proc/meminfo", "r")" should not be
> > > a blocking behavior.
> > > But I can do so if reviewer think it would be better.
> > 
> > The issue here is blocking the *content process* on the parent process while
> > the IPC delivery and responding to the message in the parent process happens.
> 
> I know what will happen when using sync.
> But I think we can use sync in this case.
> If we cannot use sync when doing a non-blocking behavior, when can we use
> sync call?

Good question!  Sometimes the content process doesn't really have any useful work to do before the response to its request arrives.  For example, if you look at NotifyAlertObserver <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2084>, here the content process cannot proceed to do other things because the alert observers in the parent process must be called before the alert() function in the child returns.

But in this case we have a method that is designed to be asynchronous (since it returns a promise), so we don't need to block the content process on the child process.  We can just send the request to the parent, get back to our event loop and do more work until the parent gets back to us and we'll resolve the promise then, which makes things a bit more efficient when you consider the fact that the event loop of the child process may be very busy waiting to do other work.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #10)
> Is that too much work?  If not, I'd prefer to do that here before landing
> the patch.

The amount of code to make an async getter out of a sync getter that returns a simple integer value) would be far more than that for the sync getter. Alphan would probably have to make a new IPDL actor and get some memory management details right in order to deal with the state as well as dealing with failure. I wouldn't hold up this bug (which follows existing conventions to boot) on that work.

Ben, let me know if I'm wrong. If not, do you know if we have a bug on file to provide such an easy way to do this?

Comment 16

[Alphan Chen [:alchen]]

In this situation, I think we can have a patch with sync version in this bug and file another bug for async version. I will renew the patch once I finish the test of try server.

Comment 17

[Alphan Chen [:alchen]]

Attachment: (0430) Move fopen into non-sanboxed process. r=mrbkap

In this patch, I reuse "mozilla::hal::GetTotalSystemMemory()" for getting totalmemory.

Comment 18

[Alphan Chen [:alchen]]

Here is the try server result.
https://tbpl.mozilla.org/?tree=Try&rev=71e90911e10a

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

I spoke with bent about this. He feels a little more strongly about sync vs async messages than I do.

There's actually a neat hack that we can use:

We split the (synchronous) message into two:

protocol PContent {
child:
  async SystemMemoryAvailable(uint64_t getterId, uint32_t memoryAvailable);

parent:
  async GetSystemMemory(uint64_t getterId);
};

The hack is that the uint64_t is actually the pointer (in the child process) to the promise created by the DOM API.

So we'll end up:

already_AddRefed<Promise>
Navigator::GetFeature(...)
{
  // ...
  nsRefPtr<Promise> promise = ...;

  nsRefPtr<Promise> ipcRef(promise); // released in ContentChild::RecvSystemMemoryAvailable.
  contentChild->SendGetSystemMemory(reinterpret_cast<uint64_t>(ipcRef.forget().take());
  // ..
  return promise.forget();
}

bool
ContentChild::RecvSystemMemoryAvailable(uint64_t aGetterId, uint32_t aMemoryAvailable)
{
  nsRefPtr<Promise> p = dont_AddRef(reinterpret_cast<Promise*>(aGetterId));
  // Use aMemoryAvailable to resolve the promise.
}

Alphan, does that make sense?

Comment 20

[Alphan Chen [:alchen]]

Hi Blake,
I need to confirm the purpose of "split the message into two".
In my understanding,
1. In GetFeature(...), we use SendGetSystemMemory(...) for getting the amount of TotalMemory in ContentParent process.
2. In RecvGetSystemMemory(...), we use SendSystemMemoryAvailable(...) for resolving the promise in ContentChild process.

So the main reason of "split the message into two rather than one" is keep "resolve the promise" in ContentChild process, am I right?

By the way, is there any concern to use one async message or resolve the promise in ContentParent?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Alphan Chen[:Alphan] from comment #20)
> So the main reason of "split the message into two rather than one" is keep
> "resolve the promise" in ContentChild process, am I right?

Well, you're close, but not exactly right. The problem is that the separation of work is:

Child: Asks for the information but can't calculate it. Doesn't want to wait while the parent calculates it.
Parent: Calculates the information but can't touch the DOM (because of the process boundary).

My proposed setup is very similar to your original implementation, actually. The only difference is that instead of the child waiting for the parent to calculate the total memory available, it asks and then goes to do other stuff (run timeouts, process user input, etc.) then when the parent notifies it with the information it notifies the web app.

> By the way, is there any concern to use one async message or resolve the
> promise in ContentParent?

ContentParent can't resolve the promise because the promise only exists in the child process. I'm using the promise's address as a unique identifier for this request, but we could have just as easily used a map from UUID (randomly generated) to promise in the child and passed the UUID around. That's more work, code, and memory use, though, so we take this shortcut.

Comment 22

[Alphan Chen [:alchen]]

Got it.
Thanks for your explaination.
I will try to implement it by your suggestion.

Comment 23

[Alphan Chen [:alchen]]

Attachment: (0515) Move fopen into non-sanboxed process. (async protocol)

In this patch, I use two async protocol as comment 19 and comment 21 mentioned.
child:
async SystemMemoryAvailable(uint64_t getterId, uint32_t memoryAvailable);
parent:
async GetSystemMemory(uint64_t getterId);

Besides, I add "GetTotalSystemMemoryLevel()" for the level of total memory calculation.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8423553 [details] [diff] [review]
(0515) Move fopen into non-sanboxed process.  (async protocol)

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

r=mrbkap with my comments addressed.

::: dom/base/Navigator.cpp
@@ +1495,4 @@
>          p->MaybeReject(NS_LITERAL_STRING("Abnormal"));
>          return p.forget();
>        }
> +      p->MaybeResolve((int)memLevel);

Why the explicit cast to int?

@@ +1506,2 @@
>    } // hardware.memory
>    else

Nit: else-after-if: you can get rid of the else.

::: dom/ipc/ContentChild.cpp
@@ +1449,5 @@
> +    p->MaybeReject(NS_LITERAL_STRING("Abnormal"));
> +    return true;
> +  }
> +
> +  p->MaybeResolve((int)aMemoryAvailable);

Again, why the cast? Please get rid of them if you can.

::: dom/ipc/ContentParent.cpp
@@ +3024,5 @@
> +#if defined(XP_LINUX)
> +  memoryTotal = mozilla::hal::GetTotalSystemMemoryLevel();
> +#endif
> +
> +  unused << SendSystemMemoryAvailable(aGetterId,memoryTotal);

Nit: space after the comma.

Comment 25

[Alphan Chen [:alchen]]

(In reply to Blake Kaplan (:mrbkap) from comment #24)
> Comment on attachment 8423553 [details] [diff] [review]
> (0515) Move fopen into non-sanboxed process.  (async protocol)
> 
> Review of attachment 8423553 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=mrbkap with my comments addressed.
> 
> ::: dom/base/Navigator.cpp
> @@ +1495,4 @@
> >          p->MaybeReject(NS_LITERAL_STRING("Abnormal"));
> >          return p.forget();
> >        }
> > +      p->MaybeResolve((int)memLevel);
> 
> Why the explicit cast to int?

> 
> ::: dom/ipc/ContentChild.cpp
> @@ +1449,5 @@
> > +    p->MaybeReject(NS_LITERAL_STRING("Abnormal"));
> > +    return true;
> > +  }
> > +
> > +  p->MaybeResolve((int)aMemoryAvailable);
> 
> Again, why the cast? Please get rid of them if you can.
> 

There is no uint32 support now.
Here is the comment from /dom/bindings/ToJSValue.h.
// The uint32_t version is disabled for now because on the super-old b2g
// compiler nsresult and uint32_t are the same type.

Comment 26

[Alphan Chen [:alchen]]

Attachment: Move fopen into non-sanboxed process. (async protocol). r=mrbkap

Here is the try server result.
It looks fine.

https://tbpl.mozilla.org/?tree=Try&rev=682dc9e1e694

Comment 27

[Alphan Chen [:alchen]]

Attachment: Move fopen into non-sanboxed process. (async protocol). r=mrbkap

Only refine the title of attachment 8425971 [details] [diff] [review].

Try server result.
https://tbpl.mozilla.org/?tree=Try&rev=682dc9e1e694

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afe8ef1b62d1

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afe8ef1b62d1

Comment 30

[Boris Zbarsky [:bzbarsky]]

The ContentChild changes in this changeset are not indented correctly....

Comment 31

[Alphan Chen [:alchen]]

Attachment: Correct indention in ContentChild.cpp and ContenParent.cpp

In this patch, I correct some indentation in ContentChild.cpp and ContenParent.cpp.
Please check it.
Thanks.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8429753 [details] [diff] [review]
Correct indention in ContentChild.cpp and ContenParent.cpp

I'm going to punt this to Blake....

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8429753 [details] [diff] [review]
Correct indention in ContentChild.cpp and ContenParent.cpp

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

A general comment.

* There's some trailing whitespace here and there floating around this patch, please nuke it.

::: dom/ipc/ContentParent.cpp
@@ +3264,5 @@
>  {
> +    if (mGeolocationWatchID != -1) {
> +        nsCOMPtr<nsIDOMGeoGeolocation> geo = do_GetService("@mozilla.org/geolocation;1");
> +        if (!geo) {
> +        return true;

Nit: The return statement didn't move as it should have.

Comment 34

[Alphan Chen [:alchen]]

Attachment: Correct indention in ContentChild.cpp and ContenParent.cpp. r=mrbkap

Update the patch again with several fixes mentioned in comment 33.

Comment 35

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/69ec66f1af97

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69ec66f1af97