894313 - GeckoThread should own its own static instance

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

I assert this is better, but happy to listen to other opinions.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 776292 [details] [diff] [review]
patch

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

Looks ok but would like to see an updated version with comments addressed.

::: mobile/android/base/GeckoApp.java
@@ +1190,4 @@
>          String args = intent.getStringExtra("args");
>          earlyStartJavaSampler(intent);
>  
> +

remove blank line

@@ +1243,4 @@
>              return;
>          }
>  
> +        if (GeckoThread.sGeckoThread != null) {

This seems like leaky abstraction to me. I would prefer if this was "if (!GeckoThread.isCreated())" or something similar. Basically, make sGeckoThread private and expose its nullness via a public method.

::: mobile/android/base/GeckoThread.java
@@ +39,5 @@
>  
> +    static GeckoThread sGeckoThread;
> +    static String sUri;
> +    static String sArgs;
> +    static String sAction;

All of these should be private.

@@ +47,5 @@
> +            sUri = uri;
> +        if (args != null)
> +            sArgs = args;
> +        if (action != null)
> +            sAction = action;

If the null checks here are really needed, add a comment as to why. From the code it looks like there might be some cases where this gets called multiple times and the null check will prevent future calls from clobbering the values set in older calls. Is that it?

@@ +127,5 @@
>  
>          Log.w(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - runGecko");
>  
> +        String args = addCustomProfileArg(sArgs);
> +        String type = getTypeFromAction(sAction != null ? sAction : 

trailing ws

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 777019 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoThread.java
@@ +44,3 @@
>  
> +    static void setStartupArgs(String uri, String args, String action) {
> +        // If a start up arg is non-null, set it

This comment is not useful. I can see that's what the code is doing, but the comment should explain *why* it only does it for non-null args. Why not just assign it unconditionally? The static members are initialized to null anyway so there's no harm in assigning a null to it again.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Comment on attachment 777019 [details] [diff] [review]
> patch
> 
> Review of attachment 777019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoThread.java
> @@ +44,3 @@
> >  
> > +    static void setStartupArgs(String uri, String args, String action) {
> > +        // If a start up arg is non-null, set it
> 
> This comment is not useful. I can see that's what the code is doing, but the
> comment should explain *why* it only does it for non-null args. Why not just
> assign it unconditionally? The static members are initialized to null anyway
> so there's no harm in assigning a null to it again.

The intention is that if you want to set one of the start up args, but not another you can just call with nulls for things you don't know yet.

Alternatively, we could have 3 separate methods to set each arg.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> The intention is that if you want to set one of the start up args, but not
> another you can just call with nulls for things you don't know yet.

Yeah but you can just as easily drop the null checks and it will work the same. As long as setStartupArgs is called only once it makes no difference whether you do the null checks or not, as you'll just be putting a null in to sUri (for example) that was already null to begin with.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: gecko_thread.patch

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 778170 [details] [diff] [review]
gecko_thread.patch

>+    static void setArgs(String args) {
>+            sArgs = args;
>+    }
>+
>+    static void setAction(String action) {
>+            sAction = action;
>+    }
>+

Fix indentation.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/03cef3d7e96d alongside bug 894885 because one or both of them broke Android.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2571b6bbc93a

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: gecko_thread.patch

try run

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

looks like I forgot to include the link to the try run https://tbpl.mozilla.org/?tree=Try&rev=9a219cbb89c1

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 784758 [details] [diff] [review]
gecko_thread.patch

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

::: mobile/android/base/GeckoThread.java
@@ +45,4 @@
>      private final String mUri;
>  
> +    public static boolean ensureInit() {
> +        if (isCreated())

Potential multithreading hazard here. Might be a good idea to stick in a call to ThreadUtils.assertOnUiThread() here to ensure it doesn't get called from any other thread, that will ensure the hazard is never tripped.

Comment 12

[Sriram Ramasubramanian [:sriram]]

Could we have this instance in GeckoApplication? Ideally Gecko should be running as long as GeckoApplication is up. In which case, GeckoApplication is a better place to take care of this thread -- as GeckoApplication is singleton, and can initialize Gecko. This would help initializing Gecko from Settings screen (if Settings is launched directly from notifications) and so on.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f93ac04d92ab