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

RESOLVED FIXED in Firefox 30

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Blocks 1 bug)

Trunk
Firefox 31
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 unaffected, firefox30 fixed, firefox31 fixed, fennec30+)

Details

(crash signature)

Attachments

(1 attachment)

Assignee

Description

5 years ago
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)
Assignee

Updated

5 years ago
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?

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

Comment 2

5 years ago
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

Updated

5 years ago
Assignee: nobody → margaret.leibovic
Priority: -- → P1
Assignee

Comment 3

5 years ago
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.
Assignee

Comment 4

5 years ago
I updated PanelBackItemView and PanelLayout to do what we do in PanelAuthLayout.
Attachment #8411362 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8411362 - Flags: review?(lucasr.at.mozilla) → review+
Assignee

Comment 6

5 years ago
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.
Assignee

Updated

5 years ago
tracking-fennec: ? → 30+
Assignee

Comment 7

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

I'll request uplift once we also uplift the empty views stuff.
Flags: needinfo?(margaret.leibovic)
Assignee

Comment 8

5 years ago
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)
https://hg.mozilla.org/mozilla-central/rev/ebe3125cd74f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8411362 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 11

5 years ago
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.