Closed Bug 1403755 Opened 2 years ago Closed 2 years ago

Crash in java.lang.IllegalStateException: Unknown top site type: 0 at org.mozilla.gecko.activitystream.ActivityStreamTelemetry$Extras$Builder.forTopSite(ActivityStreamTelemetry.java)

Categories

(Firefox for Android :: Activity Stream, defect, P1, critical)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-088ed19a-08ad-413c-9b3a-771880170927.
=============================================================
java.lang.IllegalStateException: Unknown top site type: 0
	at org.mozilla.gecko.activitystream.ActivityStreamTelemetry$Extras$Builder.forTopSite(ActivityStreamTelemetry.java:212)
	at org.mozilla.gecko.activitystream.homepanel.topsites.TopSitesCard$1.onLongClick(TopSitesCard.java:69)
	at android.view.View.performLongClick(View.java:5246)
	at android.view.View$CheckForLongPress.run(View.java:21142)
	at android.os.Handler.handleCallback(Handler.java:746)

This bug is the #13 top crasher for 57 beta: it's crashed 14 times since the beta was released.

---

Marking P1 because we P1'd all other crashes so far, including those that were less often.
We're hitting this [1]:

                switch (topSite.getType()) {
                    ...
                    // While we also have a "blank" type, it is not used by Activity Stream.
                    case BrowserContract.TopSites.TYPE_BLANK:
                    default:
                        throw new IllegalStateException("Unknown top site type: " + topSite.getType());

Which is mostly true – TYPE_BLANK is added in code for `getTopSites` but not `getActivityStreamTopSites`. However, we also insert TYPE_BLANK into our temporary table for our top sites query, which is used by both code paths [2]. I'll investigate under which cases we might make this insertion.

[1]: http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java#212
[2]: http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java#1171
The number of TYPE_BLANK sites we insert is basically, `max top site position + 1 - count(all-pinned-sites) - count(all-top-sites)`.

To reproduce this, I've been trying to build a build before Activity Stream was fully enabled, pin a top site to position 6, delete all other top sites, turn on Activity Stream, and see if I can reproduce the crash. However, I've been unable to create the build because of crashes on startup that I can't figure out. x_x I will continue working on it...
I was able to reproduce with the STR in comment 2 (I built the latest and switched ActivityStream.isEnabled to false). Looking for a fix...
Here's what it looks like when we insert a blank tile.
I filed bug 1404110 for the question mark/no title issue.
Comment on attachment 8913409 [details]
Bug 1403755: Rm code to insert blanks into top sites.

https://reviewboard.mozilla.org/r/184740/#review190812

Should we add a log+break into the switch statement that falls through to an exception, or do we want to keep hitting that case (in case we find more problems)?

::: commit-message-756e1:7
(Diff revision 1)
> +
> +This code was being mistakenly activated when getting top sites for Activity
> +Stream.
> +
> +This is the first removal of old top sites code and will mean we can't go back
> +to old top sites by flipping the `ActivityStream.isEnabled` flag. Since we're

Should we remove this flag (and the preference controlling this, unless it's already been removed)? My concern is that if we're not backward-compatible anymore keeping the master switch around can lead to more crashes.

::: commit-message-756e1:10
(Diff revision 1)
> +
> +This is the first removal of old top sites code and will mean we can't go back
> +to old top sites by flipping the `ActivityStream.isEnabled` flag. Since we're
> +planning to ship AS, this shouldn't matter.
> +
> +If we wanted to preserve support, we could create a branch but deleting the

My understanding is that we are not going to provide backwards-compatible support.
Attachment #8913409 - Flags: review?(liuche) → review+
#19 top crasher on 57.0b4: 8 crashes since the beta was released (I'm not sure when this was).
Comment on attachment 8913409 [details]
Bug 1403755: Rm code to insert blanks into top sites.

https://reviewboard.mozilla.org/r/184740/#review190812

I'm generally in favor of crashing over logging (considering we don't have a logger which collects and reports exceptional logs):
- You can find problems quickly before they compound and make the situation worse
- If you continue your program in an undefined state, your users can have a bad experience and, in the worst case, there could be data loss for them.

> Should we remove this flag (and the preference controlling this, unless it's already been removed)? My concern is that if we're not backward-compatible anymore keeping the master switch around can lead to more crashes.

This should be covered by bug 1386725. Its existence won't lead to crashes because users can't adjust it. However, developers will have undefined behavior when flipping the switch so they may not be able to use it to test the upgrade code path like I did here. :( I'll add a comment.

> My understanding is that we are not going to provide backwards-compatible support.

Since we haven't officially started removing the code yet (bug 1386725), I wasn't sure if we would want to preserve functionality until then but I can see why this is confusing and really being a little too perfect: I'll remove the comment.
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8ba3c0e7b8f7
Rm code to insert blanks into top sites. r=liuche
Comment on attachment 8913409 [details]
Bug 1403755: Rm code to insert blanks into top sites.

Approval Request Comment
[Feature/Bug causing the regression]: Activity Stream.
[User impact if declined]: Users upgrading from the old top sites who added custom tiles in a position besides one may have a blank tile on the top sites page (screenshot: [1]) which will crash the browser when they try to click it. You can remove it by removing other pinned sites but this is not intuitive.

[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Ideally Bogdan can help. Rough STR in comment 2. As steps, this might be:
1) Install Firefox 56 (i.e. no activity stream)
2) Add a top site to one of the later positions (i.e. bottom right)
3) Remove other top sites (there should be a blank tile, a +, in the top left)
4) Upgrade to Firefox 57
5) Open top sites. There should be no blank tile after this patch.

[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: We'd insert blank tiles into the top sites temporary table when it was relevant in old top sites. We accidentally used this code for the new top sites so I just removed it. The change is rather simple (don't run the insert query!) but the top sites query code in its entirety is complex, which is where the risk comes from because maybe I missed something.

[String changes made/needed]: None

[1]: https://bug1403755.bmoattachments.org/attachment.cgi?id=8913403
Attachment #8913409 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/8ba3c0e7b8f7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8913409 [details]
Bug 1403755: Rm code to insert blanks into top sites.

Fixes a medium volume Fennec crash, Beta57+
Attachment #8913409 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.