Closed
Bug 996996
Opened 11 years ago
Closed 11 years ago
[ Feature Detection(hardware.memory) ] : Move fopen into non-sanboxed process
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
2.0 S2 (23may)
People
(Reporter: alchen, Assigned: alchen)
References
Details
(Whiteboard: [dependency: marketplace][p=2])
Attachments
(2 files, 5 obsolete files)
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•11 years ago
|
||
Alphan, are you planning to take this on?
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → alchen
Assignee | ||
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
(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)
Assignee | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
(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)
Assignee | ||
Comment 6•11 years ago
|
||
OK, understood.
Thanks for your explaination :)
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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 9•11 years ago
|
||
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+
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 13•11 years ago
|
||
(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)
Comment 14•11 years ago
|
||
(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)
Comment 15•11 years ago
|
||
(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)
Assignee | ||
Comment 16•11 years ago
|
||
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.
Assignee | ||
Comment 17•11 years ago
|
||
In this patch, I reuse "mozilla::hal::GetTotalSystemMemory()" for getting totalmemory.
Attachment #8413574 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
Here is the try server result.
https://tbpl.mozilla.org/?tree=Try&rev=71e90911e10a
Comment 19•11 years ago
|
||
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)
Updated•11 years ago
|
Whiteboard: [dependency: marketplace]
Assignee | ||
Comment 20•11 years ago
|
||
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)
Comment 21•11 years ago
|
||
(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)
Assignee | ||
Comment 22•11 years ago
|
||
Got it.
Thanks for your explaination.
I will try to implement it by your suggestion.
Flags: needinfo?(alchen)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [dependency: marketplace] → [dependency: marketplace][p=2]
Target Milestone: --- → 2.0 S2 (23may)
Assignee | ||
Comment 23•11 years ago
|
||
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 24•11 years ago
|
||
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+
Assignee | ||
Comment 25•11 years ago
|
||
(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.
Assignee | ||
Comment 26•11 years ago
|
||
Here is the try server result.
It looks fine.
https://tbpl.mozilla.org/?tree=Try&rev=682dc9e1e694
Attachment #8423553 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 27•11 years ago
|
||
Only refine the title of attachment 8425971 [details] [diff] [review].
Try server result.
https://tbpl.mozilla.org/?tree=Try&rev=682dc9e1e694
Attachment #8425971 -
Attachment is obsolete: true
Comment 28•11 years ago
|
||
Keywords: checkin-needed
Comment 29•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
![]() |
||
Comment 30•11 years ago
|
||
The ContentChild changes in this changeset are not indented correctly....
Flags: needinfo?(alchen)
Assignee | ||
Comment 31•11 years ago
|
||
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 32•11 years ago
|
||
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 33•11 years ago
|
||
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+
Assignee | ||
Comment 34•11 years ago
|
||
Update the patch again with several fixes mentioned in comment 33.
Attachment #8429753 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 35•11 years ago
|
||
Keywords: checkin-needed
Comment 36•11 years ago
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•