1101438 - [Flame][Gallery]In landscape mode, re-launch gallery has no crop border on photo.

Status: VERIFIED FIXED

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

Description

[Norry.L.F(Leave from Mozilla)]

[1.Description]:
[Flame][v2.1&v2.2][Contacts]Enter Gallery editing interface from landscape mode, exit and enter the editing interface again,edit function will not work
Found time:11:29
See attachment: VIDEO_1129.mp4 and logcat_1129.txt

[2.Testing Steps]:
Precondition:The photo must take from Camera.
1.Launch Gallery.
2.Choose a picture and enter editing interface with landscape mode.
3.Back to desktop and enter editing interface with landscape mode again.

[3.Expected Result]: 
3.Photo can be edited.

[4.Actual Result]: 
3.Photo can not be edited.

[5.Reproduction build]: 
FLame 2.1 build:
Gaia-Rev        81160ad79e5b4c21967418dd63f1a1d08d77924e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3572aa3e6766
Build-ID        20141117001201
Version         34.0

Flame2.2 build:
Gaia-Rev        ae3a84acaab80a5b35d5542d63e68462273c8a1b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2d0a51ef828d
Build-ID        20141117160200
Version         36.0a1

[6.Reproduction Frequency]: 
Always Recurrence,5/5
Free Test

Comment 1

[Norry.L.F(Leave from Mozilla)]

Attachment: logcat

Comment 2

[Norry.L.F(Leave from Mozilla)]

Attachment: video

Comment 3

[Shine(Leave from Mozilla)]

Attachment: Flame2.1 logcat

Dear Mike,
This problem is verified NOT to happen on latest Flame2.2, but it can be repro on latest Flame2.1. Could you please help with it, thanks!
See attachments: Flame2.1_logcat_1057.txt
Repro time: 10:57
Rate: 5/5

Flame 2.1 build:
Gaia-Rev        73be51f998031f06db0cd660c0e388fa621c9f4c
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ea426e47bfc4
Build-ID        20141230001256
Version         34.0

Flame 2.2 build:
Gaia-Rev        322ef5116a5827a30c9a3cd9b842449a9c66a5b3
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/67872ce17918
Build-ID        20141230010205
Version         37.0a1

Comment 4

[Mike Lien[:mlien]]

Hi David, could you help with this, thanks.
Refer to attached video, update STR

1. Device in landscape mode
2. Launch Gallery app
3. Tap one photo -> tap edit button
4. Choose "Crop"
5. Tap Home button
6. Re-launch Gallery app again

The crop border will disappear after re-launch Gallery app

Comment 5

[David Flanagan [:djf]]

I can reproduce this on master, so setting that flag back and taking it to fix.

The problem is that when we relaunch the app with the phone already held in landscape mode, we get an initial resize event for portrait followed right away by another resize for landscape. My resize handler code is asynchonous, however, and so the second event is handled before the first is done.

The fix is to make the resize handler synchronous. It turns out that I can do that by moving some code from the finishEdit() function to the generateNewPreview() function.

Patch coming soon.

Comment 6

[David Flanagan [:djf]]

Attachment: link to patch on github

Punam,

This patch makes the ImageEditor resize handler synchronous so that when we get two resize events in a row the two invocations of the handler do not run at the same time.  In order make it synchronous, I had to move the code that updates the this.dest rectangle from finishEdit to generateNewPreview. That is probably a better place for it because it means that generateNewPreview and displayCropOnlyPreview are the only two places that set the dest rectangle, which makes them nicely parallel.

Comment 7

[Punam Dahiya [:pdahiya]]

Comment on attachment 8543423 [details] [review]
link to patch on github

Thanks David for the patch, the issue is difficult to replicate in 2.2. I tested the patch in 2.1 and it fixes the issue and works good in master.

Comment 8

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/54a33de7bf466d536ca706be9d488c6cefa9c5d0

Comment 9

[David Flanagan [:djf]]

Mike: if you think this is a serious enough bug, please set needinfo for me and I'll set the uplift approval request flag.

Comment 10

[Mike Lien[:mlien]]

I don't think this is serious issue.
But I wanna double comfirmed with No-Jun and Bhavana if this patch should be uplifted.

Comment 11

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

:ktucker, please verify this bug on master. Please also determine if the bug is a regression from 2.0 to 2.1.

Comment 12

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue has been verified as fixed on Flame 2.2

The user can edit a picture in landscape mode, go home and still edit the picture without issue.

Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20150108010221
Gaia: d4dac29613076bdba3cb8adc217deadb08a2ac20
Gecko: 70de2960aa87
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 13

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue does not occur on the Flame 2.0

The user can edit a picture in landscape mode, go home and still edit the picture without issue. The crop border appears.

Environmental Variables:
Device: Flame 2.0 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20150108000200
Gaia: 31d6c9422cd0a8213df9f96019c9ab7168ec3ab3
Gecko: 7c7daab470fb
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Mike Lien[:mlien] from comment #10)
> I don't think this is serious issue.
> But I wanna double comfirmed with No-Jun and Bhavana if this patch should be
> uplifted.

I'd take it given this is something we regressed. can we get a risk evaluation on the patch to make sure this is very low risk to be able to uplift on 2.1 at this point ?

Comment 15

[Punam Dahiya [:pdahiya]]

IMO it's a low risk patch that moves code to update this.dest to ensure resize handler code executes synchronously. Please note it's not a straight uplift to 2.1 and needs a 2.1 patch.

Comment 16

[David Flanagan [:djf]]

I agree with Punam that it is a low-risk patch. I don't think I'd call it "very low risk", since it does slightly alter the timing of a variable change and could, in theory cause some kind of new race condition in the process of fixing another race condition. It can only affect the cropping operation in edit mode, however, and the chance of it creating a bug worse than the one it fixes seems very small.

I'll prepare a version of the patch for v2.1 and set the uplift request flag on it.

Comment 17

[David Flanagan [:djf]]

Attachment: version of the patch for v2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):

I'm not sure what the regresssion was. I suspect a change to the system app window management code when going into and out of landscape mode caused this pre-existing race condition in the resize handler to manifest .

[User impact] if declined:

If the user is cropping an image with the phone in landscape mode and then goes to the homescreen and keeps the phone in landscape mode, and then returns to the gallery app, the crop UI will be broken.

[Testing completed]: I think Punam has tested this patch 

[Risk to taking this patch] (and alternatives if risky): low risk, but not completely trivial.

[String changes made]: none

Comment 18

[David Flanagan [:djf]]

Punam,

Would you take a quick look a the v2.1 version of this patch that I prepared? Did you resolve the merge conflict in the same way I did? And if so, do you feel confident that this patch has been tested carefully on 2.1?

Comment 19

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #18)
> Punam,
> 
> Would you take a quick look a the v2.1 version of this patch that I
> prepared? Did you resolve the merge conflict in the same way I did? And if
> so, do you feel confident that this patch has been tested carefully on 2.1?

Looked at 2.1 patch and I had resolved merge conflict in the same way and it fixes the issue reported in 2.1. Thanks!

Comment 20

[bhavana bajaj [:bajaj]]

Comment on attachment 8547782 [details] [review]
version of the patch for v2.1

Requesting QA help verify this once it lands on 2.1.

Thanks for the risk analysis and testing Punam/:djf. If you need QA to do some adhoc testing around this area feel free to add a comment.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/4676bf8582f8cb432786b4aa84f96183b90500e6

Comment 22

[Shally Li（leave from Mozilla）]

This bug has been successfully verified on Flame v2.1 via the steps as mentioned in Comment 4.
See attachment: verified_v2.1.mp4.
Reproduce rate: 0/5.

Flame 2.1 build:
Gaia-Rev        6957ac8a322234ec99c8abb7cc18dc6a2e0176db
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/6600eba54256
Build-ID        20150113161204
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150113.192836
FW-Date         Tue Jan 13 19:28:47 EST 2015
Bootloader      L1TC000118D0

Comment 23

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.1.MP4

Comment 24

[Vincent Liu[:vliu]]

v2.1s : https://github.com/mozilla-b2g/gaia/commit/b5d1043b25983467f6419647d6a3643e1bd52254

Comment 25

[Norry.L.F(Leave from Mozilla)]

Attachment: Verify_2.1s.mp4

The problem is verified not happen on latest Dolphin 2.1s via the steps as mentioned in Comment 4.
See attachment: Verify_v2.1s.mp4
Reproduce rate: 0/5.

Dolphin 2.1s 512m：
Gaia-Rev        88084bc7ef5ba6627dd09c774ef2f7fa96cbed71
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/18624fbcc39a
Build-ID        20150216161200
Version         34.0
Device-Name     scx15_sp7715ea
FW-Release      4.4.2
FW-Incremental  122
FW-Date         Thu Feb  5 12:42:58 CST 2015

Dolphin 2.1s 256m：
Build ID               20150201161200
Gaia Revision          0ef345ec8b5a2cbde4bf46304eb59ab1c32ee251
Gaia Date              2015-01-29 18:07:20
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/a58ccf08015d
Gecko Version          34.0
Device Name            scx15_sp7715ga
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150201.195057
Firmware Date          Sun Feb  1 19:51:09 EST 2015