1404144 - [geckoview] Support preloading GeckoView child process

Status: RESOLVED FIXED

(GeckoView :: Sandboxing, enhancement)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Add an option in GeckoView.preload to preload the e10s child process, so we spend less time waiting for the child process to get ready when it's needed.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1404144 - 1. Refactor child process code to support preloading;

Refactor the code in GeckoProcessManager and GeckoServiceChildProcess so
that, we can have a ChildConnection object that's bound but not started.
This way we can bind the connection to preload Gecko child process, but
hold off starting until told by Gecko main process.

Some code is simplified. For example, `IChildProcess.stop` is removed in
favor of killing the child process directly.

Review commit: https://reviewboard.mozilla.org/r/186166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186166/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1404144 - 2. Move start child process JNI call to GeckoProcessManager;

Avoid going through GeckoAppShell and move the start child process JNI
call directly to GeckoProcessManager.

Review commit: https://reviewboard.mozilla.org/r/186168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186168/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1404144 - 3. Add flag to preload child process during main GeckoThread startup;

We need to wait for GeckoThread to load the Gecko libs in the main
process before we can launch any child processes, so that the child
process doesn't try to extract libs, which can conflict with any
extraction going on in the main process.

Review commit: https://reviewboard.mozilla.org/r/186170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186170/

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1404144 - 4. Preload child process in geckoview_example in multiprocess mode;

In geckoview_example multiprocess mode, preload child process during
startup to make e10s startup faster.

Review commit: https://reviewboard.mozilla.org/r/186172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186172/

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8914926 [details]
Bug 1404144 - 3. Add flag to preload child process during main GeckoThread startup;

https://reviewboard.mozilla.org/r/186170/#review191400

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8914927 [details]
Bug 1404144 - 4. Preload child process in geckoview_example in multiprocess mode;

https://reviewboard.mozilla.org/r/186172/#review191402

Comment 7

[Randall Barker [:rbarker]]

Comment on attachment 8914925 [details]
Bug 1404144 - 2. Move start child process JNI call to GeckoProcessManager;

https://reviewboard.mozilla.org/r/186168/#review191530

Comment 8

[Randall Barker [:rbarker]]

Comment on attachment 8914924 [details]
Bug 1404144 - 1. Refactor child process code to support preloading;

https://reviewboard.mozilla.org/r/186166/#review191584

The diff makes the patch hard to read, but it looks okay to me.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoProcessManager.java
(Diff revision 1)
>          @Override
>          public void binderDied() {
> -            Log.e(LOGTAG, "Binder died. Attempt to unbind service: " + mType + " " + mPid);
> +            Log.i(LOGTAG, "Binder died for " + mType);
> -            try {
> -                GeckoAppShell.getApplicationContext().unbindService(this);
> +            GeckoAppShell.getApplicationContext().unbindService(this);
> -            } catch (final java.lang.IllegalArgumentException e) {

Are you not seeing this exception? I don't remember why this try/catch was here but thought it might be because sometimes unbindService would throw if the service had already been unbound?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/process/GeckoServiceChildProcess.java:100
(Diff revision 1)
>          Log.i(LOGTAG, "Service has been unbound. Stopping.");
> -        stop();
> +        Process.killProcess(Process.myPid());
>          return false;
>      }
>  
> -    @JNITarget
> +    public static final class geckomediaplugin extends GeckoServiceChildProcess {}

I think I marked them @JNITarget to prevent ProGuard from removing them. I guess it isn't an issue?

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8914924 [details]
Bug 1404144 - 1. Refactor child process code to support preloading;

https://reviewboard.mozilla.org/r/186166/#review191584

> Are you not seeing this exception? I don't remember why this try/catch was here but thought it might be because sometimes unbindService would throw if the service had already been unbound?

Yeah reading the source looks like `IllegalArgumentException` is possible, but I think if we're smart with managing states we shouldn't encounter it.

> I think I marked them @JNITarget to prevent ProGuard from removing them. I guess it isn't an issue?

Right, ProGuard doesn't remove public classes.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8914924 [details]
Bug 1404144 - 1. Refactor child process code to support preloading;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186166/diff/1-2/

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8914925 [details]
Bug 1404144 - 2. Move start child process JNI call to GeckoProcessManager;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186168/diff/1-2/

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8914926 [details]
Bug 1404144 - 3. Add flag to preload child process during main GeckoThread startup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186170/diff/1-2/

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8914927 [details]
Bug 1404144 - 4. Preload child process in geckoview_example in multiprocess mode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186172/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5467f3e1c9a8
1. Refactor child process code to support preloading; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/b54db0501532
2. Move start child process JNI call to GeckoProcessManager; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/dbfa1f3407f1
3. Add flag to preload child process during main GeckoThread startup; r=snorp
https://hg.mozilla.org/integration/autoland/rev/1628829b6082
4. Preload child process in geckoview_example in multiprocess mode; r=snorp

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5467f3e1c9a8
https://hg.mozilla.org/mozilla-central/rev/b54db0501532
https://hg.mozilla.org/mozilla-central/rev/dbfa1f3407f1
https://hg.mozilla.org/mozilla-central/rev/1628829b6082

Comment 16

[Chris Peterson [:cpeterson]]

Moving content process bugs to the new GeckoView::Sandboxing component.