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

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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)

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1389296 - TopSitesCard: Only load icon if we have a non-empty non-null page URL.

Review commit: https://reviewboard.mozilla.org/r/176040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176040/

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(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, ..).

Comment 5

[:Grisha Kruglov]

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.

Comment 6

[:Grisha Kruglov]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05749026567b
TopSitesCard: Only load icon if we have a non-empty non-null page URL. r=mcomella

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/05749026567b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

There appear to be crashes on 55/56 as well. Presumably not from Activity Stream :). Any idea what those are?