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

RESOLVED FIXED in Firefox 36



5 years ago
3 years ago


(Reporter: mfinkle, Assigned: esawin)



Firefox 36

Firefox Tracking Flags

(Not tracked)



(2 attachments, 4 obsolete attachments)

OS: Linux → Android
Hardware: x86_64 → All
See Also: → bug 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!

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:

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:

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

Comment 7

4 years ago
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.

Comment 8

4 years ago
Created attachment 8501787 [details] [diff] [review]

We never delete (we do remove it, but keep the reference) the listener for the "mozfullscreenchange" events, so this patch fixes it.

Comment 9

4 years ago
Created attachment 8501789 [details] [diff] [review]
(WIP) orientation-lock-mapping.patch

GeckoScreenOrientation.lock expects an Android screen orientation type, but gets called with the Gecko-internal type, this should fix it.

Comment 10

4 years ago
The crashing on lock is caused by the type mismatch, so these patches will fix bug 1061373, too.
Assignee: nobody → esawin
Blocks: 1061373


4 years ago
Attachment #8501787 - Attachment description: (WIP) orientation-unlock-crash.patch → orientation-unlock-crash.patch
Attachment #8501787 - Flags: review?(mounir)

Comment 11

4 years ago
Created attachment 8501810 [details] [diff] [review]
Attachment #8501789 - Attachment is obsolete: true
Attachment #8501810 - Flags: review?(snorp)

Comment 12

4 years ago
Created attachment 8501813 [details] [diff] [review]

Fixed comments.
Attachment #8501810 - Attachment is obsolete: true
Attachment #8501810 - Flags: review?(snorp)
Attachment #8501813 - Flags: review?(snorp)


4 years ago
No longer blocks: 1061373


4 years ago
Blocks: 1061373
Comment on attachment 8501813 [details] [diff] [review]

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

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

::: mobile/android/base/
@@ +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-

Comment 14

4 years ago
Created attachment 8504113 [details] [diff] [review]

Replaced the switch statement.
Attachment #8501813 - Attachment is obsolete: true
Attachment #8504113 - Flags: review?(snorp)
Comment on attachment 8504113 [details] [diff] [review]

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

::: mobile/android/base/
@@ +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+

Comment 16

4 years ago
Created attachment 8504129 [details] [diff] [review]

Just added a static variable prefix.
Attachment #8504113 - Attachment is obsolete: true
Attachment #8504129 - Flags: review+


4 years ago
Attachment #8501787 - Flags: review?(mounir) → review?(blassey.bugs)
Attachment #8501787 - Flags: review?(blassey.bugs) → review+
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36


4 years ago
Duplicate of this bug: 1061373
You need to log in before you can comment on or make changes to this bug.