Closed Bug 999853 Opened 6 years ago Closed 6 years ago

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


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




Firefox 31
Tracking Status
firefox29 --- unaffected
firefox30 --- fixed
firefox31 --- fixed
fennec 30+ ---


(Reporter: Margaret, Assigned: Margaret)


(Blocks 1 open bug)


Crash Data


(1 file)

I ran into this on a local build (today's m-c) with my hub kitchen sink add-on:

E/GeckoAppShell(25510): java.lang.IllegalArgumentException: Path must not be empty.
E/GeckoAppShell(25510): 	at com.squareup.picasso.Picasso.load(
E/GeckoAppShell(25510): 	at org.mozilla.gecko.home.PanelLayout.deliverDataset(
E/GeckoAppShell(25510): 	at org.mozilla.gecko.home.DynamicPanel$PanelLoaderCallbacks.onLoadFinished(
E/GeckoAppShell(25510): 	at$LoaderInfo.callOnLoadFinished(
E/GeckoAppShell(25510): 	at$LoaderInfo.onLoadComplete(
E/GeckoAppShell(25510): 	at
E/GeckoAppShell(25510): 	at org.mozilla.gecko.home.SimpleCursorLoader.deliverResult(
E/GeckoAppShell(25510): 	at org.mozilla.gecko.home.SimpleCursorLoader.deliverResult(
E/GeckoAppShell(25510): 	at
E/GeckoAppShell(25510): 	at$LoadTask.onPostExecute(
E/GeckoAppShell(25510): 	at
E/GeckoAppShell(25510): 	at$500(
E/GeckoAppShell(25510): 	at$InternalHandler.handleMessage(
E/GeckoAppShell(25510): 	at android.os.Handler.dispatchMessage(
E/GeckoAppShell(25510): 	at android.os.Looper.loop(
E/GeckoAppShell(25510): 	at
E/GeckoAppShell(25510): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoAppShell(25510): 	at java.lang.reflect.Method.invoke(
E/GeckoAppShell(25510): 	at$
E/GeckoAppShell(25510): 	at
E/GeckoAppShell(25510): 	at dalvik.system.NativeStart.main(Native Method)
Crash Signature: java.lang.IllegalArgumentException: Path must not be empty.
Picasso is fine with null image URLs but not with empty strings. Maybe we should avoid saving this kind of data altogether here?
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.
Assignee: nobody → margaret.leibovic
Priority: -- → P1
We actually handle this for panel items with a TextUtils.isEmpty check before making the Picasso call:

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.
I updated PanelBackItemView and PanelLayout to do what we do in PanelAuthLayout.
Attachment #8411362 - Flags: review?(
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/
@@ +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:

       .load(TextUtils.isEmpty(backImageUrl) ? null : backImageUrl),

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

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

Attachment #8411362 - Flags: review?( → review+
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.
tracking-fennec: ? → 30+

I'll request uplift once we also uplift the empty views stuff.
Flags: needinfo?(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
Attachment #8411362 - Flags: approval-mozilla-aurora?
Flags: needinfo?(margaret.leibovic)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8411362 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
You need to log in before you can comment on or make changes to this bug.