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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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

Comment 4

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

Attachment: Screenshot of blank tile in activity stream

Here's what it looks like when we insert a blank tile.

Comment 5

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

Attachment: Bug 1403755: Rm code to insert blanks into top sites.

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
planning to ship AS, this shouldn't matter.

If we wanted to preserve support, we could create a branch but deleting the
code is much simpler.

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

Comment 6

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

I filed bug 1404110 for the question mark/no title issue.

Comment 7

[Chenxia Liu [:liuche]]

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.

Comment 8

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

#19 top crasher on 57.0b4: 8 crashes since the beta was released (I'm not sure when this was).

Comment 9

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

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.

Comment 10

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

Comment on attachment 8913409 [details]
Bug 1403755: Rm code to insert blanks into top sites.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ba3c0e7b8f7
Rm code to insert blanks into top sites. r=liuche

Comment 12

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

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

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8ba3c0e7b8f7

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8913409 [details]
Bug 1403755: Rm code to insert blanks into top sites.

Fixes a medium volume Fennec crash, Beta57+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca0a6d284f98