Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Description • 12 years ago

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.

	Doug Turner (:dougt)
	Comment 1 • 12 years ago

chris, nice to have but not blocking, right?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 2 • 12 years ago

Correct.

	Andrew Overholt [:overholt]
	Comment 3 • 12 years ago

Not a blocker based on comments #1 and #2.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 4 • 12 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 5 • 12 years ago

It looks like we're going to need this to hit startup targets.  Siiiigh.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 8 • 12 years ago

Comment on attachment 703198 [details] [diff] [review]
part 1: Refactor privilege adjustment

Botched something in patch upload, one minute.

	Justin Lebar (not reading bugmail)
	Comment 11 • 12 years ago

Comment on attachment 703204 [details] [diff] [review]
part 2: Make the prelaunch process totipotent and specialize when it's taken

Looks great.

	Dave Hylands [:dhylands]
	Comment 12 • 12 years ago

(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).

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 13 • 12 years ago

Good idea.  Let's do that in a followup.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 14 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c26533f49aaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8463ca8a191a

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 15 • 12 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 16 • 12 years ago

Backed out, win32 borkage 

https://hg.mozilla.org/integration/mozilla-inbound/rev/66f820124d4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2f112e2de6
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8d8c2d97fd

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 17 • 12 years ago

Round two

https://hg.mozilla.org/integration/mozilla-inbound/rev/a18d9b97ac9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67c3370a85f

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 18 • 12 years ago

(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.)

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 19 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/a18d9b97ac9c
https://hg.mozilla.org/mozilla-central/rev/b67c3370a85f

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 12 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d9d299d9e6cf

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 12 years ago

Follow-up for bustage (the roll-up patch was missing an include in ContentChild.cpp).
https://hg.mozilla.org/releases/mozilla-b2g18/rev/635fb4ce37af

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 12 years ago

(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

	Gabriele Svelto [:gsvelto]
	Comment 23 • 12 years ago

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.

	Dave Hylands [:dhylands]
	Comment 24 • 12 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 25 • 12 years ago

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.

	Gabriele Svelto [:gsvelto]
	Comment 26 • 12 years ago

(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.

	Gabriele Svelto [:gsvelto]
	Comment 27 • 12 years ago

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?

	Justin Lebar (not reading bugmail)
	Comment 28 • 12 years ago

> Shall I open a separate bug for this?

Sounds good.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 29 • 12 years ago

(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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 30 • 12 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0cdacae4e612

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 12 years ago

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() :

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Assignee
	Comment 32 • 12 years ago

(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

	Landry Breuil (:gaston)
	Comment 33 • 12 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 34 • 12 years ago

Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0