Closed Bug 1402145 Opened 2 years ago Closed 2 years ago

Crash in HighlightsDivider

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 58
Iteration:
1.30
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: liuche, Assigned: liuche)

References

Details

(Whiteboard: [MobileAS])

Crash Data

Attachments

(1 file, 1 obsolete file)

This is fallout from bug 1400950, which updated how we calculated the highlight divider.

Looks like either all children are not RowItemTypes, or the model needs to be invalidated and so the numbers are mismatched.

I don't have a 100% reliable STR, but this is happening when the Highlight Empty View is being shown/hidden dynamically (either through pushing things into highlights or enabling/disabling empty highlights from Settings).

09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: java.lang.IllegalArgumentException: Requested position does not exist
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter.getItemViewType(StreamRecyclerAdapter.java:123)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at org.mozilla.gecko.activitystream.homepanel.HighlightsDividerItemDecoration.onDraw(HighlightsDividerItemDecoration.java:54)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.support.v7.widget.RecyclerView.onDraw(RecyclerView.java:3373)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.view.View.draw(View.java:17185)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.support.v7.widget.RecyclerView.draw(RecyclerView.java:3308)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.view.View.updateDisplayListIfDirty(View.java:16167)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.view.View.draw(View.java:16951)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.view.ViewGroup.drawChild(ViewGroup.java:3727)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.view.ViewGroup.dispatchDraw(ViewGroup.java:3513)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.view.View.draw(View.java:17188)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.view.View.updateDisplayListIfDirty(View.java:16167)
09-21 15:40:40.821  6981  6981 E GeckoCrashHandler: 	at android.view.View.draw(View.java:16951)
Assignee: nobody → liuche
Comment on attachment 8911231 [details]
Bug 1402145 - Use adapter child position rather than view position.

https://reviewboard.mozilla.org/r/182724/#review188002

I don't think this fixes the crash and the change as is seems unnecessary given that it's already handled (albeit implicitly).

`java.lang.IllegalArgumentException: Requested position does not exist` is the error, but this is thrown when `if (position >= recyclerViewModel.size()) {`, which will never happen for NO_POSITION because NO_POSITION is -1 and size is at minimum 0.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:45
(Diff revision 1)
>  
>          final int childCount = parent.getChildCount();
>          for (int i = 0; i < childCount; i++) {
>              final View child = parent.getChildAt(i);
>  
>              if (parent.getChildAdapterPosition(child) < START_DRAWING_AT_POSITION) {

This would cover the NO_POSITION case implicitly, but it'd probably be better to do it explicitly here.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:54
(Diff revision 1)
>              if (child.getVisibility() == View.GONE) {
>                  continue;
>              }
>  
>              // Do not draw dividers above section title items.
> -            final int childViewType = parent.getAdapter().getItemViewType(i);
> +            final int position = parent.getChildAdapterPosition(child);

That being said, I don't think NO_POSITION should ever happen: we're iterating through the children of the RecyclerView, which should all be in the adapter, provided we didn't manually add a child to the RecyclerView (which sounds overly defensive to me).

This is assuming `onDraw` will never be called at a time when the RecyclerView and adapter are in inconsistent states.
Attachment #8911231 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8911232 [details]
Bug 1402145 - Reverse incorrect sign of margin calculation.

https://reviewboard.mozilla.org/r/182726/#review188004

This patch is unrelated to the crash and doesn't seem to entirely fix the problem it's trying to solve, assuming we don't want to be drawing into child views. Happy to reverse my r- to r+ if we do want to draw into child views but this should probably be done in a different bug so the separation of concerns are clear when we try to uplift.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:68
(Diff revision 1)
>              }
>  
>              final RecyclerView.LayoutParams params = (RecyclerView.LayoutParams) child
>                      .getLayoutParams();
> -            final int topOfDivider = child.getTop() + params.topMargin;
> +            final int topOfDivider = child.getTop() - params.topMargin;
>              final int bottomOfDivider = topOfDivider + divider.getIntrinsicHeight();

We're drawing the divider into the child view - is that correct? Don't we want to draw the dividers between the child views? Currently, we start at the top border of the child and draw divider.getInstrinsicHeight units down.

Assuming we want to draw dividers above the child (which I haven't thought about why that might be vs. below), I think we want:

```
bottomOfDivider = child.getTop() - params.topMargin;
topOfDivider = bottomOfDivider - divider.getInstrinsicHeight()
```
Attachment #8911232 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8911232 [details]
Bug 1402145 - Reverse incorrect sign of margin calculation.

https://reviewboard.mozilla.org/r/182726/#review188004

Probably related to this issue, Sebastian noticed a divider cuts directly into the highlights empty state section: https://bug1400825.bmoattachments.org/attachment.cgi?id=8911243
Comment on attachment 8911231 [details]
Bug 1402145 - Use adapter child position rather than view position.

https://reviewboard.mozilla.org/r/182724/#review188052

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/HighlightsDividerItemDecoration.java:59
(Diff revision 1)
> -            final int childViewType = parent.getAdapter().getItemViewType(i);
> +            final int position = parent.getChildAdapterPosition(child);
> +            if (position == RecyclerView.NO_POSITION) {
> +                continue;
> +            }
> +
> +            final int childViewType = parent.getAdapter().getItemViewType(position);

Sorry, I missed this line – it should fix the issue.
Attachment #8911231 - Flags: review- → review+
Attachment #8911232 - Attachment is obsolete: true
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/722e1226aa7a
Use adapter child position rather than view position. r=mcomella
Comment on attachment 8911231 [details]
Bug 1402145 - Use adapter child position rather than view position.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1400950
[User impact if declined]: Crash when entering/leaving state with no highlights on activity stream newtab page
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: local testing
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no, fixes crash by using the correct index into list of view children
[Why is the change risky/not risky?]: Was using the wrong index
[String changes made/needed]: none
Attachment #8911231 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/722e1226aa7a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8911231 [details]
Bug 1402145 - Use adapter child position rather than view position.

Fix a crash, taking it.
Should be in 57b3
Attachment #8911231 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Crash Signature: [@ java.lang.IllegalArgumentException: Requested position does not exist at org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter.getItemViewType(StreamRecyclerAdapter.java)]
There are some crashes in beta 57, they appeared in 57.0b3 (buildid 20170926071548) so just after the patch landed (see[1]).

[1] http://bit.ly/2zh7wzI
Status: RESOLVED → REOPENED
Flags: needinfo?(liuche)
Resolution: FIXED → ---
(In reply to Calixte Denizet (:calixte) from comment #14)
> There are some crashes in beta 57, they appeared in 57.0b3 (buildid
> 20170926071548) so just after the patch landed (see[1]).

This is bug 1402327, which we decided was not a high priority issue (given AS has no full-time resources).
Flags: needinfo?(liuche)
If the remaining crashes are tracked in 1402327, let's close this one.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.