808262 - Otoro/unagi sometimes start up in landscape and can't be switched to portrait

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Power off otoro (or $ adb shell stop b2g)
 (2) Set on its side so that it's in landscape orientation
 (3) Power it back on (or $ adb shell start b2g)

Many times it starts in landscape and can't be switched to portrait.

Not clear whether this is a FE or widget bug yet.

Comment 1

[Dave Hylands [:dhylands]]

I've also had this happen while my otoro was lying flat on my desk.

Comment 3

[Andreas Gal :gal]

This happens to me on unagi.

Comment 4

[Andreas Gal :gal]

I am pretty sure this is a widget bug.

Comment 5

[Jason Smith [:jsmith]]

Removing qawanted per comment 3 - looks like this issue can be reproduced.

Comment 6

[Andreas Gal :gal]

I think this goes wrong in DetectDefaultOrientation() in the gonk widget.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Sometimes we get zero sized screen. It might be related to the unable to open framebuffer issue when restarting b2g that we used to have. Though now I can't reproduce this.

Comment 8

[Doug Turner (:dougt)]

I think that this is just initialized variables.

GetRotation() is being called and can return something random (sScreenRotation in nsWindow is not initialized).  if it returns 0, 90, 180, or 270 things are okay -- you might see the screen in landscape then switch to portrait.  However, if the return is anything else, we get stuck in landscape for the life of gecko.

While I am here, i am also going to set the other uninitialized variables to something reasonable.

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 686949 [details] [diff] [review]
patch v.1

These are translation-unit-static variables that will always be zeroed out by the loader.  (We rely on that in more places than I could count.)  I suspect the real bug lies elsewhere.

Comment 11

[Andrew Overholt [:overholt]]

Marking P2 until dhylands' investigation is complete.  This may lose its blocking status if it ends up being only something that can happen to developers.

Comment 12

[Dave Hylands [:dhylands]]

After looking at the code last night, I thought I may have found a way to reproduce this, but so far no luck.

Comment 13

[Doug Turner (:dougt)]

i attempted to reproduce this failure with logging.  Overnight, I stop/started b2g 994 times and recorded values in DetectDefaultOrientation and of sScreenRotation.  Everything looked the same. :(

However I did see this problem once yesterday testing something else out.

Dave, what was your thinking?

Comment 14

[Dave Hylands [:dhylands]]

So I was thinking that if the screen size gets queried as zero, then this will cause it to think that the default orientation is landscape, due to this test in nsWindow.cpp ComputeOrientation function:

bool naturallyPortrait = (aScreenSize.height > aScreenSize.width);

And the one way might be if the fb failed to open, which we've seen in the stop/start b2g. The Framebuffer::GetSize seems to open the framebuffer independently from the regular open, so maybe there's a race of some type happening where the open in GetSize fails but the regular open succeeds.

Comment 15

[Dave Hylands [:dhylands]]

I saw it again today as well (while trying to do something else). I was playing with the testupdate script which effectively does: adb shell 'stop b2g; start b2g'

One thing I did notice was that the mozilla splash screen (the one with the word mozilla in white in the bottom right corner) was also rotated.

Comment 16

[Dave Hylands [:dhylands]]

This test in Framebuffer::GetSize looks suspicious (but not likely to be causing the problem)

    if (0 <= sFd || sScreenSize) {
        *aScreenSize = *sScreenSize;
        return true;
    }

This will cause a segfault if sScreenSize == NULL and sFd > 0. I know this condition shouldn't ever happen, but shouldn't the test just be:

if (sScreenSize) {

I also discovered that Framebuffer::GetSize is called from multiple threads (MainThread and the Compositor thread) and this code doesn't appear to have any thread safety contained within

Comment 17

[Damon Sicore (:damons)]

What's next here?  Dougt, do you have a patch to finish here?

Comment 18

[Doug Turner (:dougt)]

exceptionally hard to reproduce reliability.  Consider this is exclusively a startup issue, and there is some evidence that this is related to the call to GetSize() failing (the fb can't be opened), we could just runtime abort and try again.

dave, what critical section in GetSize are you worried about?

Comment 19

[Doug Turner (:dougt)]

Attachment: bandaide v.1

Comment 20

[Doug Turner (:dougt)]

adding qa-wanted.  we need help being able to reproduce this.

Comment 21

[Dave Hylands [:dhylands]]

Comment on attachment 689586 [details] [diff] [review]
bandaide v.1

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

::: widget/gonk/nsWindow.cpp
@@ +153,5 @@
>          nsIntSize screenSize;
> +        bool gotFB = Framebuffer::GetSize(&screenSize);
> +        if (!gotFB) {
> +            NS_RUNTIMEABORT("Failed to get size from framebuffer, aborting...");
> +        }

Shouldn't this test go inside the GetSize function?

GetSize is called from at least 2 different threads.

Comment 22

[Doug Turner (:dougt)]

Yeah, I am not sure.  Some callers of GetSize() might be able to handle the failure more gracefully.  So, instead of just aborting inside GetSize(), I added the check to the one place where I have seen the failure.

Comment 23

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/cf9284647b0b
https://hg.mozilla.org/releases/mozilla-aurora/rev/574dc701721f
https://hg.mozilla.org/releases/mozilla-beta/rev/46a3bb37188b

Comment 24

[Rebecca Billings [:rbillings]]

Unable to repro on the 12/28 unagi build.

Comment 25

[Cody Roesch [:croesch]]

Unable to repro this bug on the Unagi build 20121231070201.

Comment 26

[Allen Maxwell]

Removed "qawanted" form keywords refer to above comment

Unable to verify this issue on Otoro devices, only have Unagi. 

Added "Otoro" to keywords for further verification