1310622 - 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

[Sebastian Kaspari (:sebastian; :pocmo)]

This bug was filed from the Socorro interface and is 
report bp-653b4c36-f0da-4e8f-b771-14caa2161012.
=============================================================

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

> java.lang.IllegalStateException: Page URL is required
> 	at org.mozilla.gecko.icons.IconRequestBuilder.build(IconRequestBuilder.java:123)
> 	at org.mozilla.gecko.home.TwoLinePageRow.update(TwoLinePageRow.java:279)
> 	at org.mozilla.gecko.home.TwoLinePageRow.updateFromCursor(TwoLinePageRow.java:319)
> 	at org.mozilla.gecko.home.BookmarksListAdapter.bindView(BookmarksListAdapter.java:332)
> 	at org.mozilla.gecko.home.MultiTypeCursorAdapter.getView(MultiTypeCursorAdapter.java:62)
> 	at android.widget.AbsListView.obtainView(AbsListView.java:2572)
> [..]

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

I don't understand this crash.

For this exception to be thrown pageURL needs to be null here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java#279

For it to be null stripAboutReaderUrl() needs to return null here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java#267

This can only happen if the url parameter is null.

However in this case we should have seen the nullpointer exception earlier:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java#254

Looking at mercurial there hasn't been any change recently. Also: There are only 2 reports so far. Both coming from the same device: meizu m3 note. Maybe this is a bogus report.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

So far this hasn't shown up anymore.

Comment 4

[Nevin Chen(Not active on Bugzilla)]

this crash only happened in release 51.0b.
https://crash-stats.mozilla.org/topcrashers/?product=Firefox&version=51.0b
I tired to see the if there's any way I can make url null (ex. Bookmark sync) but just like you said , if it could be null, there'll be NPE at " if (url.equals(mPageUrl)) {" part.

Can we just close this bug ?

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

This still seems to happen though. Did you check the mozilla-beta repository? Maybe we didn't uplift a patch and that's way it is only happening in Beta:
https://hg.mozilla.org/releases/mozilla-beta/

Comment 6

[Nevin Chen(Not active on Bugzilla)]

I've compared the log for TwoLinePageRow, the only difference is bug 1319302. But I think it's unrelated.
And for BookmarksListAdapter ,MultiTypeCursorAdapter they are the same in beta and central

Still can't find out the cause of this.

Maybe it happens when user sync bookmarks with empty urls. But I can't reproduce it even using Chrome nor another FireFox account.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

This looks a lot like what we fixed here:
https://hg.mozilla.org/releases/mozilla-beta/rev/5f96fd506060

But we still see some rare crashes in 51.0b2 which is the latest beta.

(In reply to Nevin Chen [:nechen] from comment #6)
> Maybe it happens when user sync bookmarks with empty urls.

Oh, yeah, that's a very good point. The URL is probably not null but an empty string. IconRequestBuilder uses TextUtils.isEmpty() and therefore will complain about empty strings too:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java#122

In this case we should exit early (and remove an existing icon) in TwoLinePageRow, I guess.

Setting this to track beta: We should uplift a patch if it is not risky.

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1310622 - Display the item as-is but do not load an icon if we do not have a page URL

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

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8817624 [details]
Bug 1310622 -  Display the item as-is but do not load an icon if we do not have a page URL

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97844/diff/1-2/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8817624 [details]
Bug 1310622 -  Display the item as-is but do not load an icon if we do not have a page URL

https://reviewboard.mozilla.org/r/97844/#review98464

This patch seems to implemen two strategies:
1) Not selecting bookmarks with empty URLs from the database
2) Hiding items with an empty URL

Shouldn't only one of them be enough?

I wonder if we should make only a small change here actually: Display the item as-is but do not load an icon if we do not have a page URL. This prevents the crash and the user can still see, delete and edit (not implement yet) the bookmark with empty URL.

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8817624 [details]
Bug 1310622 -  Display the item as-is but do not load an icon if we do not have a page URL

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97844/diff/2-3/

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8817624 [details]
Bug 1310622 -  Display the item as-is but do not load an icon if we do not have a page URL

https://reviewboard.mozilla.org/r/97844/#review99386

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:279
(Diff revision 3)
>          // remove the about:reader prefix to ensure the Favicon loads properly.
>          final String pageURL = ReaderModeUtils.stripAboutReaderUrl(url);
>  
> -        if (bookmarkId < BrowserContract.Bookmarks.FAKE_PARTNER_BOOKMARKS_START) {
> +        if (TextUtils.isEmpty(pageURL)){
> +            // If url is empty, display the item as-is but do not load an icon if we do not have a page URL (bug 1310622)
> +        }else if (bookmarkId < BrowserContract.Bookmarks.FAKE_PARTNER_BOOKMARKS_START) {

nit: style: Space after '}'.

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8817624 [details]
Bug 1310622 -  Display the item as-is but do not load an icon if we do not have a page URL

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97844/diff/3-4/

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Any reason why we didn't land this yet?

Comment 15

[Nevin Chen(Not active on Bugzilla)]

(In reply to Sebastian Kaspari (:sebastian) from comment #14)
> Any reason why we didn't land this yet?

Sorry.my bad. I'll fix the style tomorrow when I get into the office

Comment 16

[Nevin Chen(Not active on Bugzilla)]

I forgot the style has been fixed already so I just marked "checkin-needed"

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a271a9a04201
Display the item as-is but do not load an icon if we do not have a page URL r=sebastian

Comment 18

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

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

Comment 19

[Gerry Chang [:gchang]]

Too late for 51. Mark 51 won't fix. 
Hi :nechen,
Is this worth uplifting to Aurora52?

Comment 20

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8817624 [details]
Bug 1310622 -  Display the item as-is but do not load an icon if we do not have a page URL

Approval Request Comment
[Feature/Bug causing the regression]:When a bookmark's url is empty, there will be a crash
[User impact if declined]: User will have unexpected crashes.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no. 
[Why is the change risky/not risky?]: It checks for empty/null string. Just make it more secure.
[String changes made/needed]: no

Comment 21

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8817624 [details]
Bug 1310622 -  Display the item as-is but do not load an icon if we do not have a page URL

fennec crash fix, aurora52+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8678c828e53