999853 - java.lang.IllegalArgumentException: Path must not be empty.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[:Margaret Leibovic]

I ran into this on a local build (today's m-c) with my hub kitchen sink add-on: https://github.com/leibovic/hub-kitchen-sink/

E/GeckoAppShell(25510): java.lang.IllegalArgumentException: Path must not be empty.
E/GeckoAppShell(25510): 	at com.squareup.picasso.Picasso.load(Picasso.java:181)
E/GeckoAppShell(25510): 	at org.mozilla.gecko.home.PanelLayout.deliverDataset(PanelLayout.java:284)
E/GeckoAppShell(25510): 	at org.mozilla.gecko.home.DynamicPanel$PanelLoaderCallbacks.onLoadFinished(DynamicPanel.java:429)
E/GeckoAppShell(25510): 	at android.support.v4.app.LoaderManagerImpl$LoaderInfo.callOnLoadFinished(LoaderManager.java:427)
E/GeckoAppShell(25510): 	at android.support.v4.app.LoaderManagerImpl$LoaderInfo.onLoadComplete(LoaderManager.java:395)
E/GeckoAppShell(25510): 	at android.support.v4.content.Loader.deliverResult(Loader.java:104)
E/GeckoAppShell(25510): 	at org.mozilla.gecko.home.SimpleCursorLoader.deliverResult(SimpleCursorLoader.java:71)
E/GeckoAppShell(25510): 	at org.mozilla.gecko.home.SimpleCursorLoader.deliverResult(SimpleCursorLoader.java:26)
E/GeckoAppShell(25510): 	at android.support.v4.content.AsyncTaskLoader.dispatchOnLoadComplete(AsyncTaskLoader.java:223)
E/GeckoAppShell(25510): 	at android.support.v4.content.AsyncTaskLoader$LoadTask.onPostExecute(AsyncTaskLoader.java:61)
E/GeckoAppShell(25510): 	at android.support.v4.content.ModernAsyncTask.finish(ModernAsyncTask.java:461)
E/GeckoAppShell(25510): 	at android.support.v4.content.ModernAsyncTask.access$500(ModernAsyncTask.java:47)
E/GeckoAppShell(25510): 	at android.support.v4.content.ModernAsyncTask$InternalHandler.handleMessage(ModernAsyncTask.java:474)
E/GeckoAppShell(25510): 	at android.os.Handler.dispatchMessage(Handler.java:102)
E/GeckoAppShell(25510): 	at android.os.Looper.loop(Looper.java:137)
E/GeckoAppShell(25510): 	at android.app.ActivityThread.main(ActivityThread.java:4998)
E/GeckoAppShell(25510): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoAppShell(25510): 	at java.lang.reflect.Method.invoke(Method.java:515)
E/GeckoAppShell(25510): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:777)
E/GeckoAppShell(25510): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:593)
E/GeckoAppShell(25510): 	at dalvik.system.NativeStart.main(Native Method)

Comment 1

[Lucas Rocha (:lucasr)]

Picasso is fine with null image URLs but not with empty strings. Maybe we should avoid saving this kind of data altogether here?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/HomeProvider.jsm#272

Comment 2

[:Margaret Leibovic]

Yeah, that sounds like a good idea.

I just noticed this again and looked into what was causing it, and it was this:

    optionsCallback: function () {
      return {
        title: "Test Empty",
        views: [{
          type: Home.panels.View.LIST,
          dataset: "does.not.exist",
          empty: {
            text: "This is some test emtpy text",
            imageUrl: ""
          }
        }]
      };
    }

So we should also make sure to bail appropriately if add-ons use empty strings for other images.

Comment 3

[:Margaret Leibovic]

We actually handle this for panel items with a TextUtils.isEmpty check before making the Picasso call:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelItemView.java#67

I did a quick audit of where we use Picasso.with, and in addition to the empty view in PanelLayout, we could also run into this problem with PanelBackItemView.

While we could add a check higher up the stream to make sure we don't have an empty string for these values, adding a TextUtils.isEmpty check before using the Picasso API seems safest.

Comment 4

[:Margaret Leibovic]

Attachment: Never pass an empty image path to Picasso

I updated PanelBackItemView and PanelLayout to do what we do in PanelAuthLayout.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8411362 [details] [diff] [review]
Never pass an empty image path to Picasso

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

::: mobile/android/base/home/PanelBackItemView.java
@@ +36,5 @@
> +            Picasso.with(getContext())
> +                   .load(backImageUrl)
> +                   .placeholder(R.drawable.folder_up)
> +                   .into(image);
> +        }

This looks fine but wouldn't it better to just lean on Picasso for the placeholder logic? Something like:

Picasso.with(getContext())
       .load(TextUtils.isEmpty(backImageUrl) ? null : backImageUrl),
       .placeholder(R.drawable.folder_up)
       .into(image);

I don't feel strongly about it. Up to you.

::: mobile/android/base/home/PanelLayout.java
@@ +442,5 @@
>              } else {
>                  Picasso.with(getContext())
>                         .load(imageUrl)
>                         .error(R.drawable.icon_home_empty_firefox)
>                         .into(imageView);

Ditto.

Comment 6

[:Margaret Leibovic]

When working on the empty views, we discussed avoiding the placeholder because we would see a default image flash before the real image was loaded. However, when I tried using an error image instead of a placeholder, I saw a lag before the default image is displayed while Picasso tries to load the null URL (perhaps there's room for improvement in Picasso there). In any case, I found that this approach of directly setting the default image ourselves performed best.

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/ebe3125cd74f

I'll request uplift once we also uplift the empty views stuff.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8411362 [details] [diff] [review]
Never pass an empty image path to Picasso

Note to release managers: I'm requesting uplift for a series of Firefox Hub bugs. The main fixes we need are in bugs at the end of the series, but trying to rebase those patches proved difficult and risky, so I think we should just uplift the dependecies.

Note to sheriffs: I have a local patch series that I can land on aurora when these bugs all get approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dependency for initial Firefox Hub release (promoted feed add-ons in fx30)
User impact if declined: panels won't update when data changes
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): only affects dynamic panels, need an add-on to trigger this
String or IDL/UUID changes made by this patch: none

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/41b3780cc1d3

Comment 11

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.