Use mozapp/mozbrowser information when allocating remote iframes to content processes

RESOLVED FIXED in mozilla16

Status

()

Core
IPC
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(2 attachments)

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.
Blocks: 745143
(Assignee)

Comment 1

5 years ago
> 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.
You're thinking of the linux OOM killer.

I'm talking about the android lowmemkiller driver, which we can program to suit our needs.
Before you ask,

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

;)

Updated

5 years ago
blocking-basecamp: --- → +
Blocks: 768832
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
jlebar, you're not working on this, right?
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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.
Blocks: 693515
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.
(Assignee)

Comment 9

5 years ago
> 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.
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.
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?
To the best of my knowledge, we're not missing any information we need to finish the work here.
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.
(Assignee)

Comment 14

5 years ago
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.
We need the patch here very urgently.  I don't see any value in blocking this on moving some things around.
(Assignee)

Comment 16

5 years ago
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.
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 18

5 years ago
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.
Attachment #641075 - Flags: review?(jones.chris.g)
(Assignee)

Comment 19

5 years ago
Created attachment 641076 [details] [diff] [review]
Part 2: Content/DOM changes.
Attachment #641076 - Flags: review?(mounir)
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.
Attachment #641075 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 21

5 years ago
> 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 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.
Attachment #641076 - Flags: review?(mounir) → review+
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.
(Assignee)

Comment 24

5 years ago
(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.
Land.  I'll probably be able to look this weekend.  (Simple in theory but might get hairy.)
(Assignee)

Comment 26

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9ddc7f3d8271
https://hg.mozilla.org/mozilla-central/rev/d3212385b1af
https://hg.mozilla.org/mozilla-central/rev/9ddc7f3d8271
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.