Closed Bug 982491 Opened 6 years ago Closed 6 years ago

Group apps in activities chains in the same process

Categories

(Firefox OS Graveyard :: Runtime, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.3T+
Tracking Status
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.
Add Renfeng and Jesse here.
Flags: needinfo?(renfeng.mei)
Flags: needinfo?(jesse.ji)
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?
(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.
(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)
Agree.
Attached patch gecko patch v1 (obsolete) — Splinter Review
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)
Attached patch gecko patch v2 (obsolete) — Splinter Review
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)
Attached patch gaia patchSplinter Review
v1.3t branch patch to add the "parentapp" attribute on apps iframes when needed.
Attachment #8393665 - Flags: feedback?(alive)
Attachment #8393665 - Flags: feedback?(alive) → feedback+
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+
blocking-b2g: --- → 1.3T?
Whiteboard: [tarako_only]
blocking-b2g: 1.3T? → 1.3T+
should apply fabrice's patch and test around this.
Flags: needinfo?(jhammink)
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)
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 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 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.
(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.
(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.
Attachment #8393663 - Flags: review?(cyu) → review+
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 on attachment 8394629 [details] [review]
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+
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #19)
> Comment on attachment 8394629 [details] [review]
> 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.
(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #18)
> Created attachment 8394629 [details] [review]
> 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)
(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)
(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)
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)
Flags: needinfo?(jhammink)
Flags: needinfo?(fabrice.desre)
b2g-info shows processes with their names.
Flags: needinfo?(fabrice.desre)
We'll start system test tomorrow, can you land this review+ patch today?
Flags: needinfo?(fabrice)
(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)
(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)
(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)
(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".
Whiteboard: [tarako_only] → [tarako_only][priority]
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 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.
Attached patch gecko patch v3Splinter Review
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 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+
We MUST land this patch as soon as possible because our system test is staring now.
Ying, please merge this patch into v1.3t.
Flags: needinfo?(ying.xu)
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+
Ying, land it now.
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
Flags: needinfo?(ttsai)
(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?
Also, I'll land the gecko part.
(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);
(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.
(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.
(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.
Depends on: 988142
(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
(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)
Add mozilla and spreadtrum PM here.
We're waiting for this patch for v1.3t.
Flags: needinfo?(styang)
Flags: needinfo?(jason.liu)
James, feel free to uplift this since its nighttime here in the US where fabrice is located.
Flags: needinfo?(ttsai)
Blocks: 988164
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.
Re-landed with lint error fixed:
https://github.com/mozilla-b2g/gaia/commit/fd72c59d6da4b185a8e7dcf203e327b6456727de
Flags: needinfo?(fabrice)
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.
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?
(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.
https://hg.mozilla.org/mozilla-central/rev/2e5394aaf4b2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
Alive, can you land the gaia patch for master? thanks!
Flags: needinfo?(alive)
Blocks: 988731
No longer blocks: 988164
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 988164
Camera is no function launched by contact app.

--
Keven
Flags: needinfo?(fabrice)
Flags: needinfo?(alive)
If a bug is REOPEN'd, please ask for a backout.
Keywords: checkin-needed
To clarify, Gaia patch can stay in the tree since it does nothing without the Gecko patch.
We can back out by turning off "dom.ipc.reuse_parent_app" and continue rinsing the patch.
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
(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)
(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)
No longer blocks: 988731
Depends on: 988731
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Flags: needinfo?(alive)
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.
Camera video recording can't work even not in group chain. Need to check if this is a regression or the other issue.
(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
(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.
Flags: needinfo?(jesse.ji)
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)
(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.
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
Flags: needinfo?(tlee)
Flags: needinfo?(tchou)
I think we can reserved 4 process.
b2g + nuwa + group homescreen/browser/3rd apps + group other native apps
(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)
Please open new bugs instead of adding comments to fixed ones. Comment 78 looks like bug 991023.
Flags: needinfo?(fabrice)
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)
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.
(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)
Duplicate of this bug: 972773
See Also: → 996141
Flags: needinfo?(styang)
remake[about_RAM_performance]
Flags: sec-review?(ptheriault)
You need to log in before you can comment on or make changes to this bug.