Closed Bug 1061372 Opened 10 years ago Closed 10 years ago

Crash with any value passed to mozLockOrientation() but portrait-primary and portrait-secondary

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: mfinkle, Assigned: esawin)

References

Details

(Keywords: crash)

Attachments

(2 files, 4 obsolete files)

OS: Linux → Android
Hardware: x86_64 → All
See Also: → 1061373
09-01 19:16:11.909 D/GeckoScreenOrientation( 5721): locking to NONE
09-01 19:16:11.909 D/GeckoScreenOrientation( 5721): updating to new orientation NONE
09-01 19:16:12.029 D/MediaRouterService(  751): Client org.mozilla.fennec_rnewman (pid 5721): Died!


OOPS
Severity: normal → critical
Keywords: crash
Maybe I'm crazy for asking such a question, but why on earth do we even have a NONE condition?
When first added, we did not actually try to change the orientation to NONE:
http://hg.mozilla.org/mozilla-central/rev/8bf3120ed0f8

But refactorings at some point have introduced the problem.
NONE wasn't present in the original Bug 720795. It was introduced in Bug 740190. Bug 971012 moved this stuff out of GeckoAppShell.

Perhaps Mounir or Doug can remember why we have a NONE in the first place?
(In reply to Mark Finkle (:mfinkle) from comment #3)
> When first added, we did not actually try to change the orientation to NONE:
> http://hg.mozilla.org/mozilla-central/rev/8bf3120ed0f8

Actually, we did set the orientation when using NONE in that patch:

>+      case eScreenOrientation_None:
>+        orientation = ActivityInfo.SCREEN_ORIENTATION_SENSOR;
>+        break;

But now we do:

>       case DEFAULT:
>       case NONE:
>           return ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED;
>       default:
>           return ActivityInfo.SCREEN_ORIENTATION_NOSENSOR;
bug 757791 and bug 766904 tweak it into our current situation
Looks like there are multiple things going wrong here.

There are 4 different orientation representations, the DOM strings (e.g. "portrait-primary", "landscape-secondary"), Gecko screen orientations (eScreenOrientation_*) and two Android types (ActivityInfo.SCREEN_ORIENTATION_* and Configuration.ORIENTATION_*).

It happens that except for the DOM strings, all of the representations are integer constants, so type-safe interfaces are not an option.
This lead to a type mismatch when interfacing with GeckoScreenOrientation.lock. GeckoScreenOrientation has to speak all of it, since it processes requests with Gecko and Android types.

Additionally, we don't seem to support orientation lock *sets*, so we should probably fallback to the primary options for "portrait" and "landscape" respectively.
We never delete (we do remove it, but keep the reference) the listener for the "mozfullscreenchange" events, so this patch fixes it.
GeckoScreenOrientation.lock expects an Android screen orientation type, but gets called with the Gecko-internal type, this should fix it.
The crashing on lock is caused by the type mismatch, so these patches will fix bug 1061373, too.
Assignee: nobody → esawin
Blocks: 1061373
Status: NEW → ASSIGNED
Attachment #8501787 - Attachment description: (WIP) orientation-unlock-crash.patch → orientation-unlock-crash.patch
Attachment #8501787 - Flags: review?(mounir)
Attached patch orientation-lock-mapping.patch (obsolete) — Splinter Review
Attachment #8501789 - Attachment is obsolete: true
Attachment #8501810 - Flags: review?(snorp)
Attached patch orientation-lock-mapping.patch (obsolete) — Splinter Review
Fixed comments.
Attachment #8501810 - Attachment is obsolete: true
Attachment #8501810 - Flags: review?(snorp)
Attachment #8501813 - Flags: review?(snorp)
No longer blocks: 1061373
Blocks: 1061373
Comment on attachment 8501813 [details] [diff] [review]
orientation-lock-mapping.patch

Review of attachment 8501813 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly alright, but that one switch block needs fixed up.

::: mobile/android/base/GeckoScreenOrientation.java
@@ +44,4 @@
>              switch (value) {
>                  case (1 << 0): return PORTRAIT_PRIMARY;
>                  case (1 << 1): return PORTRAIT_SECONDARY;
> +                case ((1 << 0) | (1 << 1)): return PORTRAIT;

There has to be a better way to do this. Can't we just use ScreenOrientation.PORTRAIT? What is the point of having the enum if we just use the raw values?
Attachment #8501813 - Flags: review?(snorp) → review-
Attached patch orientation-lock-mapping.patch (obsolete) — Splinter Review
Replaced the switch statement.
Attachment #8501813 - Attachment is obsolete: true
Attachment #8504113 - Flags: review?(snorp)
Comment on attachment 8504113 [details] [diff] [review]
orientation-lock-mapping.patch

Review of attachment 8504113 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoScreenOrientation.java
@@ +45,5 @@
> +        public static ScreenOrientation get(int value) {
> +            for (ScreenOrientation orient: ScreenOrientation.values) {
> +                if (orient.value == value) {
> +                    return orient;
> +                }

That is a huge improvement, thanks.
Attachment #8504113 - Flags: review?(snorp) → review+
Just added a static variable prefix.
Attachment #8504113 - Attachment is obsolete: true
Attachment #8504129 - Flags: review+
Attachment #8501787 - Flags: review?(mounir) → review?(blassey.bugs)
Attachment #8501787 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/4de85788d5a6
https://hg.mozilla.org/mozilla-central/rev/317187728774
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.