917382 - [B2G][Camera][Lockscreen]Camera viewscreen turns black after viewing a thumbnail in passcode lock

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[gbennett]

Attachment: CameraLockscreenBlackscreen.txt

Description:
Take a picture and view the thumbnail from the lockscreen with passcode enabled and press the home button after. Now enter the passcode and launch the camera app as it should just show a black viewscreen.

Repro Steps:
1) Updated Buri 1.2 mozRIL to Build ID: 20130917040201
2) Enable passcode lockscreen
3) Launch camera from lockscreen > take a picture > view thumbnail
4) Press home button and enter the lockscreen code
5) Launch the camera app

Actual:
Viewscreen is black.

Expected:
Viewscreen displays properly.

Environmental Variables
Deivce: Buri 1.2 mozRIL
Build ID: 20130917040201
Gecko: http://hg.mozilla.org/mozilla-central/rev/1d27c4c9871f
Gaia: 1518b76c07c4d70fc41113ed2e424a74b2bcef27
Platform Version: 26.0a1

Repro frequency: 100%, 4/4
See attached: CameraLockscreenBlackscreen.txt

Comment 1

[sarsenyev]

Regression Window!

The bug doesn't reproduce on Buri:
Build ID: 20130818040219
Gecko: http://hg.mozilla.org/mozilla-central/rev/a71cedddadd1
Gaia: 2675cc8d2d4fb18c6071b396c182fb8fd0e56fa3
Platform Version: 26.0a1

The bug reproduces on Buri:
Build ID: 20130819040203
Gecko: http://hg.mozilla.org/mozilla-central/rev/c8c9bd74cc40
Gaia: f6de05c135913f2cb790759335875bb1b3c4f9bb
Platform Version: 26.0a1

Comment 2

[Marcia Knous [:marcia]]

During media triage asked to look into this again - still reproduce on Buri using:

Gaia   88e73da95f1c550f2fb0572480a40c989d37c997
SourceStamp 46b216260c1d
BuildID 20130920004004
Version 26.0a2

Killing and relaunching the camera app does not resolve the black screen issue when the device is in this state.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

This is caused by the screen lock object:

E/GeckoConsole( 1617): [JavaScript Error: "InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable" {file: "app://system.gaiamobile.org/camera/js/camera.js" line: 433}]

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

Oh, this regression is created by me. Sorry about that. 

In the patch of bug 897863, we replace the misleading variable name "returnToCamera" with Filmstrip.isPreviewShown(). 
In previewItem method[1], we release the screen lock at the first line and set the flag "offscreen"[2] which is used in isPreviewShown[3] after that. But we also use isPreviewShown in releaseScreenWakeLock method[4]. That makes a screen lock is never unlocked. When user press home button, that error message is shown. This is because wakeLock is already released by gecko when window is removed.


[1] https://github.com/mozilla-b2g/gaia/blob/49a0da45de783c96fb8e49bbc7f1c395d5da9273/apps/camera/js/filmstrip.js#L102
[2] https://github.com/mozilla-b2g/gaia/blob/49a0da45de783c96fb8e49bbc7f1c395d5da9273/apps/camera/js/filmstrip.js#L119
[3] https://github.com/mozilla-b2g/gaia/blob/49a0da45de783c96fb8e49bbc7f1c395d5da9273/apps/camera/js/filmstrip.js#L133
[4] https://github.com/mozilla-b2g/gaia/blob/49a0da45de783c96fb8e49bbc7f1c395d5da9273/apps/camera/js/camera.js#L431

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: move the sequence of request and release wake lock

Move the requestScreenWakeLock and releaseScreenWakeLock to be after flagging offscreen.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 808439 [details]
move the sequence of request and release wake lock

I am confused.  It used to be that the lockscreen incorporated its own copy of the camera app, so when we used the camera from a pin-locked lockscreen we were using a completely different app than the regular camera.

But the steps to reproduce say that the lockscreen camera is affecting the homescreen camera. Have we changed things so that it is just one app now?

If so, then we can no longer treat Camera._secureMode as a constant, and need to modify it to dynamically and correctly detect whether the app is being used from the lockscreen.  There are probably all kinds of security bugs now.

If this needs to be fixed, please file a bug to do it.

As for this particular patch, if it fixes the bug, you can land it.  But before you do, please take a look at the {request,release}ScreenWakeLock code in camera.js. I see that both of those methods check Filmstrip.isPreviewShown().  But now look at where (in camera.js) those methods are called. Those call sites manipulate the Camera._previewActive flag.  I wonder if somewhere along the line we mixed up the camera.js _previewActive and the filmstrip.js preview flag.

Should the request and release methods be checking the camera preview or the filmstrip preview, or both, or neither of them?  We actually seem to have enough checks elsewhere that maybe request and release could just not do any checks themselves.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

David, 

Thanks for your questions and review. I was also confused by most of them before. I feel we should pull the trigger to refactor camera app while migrating to use getUserMedia(). The following are my understanding of those questions:

(In reply to David Flanagan [:djf] from comment #6)
> Comment on attachment 808439 [details]
> move the sequence of request and release wake lock
> 
> I am confused.  It used to be that the lockscreen incorporated its own copy
> of the camera app, so when we used the camera from a pin-locked lockscreen
> we were using a completely different app than the regular camera.

I can confirm that we only have one implementation of Camera app for both lockscreen and homescreen camera. But the lockscreen one is the copied one when we hit make. In the view of system, they are two different app indeed but the implementation of them are totally the same. To tell the difference, we use a flag, _secureMode, to do that. I found that since I was on board.


> But the steps to reproduce say that the lockscreen camera is affecting the
> homescreen camera. Have we changed things so that it is just one app now?

Well. They do affect each other no matter they are two different apps or not. The main reason of black screen is the release of mozCamera object[1]. In this bug, we got an exception at the releasing procedure, but that is before releasing mozCamera object. If we don't release it, no more mozCamera can be acquired[2]. And the gotCamera[2] is never called in this case and all screen are black and buttons are invisible and inactive. So, we won't have any workable Camera app including lockscreen and home screen. And if there is another app acquiring it but not releasing it, all apps based on mozCamera are affected. That's also crazy to me.


> If so, then we can no longer treat Camera._secureMode as a constant, and
> need to modify it to dynamically and correctly detect whether the app is
> being used from the lockscreen.  There are probably all kinds of security
> bugs now.
> 
> If this needs to be fixed, please file a bug to do it.

I don't think we need to do that. Because they are running as two different app and only be set once.

> As for this particular patch, if it fixes the bug, you can land it.  But
> before you do, please take a look at the {request,release}ScreenWakeLock
> code in camera.js. I see that both of those methods check
> Filmstrip.isPreviewShown().  But now look at where (in camera.js) those
> methods are called. Those call sites manipulate the Camera._previewActive
> flag.  I wonder if somewhere along the line we mixed up the camera.js
> _previewActive and the filmstrip.js preview flag.

_previewActive, this flag is used for viewfinder not for Filmstrip. So, I don't think they are related to each other. And I had check the filmstrip.js. It doesn't check this flag.

> 
> Should the request and release methods be checking the camera preview or the
> filmstrip preview, or both, or neither of them?  We actually seem to have
> enough checks elsewhere that maybe request and release could just not do any
> checks themselves.

Ok, I was confused by this before, I mean the checking of Filmstrip.isPreviewShown(). In the normal cases, we have to release the wake lock by ourself. But in camera app, it doesn't. I had checked the code[3] before, I mean the pretty old version. It runs like that. I had asked this to alive. He told me that wake lock will be released automatically when we remove the frame from the gecko. That's the main reason that we get the exception in comment 3 in this bug. Because we are trying to unlock the wake lock when the frame is removed from the gecko. So, the unlocking procedure is only used by the case preview of filmstrip shows.



[1] https://github.com/mozilla-b2g/gaia/blob/eaae9cdb28b1fb28eddbb562b205c676e5bc1eb2/apps/camera/js/camera.js#L1512
[2] https://github.com/mozilla-b2g/gaia/blob/eaae9cdb28b1fb28eddbb562b205c676e5bc1eb2/apps/camera/js/camera.js#L912-L948
[3] https://github.com/mozilla-b2g/gaia/blob/e86c6510430639b4484be6c8a1240ca526c7b7a8/apps/camera/js/camera.js#L424-L440

Comment 8

[David Flanagan [:djf]]

John,

I misunderstood the cause of this bug so my questions about the secureMode stuff don't make any sense. Sorry!

The screen lock code seems more complicated than it needs to be, and I worry about it in the future, but that doesn't need to be addressed here.  If an exception was causing the camera hardware not to be released, consider adding appropriate exception handlers (like put the release call in a finally clause?) so that it can't happen in the future.

But my r+ on the patch stands, and you can just land it as is if you want.

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

> The screen lock code seems more complicated than it needs to be, and I worry
> about it in the future, but that doesn't need to be addressed here.  If an
> exception was causing the camera hardware not to be released, consider
> adding appropriate exception handlers (like put the release call in a
> finally clause?) so that it can't happen in the future.

That's a good idea. I will go to add one.

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:
https://github.com/mozilla-b2g/gaia/commit/b40f46141925e8863c39b67bd29fb134d15347e2

Comment 11

[James Lal [:lightsofapollo]]

Uplifted b40f46141925e8863c39b67bd29fb134d15347e2 to:
v1.2: 2412d06c237838765468d3d4a40ed1623540951a

Comment 12

[Lucas Aliaga]

Verified Fixed in latest 1.2 Buri. Camera no longer appears with black screen in app after viewing picture taken from PIN locked camera.

Environment:
Gaia   5ef3535021286ccab7af639897feaaf5955720a0
SourceStamp 75b0b968f3ed
BuildID 20131016004005
Version 26.0a2