Closed Bug 1389296 Opened 7 years ago Closed 7 years ago

Crash in java.lang.IllegalStateException: Page URL is required at org.mozilla.gecko.icons.IconRequestBuilder.build(IconRequestBuilder.java)

Categories

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

Unspecified
Android
defect

Tracking

(fennec+, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
fennec + ---
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: mccr8, Assigned: sebastian)

References

Details

(Keywords: crash, Whiteboard: [mobileAS])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-95d3a625-b61e-4d06-9ee2-2120a0170810.
=============================================================

Android top crash for the August 10th Nightly, though I don't see a ton of different installs. This crash signature is also occurring on other branches.
388 crashes in 7 days. Stack trace indicates this is AS (TopSitesCard):

java.lang.IllegalStateException: Page URL is required
	at org.mozilla.gecko.icons.IconRequestBuilder.build(IconRequestBuilder.java:134)
	at org.mozilla.gecko.activitystream.homepanel.topsites.TopSitesCard.bind(TopSitesCard.java:99)
	at org.mozilla.gecko.activitystream.homepanel.topsites.TopSitesPageAdapter.onBindViewHolder(TopSitesPageAdapter.java:92)
	at org.mozilla.gecko.activitystream.homepanel.topsites.TopSitesPageAdapter.onBindViewHolder(TopSitesPageAdapter.java:29)
	at android.support.v7.widget.RecyclerView$Adapter.onBindViewHolder(Unknown Source)
tracking-fennec: --- → ?
Whiteboard: [mobileAS]
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee: s.kaspari → nobody
Status: ASSIGNED → NEW
tracking-fennec: ? → +
Iteration: --- → 1.28
Priority: -- → P1
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Iteration: 1.28 → 1.29
Comment on attachment 8904265 [details]
Bug 1389296 - TopSitesCard: Only load icon if we have a non-empty non-null page URL.

https://reviewboard.mozilla.org/r/176040/#review181366

r+ - seems like it'd do the trick but I'm unclear on the larger picture and wondering if this is the optimal approach.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java:101
(Diff revision 1)
>              ongoingIconLoad.cancel(true);
>          }
>  
> +        if (TextUtils.isEmpty(topSite.getUrl())) {
> +            // Sometimes we get top sites without or with an empty URL - even though we do not allow
> +            // this anywhere in our UI. However with 'sync' we are not in full control of the data.

If we don't allow empty URLs near the UI (and this is a UI method), how are we getting an empty URL here?

I don't understand the "with 'sync'" comment – maybe that's why?

I don't feel like we should ever have an empty URL in this method – should we be fixing this further down the line? That being said, maybe we should keep the `isEmpty` just in case the code changes and we get nulls later. Perhaps you can add an example to your comment on when we might get an empty URI to make it clearer for doubters like me?
Attachment #8904265 - Flags: review?(michael.l.comella) → review+
Iteration: 1.29 → 1.30
(In reply to Michael Comella (:mcomella) from comment #3)
> If we don't allow empty URLs near the UI (and this is a UI method), how are
> we getting an empty URL here?
> 
> I don't understand the "with 'sync'" comment – maybe that's why?

I do not know how we get empty URLs into the database here. Our UI tries to avoid that. However we have "Firefox Sync" and maybe even add-ons writing data too (ni :grisha).

(In reply to Michael Comella (:mcomella) from comment #3)
> I don't feel like we should ever have an empty URL in this method – should
> we be fixing this further down the line?

I've been thinking about this too. Because I do not know why those values exist I do not just want to hide them. Instead I just load no icon and the user can decide what to do with this top sites (Keep, edit, delete, ..).
Flags: needinfo?(gkruglov)
DB schema doesn't allow history with null URLs, but it's possible to have a bookmark record with a null url. Most commonly that would be a folder. Combined view filters bookmarks by: type=bookmark, not deleted, not a pin, url not in table history.

Top Sites code selects from the combined view, and from pinned bookmarks.

As far as I can tell, it's not possible to pin a bookmark record with type!=bookmark (a folder, a query...). You should double check that this is the case.

And so we're left with "combined view has a bookmark record with type=bookmark and url=null". Looking through our bookmark sync code, as far as I can tell it allows in bookmarks of type=bookmark and uri=null. I haven't checked what would happen if that bookmark surfaced up in the bookmarks panel - likely "nothing good".

A better approach would be to filter these out either in the Combined view (requires a db migration), or in BrowserProvider's getTopSites(). I don't see a valid reason to have a top site with a null URI, and so that seems like a better fix for this crash. If you're still crashing, we'll know that _something else_ is also a factor, which is a win in itself.
Flags: needinfo?(gkruglov)
Another possibility is that our deletion is code is somehow screwing up, and not setting is_deleted flag after nulling-out record fields. I really hope this isn't the case, and overall this seems less likely. But, there was a lot of churn in that part of the BrowserProvider recently, so also worth checking.
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/05749026567b
TopSitesCard: Only load icon if we have a non-empty non-null page URL. r=mcomella
Grisha: Note that this exception is not only triggered for null values but also for empty strings.

I landed the patch as-is.

As I see it there are three straight-forward solutions:

* Do not show those items. As I do not know where they come from I do not want to randomly hide them. I'd prefer showing them and letting the user decide.

* Generate a default icon for it: This means changing the code to deal with null and empty values. This is a bigger change and I don't think it's really worth it.

* Not displaying an icon. That's what the patch does. The title is still shown (assuming there is one). With that the user can decide to get rid of the item if it's not needed.
https://hg.mozilla.org/mozilla-central/rev/05749026567b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
There appear to be crashes on 55/56 as well. Presumably not from Activity Stream :). Any idea what those are?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(s.kaspari)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.