Support preloading processes that are launched with INHERIT_PRIVILEGES

RESOLVED FIXED in Firefox 21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, blocking-basecamp:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

Details

(Whiteboard: [FFOS_perf])

Attachments

(3 attachments, 2 obsolete attachments)

Bug 782456 added a hack to inherit permissions for apps that need them.  This hack prevents using the prelaunched process for these inherit-permission processes, meaning that they will incur startup overhead.  We can fix that by always launching the prelaunch process with INHERIT and then dropping privs after we know which app it's going to run.
chris, nice to have but not blocking, right?
blocking-basecamp: --- → ?
Not a blocker based on comments #1 and #2.
blocking-basecamp: ? → -
We may need this to hit the targets in bug 817051 and bug 817116.

We can make this work by deferring privilege dropping until we load the first TabChild "for real", here.  That's not supported on all platforms though (linux supports it) and it makes me very squeamish security-wise.  But it's not hard to implement.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#147
Blocks: slim-fast
No longer blocks: b2g-e10s-work
It looks like we're going to need this to hit startup targets.  Siiiigh.
Assignee: nobody → jones.chris.g
Comment on attachment 703198 [details] [diff] [review]
part 1: Refactor privilege adjustment

Botched something in patch upload, one minute.
Attachment #703198 - Attachment is obsolete: true
Attachment #703198 - Flags: review?(dhylands)
Attachment #703200 - Attachment is obsolete: true
Attachment #703200 - Flags: review?(justin.lebar+bug)
Attachment #703204 - Flags: review? → review?(justin.lebar+bug)
Comment on attachment 703204 [details] [diff] [review]
part 2: Make the prelaunch process totipotent and specialize when it's taken

Looks great.
Attachment #703204 - Flags: review?(justin.lebar+bug) → review+
Attachment #703202 - Flags: review?(dhylands) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> Bug 782456 added a hack to inherit permissions for apps that need them. 
> This hack prevents using the prelaunched process for these
> inherit-permission processes, meaning that they will incur startup overhead.
> We can fix that by always launching the prelaunch process with INHERIT and
> then dropping privs after we know which app it's going to run.

I think we should drop privs as soon as possible after launching, using seteuid/setegid. Then once we know which app we're going to be, we should elevate back to root (seteuid(0)) and finally do the permanent set (so no more elevation possible).
Good idea.  Let's do that in a followup.
Blocks: 831912
I seem to have grown approval powers, but it's bad form to get high on your own supply.

This is not a zero-risk change, but it's relatively safe, easy to verify correct, and wins back startup time needed for product requirements that we can't get in any other way.
Attachment #703486 - Flags: review+
Attachment #703486 - Flags: approval-mozilla-b2g18?
blocking-b2g: --- → tef?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> Created attachment 703486 [details] [diff] [review]
> Rebased on gecko-18

(Imported the fix to this patch, but not reposting to avoid churn.)
blocking-b2g: tef? → tef+
Attachment #703486 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/mozilla-central/rev/a18d9b97ac9c
https://hg.mozilla.org/mozilla-central/rev/b67c3370a85f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Follow-up for bustage (the roll-up patch was missing an include in ContentChild.cpp).
https://hg.mozilla.org/releases/mozilla-b2g18/rev/635fb4ce37af
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> > Created attachment 703486 [details] [diff] [review]
> > Rebased on gecko-18
> 
> (Imported the fix to this patch, but not reposting to avoid churn.)

Guess that should have been enough to keep me away. Backed out for win32 bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/668e0cdfcc84

Updated

7 years ago
Blocks: 832723
We did some subjective testing of this patch and it didn't seem to gain us much, so I run it through the profiler and discovered something weird. This is the Camera application starting up w/o the patch:

http://people.mozilla.com/~bgirard/cleopatra/#report=2d352069167ff49b2f6239fea3ef1b98c3c97ded

If you look at the phase which precedes the first ProcessNextNativeEvent::Wait pause it lasts for ~2.6s (256 samples), that's the app starting up from scratch.

This on the other hand is a profile with the patch applied:

http://people.mozilla.com/~bgirard/cleopatra/#report=fd3a57acc94e727a0eba4af5053d573d0ce0ddad

The first phase which happens while the app is being preloaded takes ~0.6s (63 samples) then the real launch phase lasts ~2.4s (236 samples). So it looks like a ~0.4s slow down was introduced during the application launch phase which eats up most of the time saved by preloading it.

Unfortunately just looking at the profiles I cannot easily tell where the extra delay is coming from. BTW all testing was done on the Otoro with this mornings gecko/gaia checkouts.
If I had to guess, I'd say that you're losing due to cold caches (the L1/L2 caches) and possibly (although seems like more of a stretch) cold file cache.

The L1/L2 stuff is really hard to instrument without access to on-chip performance registers.

If you could monitor cache-hits/cache-misses then you could compare those between the 2 runs.
Gabriele, do your results apply to other apps as well?  That is, how much is preloading buying us for, say, the FM radio app?

I think all you need to do to disable preloading is to set dom.ipc.processPrelaunch.enabled to false (b2g/app/b2g.js).

> The L1/L2 stuff is really hard to instrument without access to on-chip performance 
> registers.

Hopefully I'll get perf working soon, and then we can go to town.  Bug 831611.
(In reply to Dave Hylands [:dhylands] from comment #24)
> If I had to guess, I'd say that you're losing due to cold caches (the L1/L2
> caches) and possibly (although seems like more of a stretch) cold file cache.

IMHO 400ms sound like a bit too much to be caused by cold caches/TLB, but your comment gave me an idea. I want to check if we're flushing some of our internal caches and how much I/O is involved in the process; I've run these tests on the Otoro so slow I/O might actually be a significant factor.

(In reply to Justin Lebar [:jlebar] from comment #25)
> Gabriele, do your results apply to other apps as well?  That is, how much is
> preloading buying us for, say, the FM radio app?
>
> I think all you need to do to disable preloading is to set
> dom.ipc.processPrelaunch.enabled to false (b2g/app/b2g.js).

I haven't tried yet but I will. I'll start with the Video app which should also be affected by this patch.
I tried profiling the Template app with and without preloading and I can reproduce the same issue:

- This is without preloading the whole startup process takes ~1240ms

http://people.mozilla.com/~bgirard/cleopatra/#report=a85b122c1f323253cb9ee322f28303111e6625b0

- This is with preloading, the preload phase takes ~620ms and the launch phase ~980ms so 1600ms in total

http://people.mozilla.com/~bgirard/cleopatra/#report=15dc4e0c9ff20b64dc607c3329e34c48dd834bea

I will try to experiment on another phone to see if it might be caused by slow I/O. Shall I open a separate bug for this?
> Shall I open a separate bug for this?

Sounds good.
(In reply to Ryan VanderMeulen from comment #22)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> > > Created attachment 703486 [details] [diff] [review]
> > > Rebased on gecko-18
> > 
> > (Imported the fix to this patch, but not reposting to avoid churn.)
> 
> Guess that should have been enough to keep me away. Backed out for win32
> bustage.
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/668e0cdfcc84

Sorry, should have announced comment 18 louder.
Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f00db85a56da

https://tbpl.mozilla.org/php/getParsedLog.php?id=19061198&tree=Mozilla-B2g18

/builds/slave/m-b2g18-osx64-g/build/dom/ipc/ContentChild.cpp:501:27: error: use of undeclared identifier 'GeckoChildProcessHost'
                          GeckoChildProcessHost::DefaultChildPrivileges() :
(In reply to Ryan VanderMeulen from comment #31)
> Backed out for bustage.
> f00d

Mmmmm best lunchtime backout evar.  Thanks!

Repushed with header fix, for what must have been new rottage

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e718cccd8ed8
Fwiw https://hg.mozilla.org/mozilla-central/rev/a18d9b97ac9c broke builds on bsd since SetCurrentProcessPrivileges() is not defined in process_util_bsd.cc. Filing a followup as soon as i have a fix.
Whiteboard: [perf_tsk]
Whiteboard: [perf_tsk] → [FFOS_perf]
Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0
You need to log in before you can comment on or make changes to this bug.