Closed Bug 996996 Opened 10 years ago Closed 10 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
2.0 S2 (23may)

People

(Reporter: alchen, Assigned: alchen)

References

Details

(Whiteboard: [dependency: marketplace][p=2])

Attachments

(2 files, 5 obsolete files)

Move fopen into non-sanboxed process. (async protocol). r=mrbkap
11.11 KB, patch
Details | Diff | Splinter Review
Correct indention in ContentChild.cpp and ContenParent.cpp. r=mrbkap
53.35 KB, patch
Details | Diff | Splinter Review 
We should do the open() in a non-sandboxed process to prevent main-thread IO.
This is a follow up bug for bug 983502.
Blocks: 983502
Alphan, are you planning to take this on?
Assignee: nobody → 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.
Flags: needinfo?(ehsan)
(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.  :-)
Flags: needinfo?(ehsan)
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.
No longer blocks: 983502
Depends on: 983502
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
OK, understood.
Thanks for your explaination :)
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.
Attachment #8413574 - Flags: review?(ehsan)
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.
Attachment #8413574 - Flags: review?(ehsan) → review?(mrbkap)
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.
Attachment #8413574 - Flags: review?(mrbkap) → review+
(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.
(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);
(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.
(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?
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(bent.mozilla)
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.
In this patch, I reuse "mozilla::hal::GetTotalSystemMemory()" for getting totalmemory.
Attachment #8413574 - Attachment is obsolete: true
Here is the try server result.
https://tbpl.mozilla.org/?tree=Try&rev=71e90911e10a
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?
Flags: needinfo?(bent.mozilla) → needinfo?(alchen)
Whiteboard: [dependency: marketplace]
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?
Flags: needinfo?(alchen) → needinfo?(mrbkap)
(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.
Flags: needinfo?(mrbkap) → needinfo?(alchen)
Got it.
Thanks for your explaination.
I will try to implement it by your suggestion.
Flags: needinfo?(alchen)
Whiteboard: [dependency: marketplace] → [dependency: marketplace][p=2]
Target Milestone: --- → 2.0 S2 (23may)
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.
Attachment #8415138 - Attachment is obsolete: true
Attachment #8423553 - Flags: review?(mrbkap)
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.
Attachment #8423553 - Flags: review?(mrbkap) → review+
(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.
Here is the try server result.
It looks fine.

https://tbpl.mozilla.org/?tree=Try&rev=682dc9e1e694
Attachment #8423553 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/afe8ef1b62d1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
The ContentChild changes in this changeset are not indented correctly....
Flags: needinfo?(alchen)
In this patch, I correct some indentation in ContentChild.cpp and ContenParent.cpp.
Please check it.
Thanks.
Attachment #8429753 - Flags: review?(bzbarsky)
Flags: needinfo?(alchen)
Comment on attachment 8429753 [details] [diff] [review]
Correct indention in ContentChild.cpp and ContenParent.cpp

I'm going to punt this to Blake....
Attachment #8429753 - Flags: review?(bzbarsky) → review?(mrbkap)
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.
Attachment #8429753 - Flags: review?(mrbkap) → review+
Update the patch again with several fixes mentioned in comment 33.
Attachment #8429753 - Attachment is obsolete: true
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.