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

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
General
P1
critical
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: sebastian, Assigned: nechen)

Tracking

({crash})

unspecified
Firefox 53
All
Android
crash
Points:
---

Firefox Tracking Flags

(fennec51+, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-653b4c36-f0da-4e8f-b771-14caa2161012.
=============================================================
(Reporter)

Comment 1

a year ago
> 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)
> [..]
(Reporter)

Comment 2

a year ago
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.
(Reporter)

Comment 3

a year ago
So far this hasn't shown up anymore.
Assignee: s.kaspari → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 4

11 months ago
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 ?
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 5

11 months ago
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/
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 6

11 months ago
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.
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 7

11 months ago
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.
Assignee: nobody → cnevinchen
Status: NEW → ASSIGNED
tracking-fennec: --- → 51+
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
Flags: needinfo?(s.kaspari)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

10 months ago
mozreview-review
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.
Attachment #8817624 - Flags: review?(s.kaspari)
Comment hidden (mozreview-request)
(Reporter)

Comment 12

10 months ago
mozreview-review
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 '}'.
Attachment #8817624 - Flags: review?(s.kaspari) → review+
Comment hidden (mozreview-request)
Priority: -- → P1
(Reporter)

Comment 14

10 months ago
Any reason why we didn't land this yet?
Flags: needinfo?(cnevinchen)
(Assignee)

Comment 15

10 months ago
(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
Flags: needinfo?(cnevinchen)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
(Assignee)

Comment 16

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

Comment 17

10 months ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 18

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a271a9a04201
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Too late for 51. Mark 51 won't fix. 
Hi :nechen,
Is this worth uplifting to Aurora52?
status-firefox51: affected → wontfix
Flags: needinfo?(cnevinchen)
(Assignee)

Comment 20

9 months ago
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
Flags: needinfo?(cnevinchen)
Attachment #8817624 - Flags: approval-mozilla-aurora?
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+
Attachment #8817624 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 22

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8678c828e53
status-firefox52: affected → fixed
You need to log in before you can comment on or make changes to this bug.