Last Comment Bug 762802 - Use mozapp/mozbrowser information when allocating remote iframes to content processes
: Use mozapp/mozbrowser information when allocating remote iframes to content p...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: browser-api b2g-e10s-work 768832
  Show dependency treegraph
 
Reported: 2012-06-08 00:46 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-09-19 13:52 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Part 1: IPC changes. (9.87 KB, patch)
2012-07-11 09:05 PDT, Justin Lebar (not reading bugmail)
cjones.bugs: review+
Details | Diff | Splinter Review
Part 2: Content/DOM changes. (8.64 KB, patch)
2012-07-11 09:06 PDT, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 00:46:46 PDT
For now, let's assume we have 1 content process allocated to "browser tabs", and ∞ allocated to "web apps".  Then the way we should allocate processes is
 - if it's "mozapp", launch a new process
 - else, if it's "mozbrowser", launch the "browser tabs" process if it doesn't exist, and put the frame in the "browser tabs" process

(We have to check in this order because current "mozapp" iframes are also "mozbrowser", as an implementation detail.)

We want one OS process per mozapp process for a variety of reasons around security and responsiveness.  In Gonk, we can get away with this willy-nilly allocation because we can rely on the kernel lowmemkiller to gun down background processes if memory gets tight.

We can allocate "browser tabs" however we want, but we don't have much reason to do something different yet.
Comment 1 Justin Lebar (not reading bugmail) 2012-06-08 14:28:14 PDT
> In Gonk, we can get away with this willy-nilly allocation because we can rely on the kernel 
> lowmemkiller to gun down background processes if memory gets tight.

Do we have any evidence that the oomkiller will kill the process we want in this case?

AIUI if we launch a new process which eats up a lot of RAM, the oomkiller will shoot *it*.  That's precisely not the behavior we want.

http://linux-mm.org/OOM_Killer

Maybe if we immunized important processes from being killed (/proc/<pid>/oomadj set to OOM_DISABLE), we could get away with this as described here.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 14:29:30 PDT
You're thinking of the linux OOM killer.

I'm talking about the android lowmemkiller driver, which we can program to suit our needs.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 14:31:50 PDT
Before you ask,

https://bugzilla.mozilla.org/show_bug.cgi?id=744515#c4

;)
Comment 4 Sotaro Ikeda [:sotaro] 2012-06-27 18:54:24 PDT
how to control lowmemkiller is different between GB and ICS. ICS code's comment could help to understand how to use it.
- until GB: lowmemkiller setting code is with in init.rc. [1]
- ICS     : the setting code is within ProcessList.java [2]

[1] http://androidxref.com/2.3.6/xref/system/core/rootdir/init.rc#212

[2] http://androidxref.com/4.0.4/xref/frameworks/base/services/java/com/android/server/am/ProcessList.java
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-06 23:23:42 PDT
jlebar, you're not working on this, right?
Comment 6 Justin Lebar (not reading bugmail) 2012-07-07 09:32:59 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> jlebar, you're not working on this, right?

I haven't looked at this bug, but I can.  It's simple to do, but it seems that there's actually some heuristics involved here.  In particular, we /could/ have more than one process for our browser tabs, so long as all tabs that can touch each other are in the same process.  That's how things work at the moment (in theory!).

Anyway, it's not clear to me that the correct way to tune this to put all browser tabs in the same process, so we could wait to tune this until we have more things running OOP and we have a better idea of the trade-offs involved in having lots of content processes.
Comment 7 Justin Lebar (not reading bugmail) 2012-07-07 09:34:03 PDT
This isn't strictly a browser-api issue, but marking it as blocking that bug so I don't lose track of it.  Anyway the main change will be for specifically browser tabs.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-07 12:36:49 PDT
The critically important part of this work is process-per-mozapp.  To a first approximation, at the moment, I don't care how we allocate processes to mozbrowser-!mozapp.  One process for all of them is fine by me.
Comment 9 Justin Lebar (not reading bugmail) 2012-07-07 12:42:32 PDT
> The critically important part of this work is process-per-mozapp.

I haven't been working on that, modulo what's already done.  Is there more work to do there?  I guess right now we have some arbitrary process limit and allocate randomly within it.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-07 12:45:11 PDT
Yes, in nsFrameLoader, when we ask for a process, we need to know whether the process is going to be used for a mozapp or !mozapp frame.  If mozapp, we need to force ContentParent to give us a freshly allocated process.

We can keep the current arbitrary limit alive for !mozapp frames, but like I said I don't care about that too much right now.
Comment 11 Mounir Lamouri (:mounir) 2012-07-10 05:24:26 PDT
With the work being done for the security model, it should be quite easy to get all those information (DocShell and Principal should carry enough data for that). Maybe we could wait for that? Instead of hurrying up a temporary solution that will need to be fixed?
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-10 10:21:43 PDT
To the best of my knowledge, we're not missing any information we need to finish the work here.
Comment 13 Mounir Lamouri (:mounir) 2012-07-10 10:25:33 PDT
Indeed, we might be able to handle that currently but we might have to re-write that later to use other ways. The same way nsIDOMWindowUtils.setApp() is very likely going to be removed in favor of using Principal. I would prefer not to add to many things that will need to be rewritten.
Comment 14 Justin Lebar (not reading bugmail) 2012-07-10 10:26:49 PDT
What's happening is, the information of "is this docshell/frame" a mozbrowser/mozapp is all moving around as part of Mounir's patches.  We have the information, for sure, but mounir needs to figure out where it's going to live.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-10 10:33:15 PDT
We need the patch here very urgently.  I don't see any value in blocking this on moving some things around.
Comment 16 Justin Lebar (not reading bugmail) 2012-07-10 10:35:35 PDT
The value is that it's less work for mounir when he goes to move things around, which means that we get his permission work quicker.  But it's not worth arguing about; I'll write this patch this afternoon.
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-10 10:37:32 PDT
Yes, but the work here is pretty much the simplest possible use of mozapp/mozbrowser bits.  I don't expect this to create nontrivial "porting" effort.
Comment 18 Justin Lebar (not reading bugmail) 2012-07-11 09:05:38 PDT
Created attachment 641075 [details] [diff] [review]
Part 1: IPC changes.

The handling of these lists (which are deleted when they become empty) is
pretty ugly.  I have a plan to fix it, but since this is urgent, I don't think
we should block here.
Comment 19 Justin Lebar (not reading bugmail) 2012-07-11 09:06:30 PDT
Created attachment 641076 [details] [diff] [review]
Part 2: Content/DOM changes.
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-11 11:33:00 PDT
Comment on attachment 641075 [details] [diff] [review]
Part 1: IPC changes.

We need to shut down "app" subprocesses when the app dies, and we would like to be able to use the isApp bit to make some decisions in the subprocess, but I'm ok doing that in followups.
Comment 21 Justin Lebar (not reading bugmail) 2012-07-11 13:50:08 PDT
> The handling of these lists (which are deleted when they become empty) is
> pretty ugly.  I have a plan to fix it

This is bug 772987.
Comment 22 Mounir Lamouri (:mounir) 2012-07-12 15:47:38 PDT
Comment on attachment 641076 [details] [diff] [review]
Part 2: Content/DOM changes.

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

r=me

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +330,5 @@
> +  // TODO: We surely need a permissions check here, particularly once we no
> +  // longer rely on the mozbrowser permission check.
> +
> +  nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
> +  NS_ENSURE_STATE(appsService);

nit: NS_ENSURE_STATE() is evil, we should use more explicit macros.

@@ +333,5 @@
> +  nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID);
> +  NS_ENSURE_STATE(appsService);
> +
> +  nsAutoString manifestURL;
> +  GetAttr(kNameSpaceID_None, nsGkAtoms::mozapp, manifestURL);

You could do an early check way before with HasAttr(). Also, you could at least check if manifestURL is the empty string. In that case, there is no need bothering the service.
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 12:55:39 PDT
Actually, it would be quite easy to shut down "app subprocesses" when their content is closed, so now I'm thinking I want to do that here.
Comment 24 Justin Lebar (not reading bugmail) 2012-07-13 13:50:16 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> Actually, it would be quite easy to shut down "app subprocesses" when their
> content is closed, so now I'm thinking I want to do that here.

Up to you; I can land this now, or I can figure out killing the subprocesses on Monday.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 13:59:37 PDT
Land.  I'll probably be able to look this weekend.  (Simple in theory but might get hairy.)
Comment 26 Justin Lebar (not reading bugmail) 2012-07-13 15:23:22 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9ddc7f3d8271

Note You need to log in before you can comment on or make changes to this bug.