Closed Bug 1545429 Opened 5 years ago Closed 5 years ago

Crash in [@ InternalOrientationToType ]

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- fixed
firefox66 --- wontfix
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: marcia, Assigned: hsivonen)

References

Details

(Keywords: crash, regression, Whiteboard: [geckoview:p1] [fennec68.1])

Crash Data

Attachments

(1 file)

We tried a speculative fix for this in Bug 1493980, but it appears this crash is still hanging around in the 66 release top 15 with over 3700 crashes on release. Looking back at 6 months of data, it appears the numbers were reduced compared to the 65 release.

(100.0% in signature vs 00.99% overall) moz_crash_reason = MOZ_CRASH(Bad aOrientation value)

Not sure if there is anything else we can do, but wanted to make sure we still had a bug on our radar since this is the top crash realm for Fennec.

Adjusting the summary. Apparently when I cloned the bug it lost the brackets.

Summary: Crash in InternalOrientationToType → Crash in [@ InternalOrientationToType ]

Matt, can you take a look since you worked on bug 1493980?

Flags: needinfo?(mbrubeck)
Priority: P2 → P1
Assignee: nobody → mbrubeck
Flags: needinfo?(mbrubeck)

The volume of crashes in 67 seems to indicate that this won't be as much a problem in 67 than it is currently in 66, so marking as fic-optional for 67 as we are one week away from RC week.

In early 67 returns this is #4 overall. When we unthrottle we can check where we are in terms of volume.

Assignee: mbrubeck → nobody

This continues to be the #4 overall top crash in 67 release. Hsin-Yi - Since it affects 68 as well can we get someone to take a look? Thanks.

Flags: needinfo?(htsai)

Hi snorp, I feel like we need your assistance here as the crash seemed from the changes made in the Java-side for handling/sending orientation values, referring to previous discussion on bug 1493980 comment 3. Would your team please take a look?

Flags: needinfo?(htsai) → needinfo?(snorp)

Adding [geckoview] so it gets on the triage list.

Flags: needinfo?(snorp)
Whiteboard: [geckoview]

ScreenOrientation is a bitflag type, but InternalOrientationToType treats it as an enum. InternalOrientationToType also does not explicitly handle eScreenOrientation_None.

This is Fennec 68 Beta top crash #17.

Hsin-Yi, can a DOM engineer take a look at this Android bug? It has been a steady top crash in Fennec Android for at least a year. The fix should be pretty straightforward.

Flags: needinfo?(htsai)
Whiteboard: [geckoview] → [geckoview:p1]

Henri, could you please help us?

Flags: needinfo?(htsai) → needinfo?(hsivonen)

(In reply to Chris Peterson [:cpeterson] from comment #8)

ScreenOrientation is a bitflag type, but InternalOrientationToType treats it as an enum. InternalOrientationToType also does not explicitly handle eScreenOrientation_None.

We can get None from at least:
https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/widget/android/AndroidBridge.cpp#671

Additionally, Java code accommodates Gecko expectations at:
https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoScreenOrientation.java#165
But first saves the original value in mScreenOrientation:
https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoScreenOrientation.java#165

Which can be exposed to Gecko via:
https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoScreenOrientation.java#218
https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java#1674
https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/widget/android/AndroidBridge.cpp#671
(The last one only checks for zero and not for combined flags).

Let's try collapsing the value set on the Java side earlier:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aeed97f429809a4d66266154b416cecca8dd4d47

Flags: needinfo?(hsivonen)

I don't know how to reproduce the problem or how to write a test case, but by code inspection, I expect the patch to fix this.

Assignee: nobody → hsivonen
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2521756a55f0
Avoid passing unsupported Android screen orientation flag combinations to Gecko. r=snorp

Care to request uplift to esr68 so we can fix this in fennec?

Flags: needinfo?(hsivonen)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Comment on attachment 9074833 [details]
Bug 1545429 - Avoid passing unsupported Android screen orientation flag combinations to Gecko.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Avoid crash on what's effectively a release assertion.
  • User impact if declined: More crashes.
  • Fix Landed on Version: 60
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This change should limit the possible values that Java code can pass to C++, but no new values should be introduced.
  • String or UUID changes made by this patch: None
Flags: needinfo?(hsivonen)
Attachment #9074833 - Flags: approval-mozilla-esr68?

Comment on attachment 9074833 [details]
Bug 1545429 - Avoid passing unsupported Android screen orientation flag combinations to Gecko.

Fixes a Fennec crash. Approved for Fennec 68.1b1.

Attachment #9074833 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Approved for Fennec 68.1b1.

Whiteboard: [geckoview:p1] → [geckoview:p1] [fennec68.1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: