1493980 - Crash in InternalOrientationToType

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-833c5b4a-0f14-4f90-b23b-6f3e70180925.
=============================================================

Seen while looking at beta Android crashes: https://bit.ly/2QWriXG. These crashes spiked a bit in 63b5. They are present in 62 as well, with ~49 crashes. Mix of devices ranging from API 28 down to API 19.

moz_crash_reason = MOZ_CRASH(Bad aOrientation value)

Top 10 frames of crashing thread:

0 libxul.so InternalOrientationToType dom/base/ScreenOrientation.cpp:49
1 libxul.so mozilla::dom::ScreenOrientation::ScreenOrientation dom/base/ScreenOrientation.cpp:80
2 libGLES_mali.so libGLES_mali.so@0xe3058 
3 libxul.so nsScreen::nsScreen dom/base/nsScreen.cpp:40
4 libxul.so nsScreen::Create dom/base/nsScreen.cpp:34
5 libxul.so mbrtoc32 
6 libGLES_mali.so libGLES_mali.so@0xeb50e 
7 libxul.so nsGlobalWindowInner::GetScreen dom/base/nsGlobalWindowInner.cpp:2234
8 libGLES_mali.so libGLES_mali.so@0xeb50e 
9 libxul.so mozilla::dom::Window_Binding::get_screen dom/bindings/WindowBinding.cpp:3132

=============================================================

Comment 1

[Marcia Knous [:marcia]]

Although the spike in b5 is noticeable, it appears only 5 installations were causing that amount of crashes. The crash continues into other betas but in fairly low volume.

Comment 2

[Andrew Overholt [:overholt]]

snorp, do you know if things have changed recently in the Java-side for handling/sending orientation values?

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

We did change some stuff in bug 1476106 and followups. Jan, do you feel like taking a look at this?

Comment 4

[Jan Henning [:JanH]]

I can try taking a look.

Comment 5

[Jan Henning [:JanH]]

I won't have the time to look at this in the near future, though, so if someone else wants to investigate feel free.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

It's closer to 1000 crashes/600 installs per week on Fennec release. 
Fix-optional for 64 since we're getting close to the end of the cycle; we could still take a patch for 65.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1493980 - Don't pass indefinite screen orientations to Gecko. r?snorp

This eliminates one potential source of crashes from passing bad orientation
values to onOrientationChange.

Comment 8

[Matt Brubeck (:mbrubeck)]

I can't reproduce this crash, so this patch is speculative, based on one possible place where a bad value might slip through.

Comment 9

[Jan Henning [:JanH]]

Thanks for looking at this.

Comment 10

[Matt Brubeck (:mbrubeck)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2379f60e513222f07057efb7cff10dea7acf6061

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/972a66d8aab1
Don't pass indefinite screen orientations to Gecko. r=snorp

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/972a66d8aab1

Comment 13

[Julien Cristau [:jcristau]]

Might be worth fixing in 65 still?

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 9035665 [details]
Bug 1493980 - Don't pass indefinite screen orientations to Gecko. r?snorp

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1476106

User impact if declined: Crashes in Firefox for Android, at a rate of about 1000/week. This patch isn't known to eliminate these crashes, but it might.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Very low-risk patch. Small, Android-only, and it just adds some validation to prevent "should never happen" values from being passed to code that can't handle them.

String changes made/needed:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9035665 [details]
Bug 1493980 - Don't pass indefinite screen orientations to Gecko. r?snorp

[Triage Comment]
Low-risk speculative fix for a longstanding crash. Approved for 65.0b12.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1cf642fe06ab