1191083 - Add mechanism to handle native calls before Gecko is loaded

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

As we're switching GeckoEvent to native calls, we want to be able to accept native calls before Gecko is loaded.

Comment 1

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

Attachment: Rename and expand GeckoThread.LaunchState (v1)

GeckoThread.LaunchState now covers the entire GeckoThread lifetime and
not just launch, so it's renamed to GeckoThread.State. More utility
methods are added to check for the current state.

Comment 2

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

Attachment: Implement more GeckoThread states (v1)

Implement the MOZGLUE_READY and JNI_READY states in GeckoThread. Also
change GeckoJavaSampler to use the new states instead of a separate
flag.

Comment 3

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

Attachment: Introduce pending objects support (v1)

Pending objects, which implement GeckoThread.PendingObject, are objects
that depend on Gecko or the object itself to be in a certain state,
before its methods can be safely called. For example, objects with
native methods depend on libxul being loaded and initialized to a
certain degree; and objects that have instance native methods depend on
a C++ object being attached to the Java object.

GeckoThread.newPendingObjectBuffer produces an object that will buffer
all incoming calls on an interface that the pending object implements,
until the object's readyForCalls method returns true, at which point all
buffered calls are played back in sequence. For example, for pending
object A that depends on Gecko state S to work and implements interface
B (along with GeckoThread.PendingObject), newPendingObjectBuffer
produces a new object, which also implements B, that will buffer (i.e.
save) all calls to B's methods. Once A.readyForCalls(S) returns true,
all previously buffered calls will be played back on the original object
A, in the sequence of the original calls.

Comment 4

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

Comment on attachment 8643433 [details] [diff] [review]
Introduce pending objects support (v1)

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

This is pretty complicated. I am still thinking we should just not call stuff when gecko isn't ready. We could add add some features to GeckoThread to make it easier to register for work to be done upon entering a certain state, maybe. Like maybe you give it a Runnable to execute when we enter JNI_READY? I could be convinced otherwise, but this is a lot of complexity that seems like it could be avoided if we just manage our startup sequence better (which we should do anyway).

Comment 5

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

I actually feel like this is the least complicated way.

Avoiding calling Gecko involves auditing every sender of an event to see if it's possible to be called during startup or not, with startup lasting an arbitrary amount of time. That seems like a huge task by itself, because even with knowledge of the code, it's not always obvious whether some code could run at startup or not. We could just experiment and see what crashes at startup, but that just sounds like a trap for regressions and it can get time-consuming as well.

Assuming we find all the places that call into Gecko at startup, now we need to find ways to avoid it. We can rewrite the code to be dependent on Gecko loading, but I have a feeling that will involve redesigning the entire startup process, which seems like it will take a long time. The more likely approach, like you said, is saving the call to a Runnable and keeping a list of Runnables in GeckoThread, but that's basically the approach taken by this patch, except you have to write a Runnable for each event, whereas this patch doesn't require you to write Runnables manually.

Comment 6

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

This is a list of events likely to be sent during startup,

BROADCAST
PREFERENCES_OBSERVE
PREFERENCES_GET
PROCESS_OBJECT
SCREENORIENTATION_CHANGED
TELEMETRY_HISTOGRAM_ADD
TELEMETRY_UI_EVENT
TELEMETRY_UI_SESSION_START
TELEMETRY_UI_SESSION_STOP

Comment 7

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

(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> This is a list of events likely to be sent during startup,
> 
> BROADCAST

Most of those can probably just be dropped on the floor, I'd think?

> PREFERENCES_OBSERVE
> PREFERENCES_GET

These would be one of those things that gets queued into a runnable.

> PROCESS_OBJECT

Ugh. Where does that get used?

> SCREENORIENTATION_CHANGED

Can just drop this one

> TELEMETRY_HISTOGRAM_ADD
> TELEMETRY_UI_EVENT
> TELEMETRY_UI_SESSION_START
> TELEMETRY_UI_SESSION_STOP

All of that would be in one runnable for booting UI telemetry.

Overall this list doesn't seem *that* bad. To try to root out all of the possible crashers here we could delay Gecko startup for an arbitrarily long period of time and click around.

Comment 8

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

Attachment: Add mechanism to queue native calls in GeckoThread (v1)

We cannot call native methods until Gecko is loaded. This patch adds a
mechanism in GeckoThread so that other code can queue up native method
calls and have those calls automatically delivered when Gecko is ready.

Comment 9

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

Attachment: Merge pending events handling into mechanism for queued native calls (v1)

Right now we have a separate way of handling pending events before Gecko
is loaded. We can merge that into the new mechanism for queuing native
calls.

Comment 10

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

Comment on attachment 8646526 [details] [diff] [review]
Add mechanism to queue native calls in GeckoThread (v1)

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

::: mobile/android/base/GeckoThread.java
@@ +89,5 @@
>      @WrapForJNI
>      private static MessageQueue msgQueue;
>  
> +    private static final int QUEUED_CALLS_COUNT = 16;
> +    private static final ArrayList<Method> QUEUED_METHODS = new ArrayList<>(QUEUED_CALLS_COUNT);

I think this should just be one list containing an object that has the method, object, args, etc.

Comment 11

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

Comment on attachment 8646535 [details] [diff] [review]
Merge pending events handling into mechanism for queued native calls (v1)

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

This will be a little simpler after you merge the arraylists all into one

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8234563d01fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a3f57f493a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e993120c414
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c457344c0e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8234563d01fc
https://hg.mozilla.org/mozilla-central/rev/c0a3f57f493a
https://hg.mozilla.org/mozilla-central/rev/9e993120c414
https://hg.mozilla.org/mozilla-central/rev/d3c457344c0e