Closed
Bug 982491
Opened 11 years ago
Closed 11 years ago
Group apps in activities chains in the same process
Categories
(Firefox OS Graveyard :: Runtime, defect)
Tracking
(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v2.0 fixed)
People
(Reporter: fabrice, Assigned: fabrice, NeedInfo)
References
Details
(Whiteboard: [tarako_only][priority])
Attachments
(3 files, 2 obsolete files)
For low memory devices we may want to do that. This has security implications though since we're breaking the process sandbox model so we may not enable that for 3rd party apps.
Comment 1•11 years ago
|
||
Add Renfeng and Jesse here.
Flags: needinfo?(renfeng.mei)
Flags: needinfo?(jesse.ji)
Comment 2•11 years ago
|
||
for grouping activities to single process, there's options
1. group severa activities in a another process except b2g/current-app
2. starting in current-app when run an activity
3. start activity in b2g process. (this is almost the same as disable-oop)
is there any option can be reasonable?
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to renfeng.mei from comment #2)
> for grouping activities to single process, there's options
> 1. group severa activities in a another process except b2g/current-app
> 2. starting in current-app when run an activity
> 3. start activity in b2g process. (this is almost the same as disable-oop)
>
> is there any option can be reasonable?
Option 2. is what we need to do.
Comment 4•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #3)
> (In reply to renfeng.mei from comment #2)
> > for grouping activities to single process, there's options
> > 1. group severa activities in a another process except b2g/current-app
> > 2. starting in current-app when run an activity
> > 3. start activity in b2g process. (this is almost the same as disable-oop)
> >
> > is there any option can be reasonable?
>
> Option 2. is what we need to do.
agree with you. hope it goes well.
Flags: needinfo?(renfeng.mei)
Comment 5•11 years ago
|
||
Agree.
Assignee | ||
Comment 6•11 years ago
|
||
This patch adds support for a 'parentapp' attribute on mozapps iframes. If we have a ContentParent for this app, we reuse it and start a second TabParent.
I had to fix the way we check permissions because we were killing the process at the first error when iterating the browsers.
I also had to remove the assertApp() for the 'unregister' messages in SystemMessages because the iframe is already removed when this is called. When we have a single app per process we don't even run this code because the child is dead so we never noticed the issue before.
Cervantes, ignore the default pref value in b2g/app/b2g.js - it's there for testing purposes. If we decide to take this we'll move it to the device specific preferences.
Attachment #8393093 -
Flags: feedback?(cyu)
Assignee | ||
Comment 7•11 years ago
|
||
Updated patch to only let certified apps share processes.
Attachment #8393093 -
Attachment is obsolete: true
Attachment #8393093 -
Flags: feedback?(cyu)
Attachment #8393663 -
Flags: feedback?(cyu)
Assignee | ||
Comment 8•11 years ago
|
||
v1.3t branch patch to add the "parentapp" attribute on apps iframes when needed.
Attachment #8393665 -
Flags: feedback?(alive)
Updated•11 years ago
|
Attachment #8393665 -
Flags: feedback?(alive) → feedback+
Comment 9•11 years ago
|
||
Comment on attachment 8393663 [details] [diff] [review]
gecko patch v2
Review of attachment 8393663 [details] [diff] [review]:
-----------------------------------------------------------------
I am not familiar with the changes in SystemMessageInternal.js and AppProcessChecker.cpp. The other parts looks good.
Attachment #8393663 -
Flags: feedback?(cyu) → feedback+
Updated•11 years ago
|
blocking-b2g: --- → 1.3T?
Whiteboard: [tarako_only]
Updated•11 years ago
|
blocking-b2g: 1.3T? → 1.3T+
Comment 10•11 years ago
|
||
should apply fabrice's patch and test around this.
Updated•11 years ago
|
Flags: needinfo?(jhammink)
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8393665 [details] [diff] [review]
gaia patch
Review of attachment 8393665 [details] [diff] [review]:
-----------------------------------------------------------------
This is the 1.3t version. Alive, do you think that you or someone else could look at porting to master? I'll do it if no-one can but I'm pretty swamped.
Attachment #8393665 -
Flags: review?(alive)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8393663 [details] [diff] [review]
gecko patch v2
Review of attachment 8393663 [details] [diff] [review]:
-----------------------------------------------------------------
Gene, can you review the SystemMessage change and the AppProcessChecker? I explained them in comment 6. thanks!
Attachment #8393663 -
Flags: review?(gene.lian)
Attachment #8393663 -
Flags: review?(cyu)
Comment 13•11 years ago
|
||
Comment on attachment 8393663 [details] [diff] [review]
gecko patch v2
Review of attachment 8393663 [details] [diff] [review]:
-----------------------------------------------------------------
Yeap, I noticed the SystemMessage issue before. If my recall is right, "child-process-shutdown" and "SystemMessageManager:Unregister" would race with each other (under some cases that I kind of forgot). If "child-process-shutdown" comes first, "SystemMessageManager:Unregister" won't work because the target has already died (it just asserts somewhere, but doesn't matter because it has died anyway). However, how about if the "SystemMessageManager:Unregister" comes first without the "child-process-shutdown" coming? Say, open an app first, request an activity on it and close that iframe. Is that a valid case?
If yes, I think we still need to keep the security check for "SystemMessageManager:Unregister" and add some checks within assertContainApp() to see if the target is still alive. If it is dead, don't need to assert for it.
Please correct me if I'm wrong.
Attachment #8393663 -
Flags: review?(gene.lian)
Comment 14•11 years ago
|
||
Comment on attachment 8393663 [details] [diff] [review]
gecko patch v2
Review of attachment 8393663 [details] [diff] [review]:
-----------------------------------------------------------------
::: b2g/app/b2g.js
@@ +692,5 @@
> pref("dom.ipc.processPrelaunch.delayMs", 5000);
> #endif
>
> +pref("dom.ipc.reuse_parent_app", true);
> +
I think we can add this pref to dev-pref.js on my side.
::: dom/ipc/ContentChild.cpp
@@ +393,2 @@
> } else {
> + SetProcessName(NS_LITERAL_STRING("Browser"), false);
I think browser should be in Nuwa process, otherwise it will cause LMK issue.
Comment 15•11 years ago
|
||
(In reply to James Zhang from comment #14)
> Comment on attachment 8393663 [details] [diff] [review]
> gecko patch v2
>
> ::: dom/ipc/ContentChild.cpp
> @@ +393,2 @@
> > } else {
> > + SetProcessName(NS_LITERAL_STRING("Browser"), false);
>
> I think browser should be in Nuwa process, otherwise it will cause LMK issue.
It's only setting the process name. This doesn't change that and just add a new boolean argument.
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to James Zhang from comment #14)
> Comment on attachment 8393663 [details] [diff] [review]
> gecko patch v2
>
> Review of attachment 8393663 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: b2g/app/b2g.js
> @@ +692,5 @@
> > pref("dom.ipc.processPrelaunch.delayMs", 5000);
> > #endif
> >
> > +pref("dom.ipc.reuse_parent_app", true);
> > +
>
> I think we can add this pref to dev-pref.js on my side.
Yes, see the last part of comment 6.
> ::: dom/ipc/ContentChild.cpp
> @@ +393,2 @@
> > } else {
> > + SetProcessName(NS_LITERAL_STRING("Browser"), false);
>
> I think browser should be in Nuwa process, otherwise it will cause LMK issue.
Here "Browser" means that this iframe is a browser tab and not an app. We can't run the browser app itself OOP because we have no support for nested content processes (see bug 761935 and bug 879475), but each tab of content runs in its own process. So the browser app iframe still runs in the parent process - I'll check if we can kill it on memory pressure.
Updated•11 years ago
|
Attachment #8393663 -
Flags: review?(cyu) → review+
Comment 17•11 years ago
|
||
Comment on attachment 8393665 [details] [diff] [review]
gaia patch
Review of attachment 8393665 [details] [diff] [review]:
-----------------------------------------------------------------
I will make the master patch.
Attachment #8393665 -
Flags: review?(alive) → review+
Comment 18•11 years ago
|
||
Attachment #8394629 -
Flags: review?(timdream)
Comment 19•11 years ago
|
||
Comment on attachment 8394629 [details]
gaia patch for master
Do we need a 1.3t specific patch here? Will Gecko patch land on m-c too?
The whiteboard said [tarako_only] but this patch is written against master.
Attachment #8394629 -
Flags: review?(timdream) → review+
Comment 20•11 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #19)
> Comment on attachment 8394629 [details]
> gaia patch for master
>
> Do we need a 1.3t specific patch here? Will Gecko patch land on m-c too?
According to cervantes: yes. But we will pref it off in m-c.
>
> The whiteboard said [tarako_only] but this patch is written against master.
Assignee | ||
Comment 21•11 years ago
|
||
(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #18)
> Created attachment 8394629 [details]
> gaia patch for master
\o/ Thanks Alive!
Just making sure, has this plan been brought to the attention of the security team?
Flags: needinfo?(fabrice)
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #13)
> Comment on attachment 8393663 [details] [diff] [review]
> gecko patch v2
>
> Review of attachment 8393663 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Yeap, I noticed the SystemMessage issue before. If my recall is right,
> "child-process-shutdown" and "SystemMessageManager:Unregister" would race
> with each other (under some cases that I kind of forgot). If
> "child-process-shutdown" comes first, "SystemMessageManager:Unregister"
> won't work because the target has already died (it just asserts somewhere,
> but doesn't matter because it has died anyway). However, how about if the
> "SystemMessageManager:Unregister" comes first without the
> "child-process-shutdown" coming? Say, open an app first, request an activity
> on it and close that iframe. Is that a valid case?
In our case, we close the frame, so the TabParent for the activity provider is gone, but not the process since we still run the parent app in it. And then unregister checks at the process level the manifest url and that fails.
> If yes, I think we still need to keep the security check for
> "SystemMessageManager:Unregister" and add some checks within
> assertContainApp() to see if the target is still alive. If it is dead, don't
> need to assert for it.
That can't work because the target is already gone (ie. we have only one browser to iterate in AssertAppProcess() )
I'm not sure what the best solution here is...
Flags: needinfo?(fabrice) → needinfo?(gene.lian)
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #22)
> Just making sure, has this plan been brought to the attention of the
> security team?
Good catch Ben, even if you may ruin my day.
Paul, the goal here is on memory devices to have an activity provider share the same process as the caller. We limit that to providers that are certified apps, and I will add a check to not run providers in the parent process if the activity is triggered by the system app.
Flags: needinfo?(ptheriault)
Comment 25•11 years ago
|
||
Fabrice - is it possible to get steps to test this (feel free to point me to someone/somewhere else :). Which tools (besides ps -l ) can we use to check processes?
Flags: needinfo?(jhammink)
Updated•11 years ago
|
Flags: needinfo?(jhammink)
Flags: needinfo?(fabrice.desre)
Assignee | ||
Comment 26•11 years ago
|
||
b2g-info shows processes with their names.
Flags: needinfo?(fabrice.desre)
is Bug 972773 a duplicate of this bug?
Comment 28•11 years ago
|
||
We'll start system test tomorrow, can you land this review+ patch today?
Flags: needinfo?(fabrice)
Assignee | ||
Comment 29•11 years ago
|
||
(In reply to James Zhang from comment #28)
> We'll start system test tomorrow, can you land this review+ patch today?
It's not ready yet, there one remaining issue (see Gene's review).
Flags: needinfo?(fabrice)
Comment 30•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #23)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #13)
> > Say, open an app first, request an activity
> > on it and close that iframe. Is that a valid case?
>
> In our case, we close the frame, so the TabParent for the activity provider
> is gone, but not the process since we still run the parent app in it. And
> then unregister checks at the process level the manifest url and that fails.
I'd like to clarify myself here. I mean the following case for example:
1. Open the Contact App first.
2. Open the Messaging App to send an message to someone.
3. Use the Contact App to choose a contact (that's what we use Activity here)
4. When #3 finished, that prompt closes.
In step #4, the iFrame (i.e. the prompt) closes but the Contact process (#1) itself is still alive. In this case, the SystemMessageInternal will receive "SystemMessageManager:Unregister" but not "child-process-shutdown". That's why we still need to do security check on "SystemMessageManager:Unregister" because the target is still alive.
Sorry I didn't run this case myself but I still remember this is a valid case. Does this case sound reasonable to you?
>
> > If yes, I think we still need to keep the security check for
> > "SystemMessageManager:Unregister" and add some checks within
> > assertContainApp() to see if the target is still alive. If it is dead, don't
> > need to assert for it.
>
> That can't work because the target is already gone (ie. we have only one
> browser to iterate in AssertAppProcess() )
>
> I'm not sure what the best solution here is...
I think we can still somehow check if the target is still alive in the _listeners. Say, refactoring the following part in _removeTargetFromListener(...):
_findTargetFromListener: function(aTarget, aManifest) {
let targets = this._listeners[aManifest];
if (!targets) {
return null;
}
let index = this._findTargetIndex(targets, aTarget);
if (index === -1) {
return null;
}
return { targets: targets, index: index }
}
and do:
case "child-process-shutdown":
{
debug("Got child-process-shutdown from " + aMessage.target);
for (let manifest in this._listeners) {
// See if any processes in this manifest have this target.
let result = this._findTargetFromListener(aMessage.target, manifest);
if (!result) {
continue;
}
debug("remove the listener for " + manifest);
delete this._listeners[manifest];
break;
}
break;
}
case "SystemMessageManager:Unregister":
{
debug("Got Unregister from " + aMessage.target + "innerWinID " + msg.innerWindowID);
let result = this._findTargetFromListener(aMessage.target, msg.manifest);
if (!result) {
return;
}
let targets = result.targets;
let index = result.index;
let target = targets[index];
if (aUri && target.winCounts[aUri] !== undefined &&
--target.winCounts[aUri] === 0) {
delete target.winCounts[aUri];
}
if (this._isEmptyObject(target.winCounts)) {
if (targets.length === 1) {
// If it's the only one, get rid of this manifest entirely.
debug("remove the listener for " + aManifest);
delete this._listeners[aManifest];
} else {
// If more than one left, remove this one and leave the rest.
targets.splice(index, 1);
}
}
break;
}
How do you feel?
Flags: needinfo?(gene.lian)
Comment 31•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #24)
> (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #22)
> > Just making sure, has this plan been brought to the attention of the
> > security team?
>
> Good catch Ben, even if you may ruin my day.
Security is your friend! :)
>
> Paul, the goal here is on memory devices to have an activity provider share
> the same process as the caller. We limit that to providers that are
> certified apps, and I will add a check to not run providers in the parent
> process if the activity is triggered by the system app.
It's a bit complex: is the following basically correct?
1. When an activity is fired, gaia system app sets the parentapp attribute on the iframe it creates to hold the activity handler, in case we can re-use an existing parent process
2. When gecko creates the contentParent behind this frame, it checks to see if the activity provider is a certified app. (I don't see the check for system app yet... I assume this is todo...)
3. If so, instead of creating a new process for the activityHandler window, we re-use the process holding the app which initiated the activity.
What are the implications for permissions checks, and data stored in certified apps protected via same-origin? I.E. if I am a malicious web page that has compromised a child process, can't I just invoke a web activity to get the certified app of my choice loaded into my process and access APIs it has permissions for, or data it has access to? Or am I overlooking something here...
(In reply to Fabrice Desré [:fabrice] from comment #29)
> (In reply to James Zhang from comment #28)
> > We'll start system test tomorrow, can you land this review+ patch today?
>
> It's not ready yet, there one remaining issue (see Gene's review).
Also need to add the check to make sure that we don't load other apps in the system app's process? (unless its already there, but I couldn't see this.)
Flags: needinfo?(ptheriault) → sec-review?(ptheriault)
Comment 32•11 years ago
|
||
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #30)
> case "SystemMessageManager:Unregister":
> {
> debug("Got Unregister from " + aMessage.target + "innerWinID " +
> msg.innerWindowID);
> let result = this._findTargetFromListener(aMessage.target, msg.manifest);
> if (!result) {
> return;
> }
And I mean we can call assertContainApp(...) here because the target is still alive.
All these codes are based on the fact that whenever a process goes away, SystemMessageInternal must receive a "child-process-shutdown". Then,
1. If "child-process-shutdown" comes first, "SystemMessageManager:Unregister" won't do the security check because the target is already dead (then it doesn't matter if the process is malicious or not).
2. If "SystemMessageManager:Unregister" comes first, it can still have a chance to call target.assertContainApp(...) because the target is not yet removed by "child-process-shutdown".
Updated•11 years ago
|
Whiteboard: [tarako_only] → [tarako_only][priority]
Assignee | ||
Comment 33•11 years ago
|
||
Gene, it's not a matter of whether Unregister or process-shutdown comes first. In this case we never get process-shutdown because we don't kill the process, but we get Unregister after the TabParent is removed. I'll see if I can track the inner-window-destroyed to help.
Comment 34•11 years ago
|
||
Comment on attachment 8393665 [details] [diff] [review]
gaia patch
Review of attachment 8393665 [details] [diff] [review]:
-----------------------------------------------------------------
::: apps/system/js/app_window_factory.js
@@ +59,4 @@
> }
> config.changeURL = !detail.onlyShowApp;
> config.stayBackground = !detail.showApp;
> + config.parentApp = detail.extra.manifestURL;
When I made the master patch I notice detail.extra may not exist, so you have to check it here.
Assignee | ||
Comment 35•11 years ago
|
||
Update gecko patch:
- only allows certified callers & callee to share a process.
- prevents sharing the parent process when the system app initiates an activity.
Gene, I'll file a followup for the issue we discussed.
Attachment #8393663 -
Attachment is obsolete: true
Attachment #8396832 -
Flags: review?(gene.lian)
Attachment #8396832 -
Flags: review?(cyu)
Comment 36•11 years ago
|
||
Comment on attachment 8396832 [details] [diff] [review]
gecko patch v3
Review of attachment 8396832 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks Fabrice!
::: dom/messages/SystemMessageInternal.js
@@ -341,5 @@
>
> // To prevent the hacked child process from sending commands to parent
> // to manage system messages, we need to check its manifest URL.
> if (["SystemMessageManager:Register",
> - "SystemMessageManager:Unregister",
That would be a bonus if you could put the follow-up bug number around here. Don't really need to remove this line. Just comment it out.
// "SystemMessageManager:Unregister",
// TODO Bug XXX...
Attachment #8396832 -
Flags: review?(gene.lian) → review+
Comment 37•11 years ago
|
||
r=gene btw.
Comment 38•11 years ago
|
||
We MUST land this patch as soon as possible because our system test is staring now.
Comment 40•11 years ago
|
||
Comment on attachment 8396832 [details] [diff] [review]
gecko patch v3
Review of attachment 8396832 [details] [diff] [review]:
-----------------------------------------------------------------
The changes in ContentParent looks good.
Attachment #8396832 -
Flags: review?(cyu) → review+
Comment 41•11 years ago
|
||
Ying, land it now.
Comment 42•11 years ago
|
||
Flags: needinfo?(ying.xu)
Comment 43•11 years ago
|
||
I meet some problems when push gecko patch
Can someone help merge the gecko patch?
ffos@ffos2:~/yingxu/hg/v1.3t$ hg outgoing
comparing with ssh://hg.mozilla.org/releases/mozilla-b2g28_v1_3t
正在搜索修改
修改集: 171321:c0d4a5f9b594
标签: tip
用户: Fabrice Desré <fabrice@mozilla.com>
日期: Wed Mar 26 11:16:04 2014 +0800
摘要: Bug 982491 - Group apps in activities chains in the same process
ffos@ffos2:~/yingxu/hg/v1.3t$ hg push
正在推到 ssh://hg.mozilla.org/releases/mozilla-b2g28_v1_3t
正在搜索修改
remote: 中止: 不能锁定 repository /repo/hg/mozilla/releases/mozilla-b2g28_v1_3t: Permission denied
中止: unexpected response: empty string
Assignee | ||
Comment 44•11 years ago
|
||
(In reply to ying.xu from comment #42)
> https://github.com/mozilla-b2g/gaia/commit/
> 7f2a3a6fa1e372ca49a51139e68e4b2c59c3a7aa
It looks you did not apply the change that Alive asked for in comment 34. Can you push a followup?
Assignee | ||
Comment 45•11 years ago
|
||
Also, I'll land the gecko part.
Comment 46•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #44)
> (In reply to ying.xu from comment #42)
> > https://github.com/mozilla-b2g/gaia/commit/
> > 7f2a3a6fa1e372ca49a51139e68e4b2c59c3a7aa
>
> It looks you did not apply the change that Alive asked for in comment 34.
> Can you push a followup?
You mean change the code like below?
I will fire a new pull request?
But I dont know the new PR should only fix the manifestURL problem or contains all the gaia modifications.
diff --git a/apps/system/js/app_window_factory.js b/apps/system/js/app_window_factory.js
index b064e17..48f4965 100644
--- a/apps/system/js/app_window_factory.js
+++ b/apps/system/js/app_window_factory.js
@@ -59,6 +59,7 @@
}
config.changeURL = !detail.onlyShowApp;
config.stayBackground = !detail.showApp;
+ config.parentApp = detail.manifestURL;
// TODO: Create activity window instance
// or background app window instance for system message here.
this.publish('launchapp', config);
Comment 47•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #45)
> Also, I'll land the gecko part.
Fabrice, our PM and QA are waiting for the first system test build now.
Assignee | ||
Comment 48•11 years ago
|
||
(In reply to ying.xu from comment #46)
> (In reply to Fabrice Desré [:fabrice] from comment #44)
> > (In reply to ying.xu from comment #42)
> > > https://github.com/mozilla-b2g/gaia/commit/
> > > 7f2a3a6fa1e372ca49a51139e68e4b2c59c3a7aa
> >
> > It looks you did not apply the change that Alive asked for in comment 34.
> > Can you push a followup?
>
> You mean change the code like below?
>
> I will fire a new pull request?
> But I dont know the new PR should only fix the manifestURL problem or
> contains all the gaia modifications.
>
> diff --git a/apps/system/js/app_window_factory.js
> b/apps/system/js/app_window_factory.js
> index b064e17..48f4965 100644
> --- a/apps/system/js/app_window_factory.js
> +++ b/apps/system/js/app_window_factory.js
> @@ -59,6 +59,7 @@
> }
> config.changeURL = !detail.onlyShowApp;
> config.stayBackground = !detail.showApp;
> + config.parentApp = detail.manifestURL;
> // TODO: Create activity window instance
> // or background app window instance for system message here.
> this.publish('launchapp', config);
No, you want:
config.parentApp = detail.extra ? detail.extra.manifestURL : null
and the new PR should have just that.
Assignee | ||
Comment 49•11 years ago
|
||
(In reply to James Zhang from comment #47)
> (In reply to Fabrice Desré [:fabrice] from comment #45)
> > Also, I'll land the gecko part.
>
> Fabrice, our PM and QA are waiting for the first system test build now.
I need to land on an integration branch first and will uplift to 1.3t once it's green on m-c. That should happen tomorrow.
Assignee | ||
Comment 50•11 years ago
|
||
Comment 51•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #48)
> No, you want:
> config.parentApp = detail.extra ? detail.extra.manifestURL : null
>
> and the new PR should have just that.
https://github.com/mozilla-b2g/gaia/commit/659491c65a1440df3df7c3cd9acc0bbb87512ce3
Comment 52•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #49)
> (In reply to James Zhang from comment #47)
> > (In reply to Fabrice Desré [:fabrice] from comment #45)
> > > Also, I'll land the gecko part.
> >
> > Fabrice, our PM and QA are waiting for the first system test build now.
>
> I need to land on an integration branch first and will uplift to 1.3t once
> it's green on m-c. That should happen tomorrow.
It'll cause our system test delay one day.
Flags: needinfo?(fabrice)
Comment 53•11 years ago
|
||
Add mozilla and spreadtrum PM here.
We're waiting for this patch for v1.3t.
Flags: needinfo?(styang)
Flags: needinfo?(jason.liu)
Comment 54•11 years ago
|
||
This might also fix this: https://bugzilla.mozilla.org/show_bug.cgi?id=972779#c45
Comment 55•11 years ago
|
||
James, feel free to uplift this since its nighttime here in the US where fabrice is located.
Updated•11 years ago
|
Flags: needinfo?(ttsai)
Comment 56•11 years ago
|
||
Hello, unfortunately this had to be backed out for breaking our 1.3t ci suite on travis. The following lint error exists after landing:
----- FILE : /home/travis/build/mozilla-b2g/gaia/apps/system/js/window_manager.js -----
Line 744, E:0110: Line too long (88 characters).
Line 814, E:0110: Line too long (84 characters).
Found 2 errors, including 0 new errors, in 1 files (1387 files OK).
https://github.com/mozilla-b2g/gaia/commit/f4364484e924f62e845594554a87f15d3570afaf
https://github.com/mozilla-b2g/gaia/commit/3fdf6251e536700d4beacf0a0d43298cb4694c58
As this is only a lint error any previously granted R+ remains. Please update the lint error and re-land.
Assignee | ||
Comment 57•11 years ago
|
||
Re-landed with lint error fixed:
https://github.com/mozilla-b2g/gaia/commit/fd72c59d6da4b185a8e7dcf203e327b6456727de
Flags: needinfo?(fabrice)
Assignee | ||
Comment 58•11 years ago
|
||
And... the linter has bad taste but let's not argue:
https://github.com/mozilla-b2g/gaia/commit/ff50d80c9a5a241bf83037d029bad8ec50e3778e
As a general rule we likely don't want to ever put a certified and a non-certified app in the same process.
Or rather, there's likely a set of permissions that if the app has any of those permissions we don't want it to share process with an app that doesn't have those permissions. So for example SMS and Telephony.
Camera we could possibly be ok with since it's "just" a privacy thing. However camera has it's own issues on the 1.3 branch so unless we backport bug 976398 and any dependencies we probably can't put camera in-process with any other apps.
Assignee | ||
Comment 60•11 years ago
|
||
Jonas, we went with the most conservative approach here: only certified apps can share a process, and not the parent process for cases where the system app is part of the activity. It's also pref'ed off by default everywhere but on 1.3t.
Would it still be ok to have the SMS/dialer apps never share process with any other app? I guess that would make initiating text messages and phone calls from the contacts app require an extra process?
Assignee | ||
Comment 62•11 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #61)
> Would it still be ok to have the SMS/dialer apps never share process with
> any other app? I guess that would make initiating text messages and phone
> calls from the contacts app require an extra process?
Currently dialer and contact are the same app, so they already share the same process. SMS is another app so initiating sms from contact (a pretty major use case) would use 2 processes with your proposal.
We can implement any policy (like comparing permissions) to decide who share processes of course - I thought that limiting sharing to certified apps was safe enough. Please file a follow up if you want to refine that.
Comment 63•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
Updated•11 years ago
|
status-b2g-v1.3T:
--- → ?
Assignee | ||
Comment 64•11 years ago
|
||
Assignee | ||
Comment 65•11 years ago
|
||
Alive, can you land the gaia patch for master? thanks!
Flags: needinfo?(alive)
Comment 66•11 years ago
|
||
status-b2g-v2.0:
--- → fixed
Flags: needinfo?(alive)
Updated•11 years ago
|
Comment 67•11 years ago
|
||
Camera is no function launched by contact app.
--
Keven
Flags: needinfo?(fabrice)
Flags: needinfo?(alive)
Comment 69•11 years ago
|
||
To clarify, Gaia patch can stay in the tree since it does nothing without the Gecko patch.
Comment 70•11 years ago
|
||
We can back out by turning off "dom.ipc.reuse_parent_app" and continue rinsing the patch.
Comment 71•11 years ago
|
||
Hi! Cervantes,
No need to back out. Partner will treat this as known issue and keep testing going.
Partner will fail other cases if we do back out.
--
Keven
Assignee | ||
Comment 72•11 years ago
|
||
(In reply to Keven Kuo [:kkuo] from comment #67)
> Camera is no function launched by contact app.
Ha yes. I know why, my bad. Did you file a follow up? this is what we do in these cases.
Flags: needinfo?(fabrice)
Updated•11 years ago
|
Keywords: checkin-needed
Comment 73•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #72)
> (In reply to Keven Kuo [:kkuo] from comment #67)
> > Camera is no function launched by contact app.
>
> Ha yes. I know why, my bad. Did you file a follow up? this is what we do in
> these cases.
Bug 988731
Flags: needinfo?(styang)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Flags: needinfo?(alive)
Comment 74•11 years ago
|
||
Gallery is not grouped with following steps, not sure is it expeced:
1. Launch Contacts and select anyone
2. Tap message button to launch Messages
3. Add attachment and select from Gallery
# Not only Gallery but also Video/Music/Camera is not grouped at step 3.
Comment 75•11 years ago
|
||
Camera video recording can't work even not in group chain. Need to check if this is a regression or the other issue.
Comment 76•11 years ago
|
||
(In reply to Ting-Yu Chou [:ting] from comment #74)
> Gallery is not grouped with following steps, not sure is it expeced:
>
> 1. Launch Contacts and select anyone
> 2. Tap message button to launch Messages
> 3. Add attachment and select from Gallery
>
> # Not only Gallery but also Video/Music/Camera is not grouped at step 3.
I didn't see this issue on following build.
Gaia ecbd79aab76e119a7987092b59cbafc97f7ba72f
Gecko f23735074dc673457426afa5d87a67f934a18a9b
BuildID 20140328060053
Version 28.1
ro.build.version.incremental=70
ro.build.date=Fri Mar 28 06:17:40 CST 2014
Comment 77•11 years ago
|
||
(In reply to thomas tsai from comment #75)
> Camera video recording can't work even not in group chain. Need to check if
> this is a regression or the other issue.
Yes, it's true that video taking does not work on today's build. It's a regression from yesterday's build. I filed bug 989218.
Comment 78•11 years ago
|
||
Hi Fabrice,
Another 100% reproduce bug. Do we need any patch to fix it?
1. Enter camera->enter gallery->home key to home screen
2. Enter camera->can't enter gallery again
Flags: needinfo?(styang)
Flags: needinfo?(pcheng)
Flags: needinfo?(fabrice)
Comment 79•11 years ago
|
||
(In reply to James Zhang from comment #78)
> Hi Fabrice,
>
> Another 100% reproduce bug. Do we need any patch to fix it?
>
> 1. Enter camera->enter gallery->home key to home screen
> 2. Enter camera->can't enter gallery again
Can we group all native apps to one process.
Comment 80•11 years ago
|
||
1. Native apps, include Homescreen/Camera/Gallery/Settings/Calendar/Clock/Email/Communication/FM/Music/Video apps group to foreground group. And limit the foreground apps number.
2. FM/Music background playback group to system apps.
3. 3rd apps group to 3rd group.
When I have launched 5 apps(homescreen/camera/communication/gallery/setting) with group patch, the 5 apps memory usage is only 25MB. And the free+cache is enough for launch a 3rd party apps.
APPLICATION PID Vss Rss Pss Uss cmdline
b2g 82 38892K 35004K 28765K 25184K /system/b2g/b2g
Homescreen 430 32772K 31620K 25126K 21304K /system/b2g/plugin-container
(Preallocated a 548 7108K 7104K 2601K 600K /system/b2g/plugin-container
(Nuwa) 342 4068K 4068K 1106K 0K /system/b2g/plugin-container
| megabytes |
NAME PID PPID CPU(s) NICE USS PSS RSS VSIZE OOM_ADJ USER
b2g 82 1 45.1 0 29.6 33.1 39.2 137.6 0 root
(Nuwa) 342 82 1.3 0 0.0 1.1 4.0 50.5 0 root
Homescreen 430 342 15.9 18 19.3 23.0 29.4 80.6 8 app_430
(Preallocated a 548 342 1.3 18 0.6 2.5 6.9 57.5 1 root
System memory info:
Total 98.5 MB
Used - cache 70.5 MB
B2G procs (PSS) 59.7 MB
Non-B2G procs 10.7 MB
Free + cache 28.0 MB
Free 7.0 MB
Cache 21.1 MB
Low-memory killer parameters:
notify_trigger 14336 KB
oom_adj min_free
0 1024 KB
1 2048 KB
2 4096 KB
6 8172 KB
8 16384 KB
10 18432 KB
Updated•11 years ago
|
Flags: needinfo?(tlee)
Flags: needinfo?(tchou)
Comment 81•11 years ago
|
||
I think we can reserved 4 process.
b2g + nuwa + group homescreen/browser/3rd apps + group other native apps
Comment 82•11 years ago
|
||
(In reply to James Zhang from comment #80)
> When I have launched 5 apps(homescreen/camera/communication/gallery/setting)
> with group patch, the 5 apps memory usage is only 25MB.
James, could you please let us know how did you get the number (25MB)? I couldn't find it from the b2g-info you pasted.
Flags: needinfo?(tchou)
Assignee | ||
Comment 83•11 years ago
|
||
Please open new bugs instead of adding comments to fixed ones. Comment 78 looks like bug 991023.
Flags: needinfo?(fabrice)
Comment 84•11 years ago
|
||
Fabrice, could we give BSP/gonk a chance to set how to merge app processes?
It looks very very depending on vendor's favorite.
Flags: needinfo?(tlee)
Comment 85•11 years ago
|
||
Settings is very rare to be used except to set networks. Maybe we should move network configuration to a separated app.
I guess a giant grouped process would be killed very often. We should not look at just static memory info, but also check how their interaction. So, please do more tests, and list cons and pros, then we could judge the solution in objective.
Comment 86•11 years ago
|
||
(In reply to James Zhang from comment #78)
> Hi Fabrice,
>
> Another 100% reproduce bug. Do we need any patch to fix it?
>
> 1. Enter camera->enter gallery->home key to home screen
> 2. Enter camera->can't enter gallery again
This behavior has the same root cause with bug 991023
Flags: needinfo?(pcheng)
Updated•11 years ago
|
Updated•11 years ago
|
Flags: needinfo?(styang)
Comment 88•10 years ago
|
||
remake[about_RAM_performance]
Updated•9 years ago
|
Flags: sec-review?(ptheriault)
You need to log in
before you can comment on or make changes to this bug.
Description
•