1142437 - [Flame][Video]The video will automatically fast forward/fast rewind at the speed of 10s per second. And it can not be stopped by tapping Play/Rewind/Fast forward button.

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[SandKing]

Attachment: Bug log: logcat_1013.txt

[1.Description]:
[Flame][v2.2][Video]When you press forward/rewind button 5s on playing video， the video will automatically fast forward/fast rewind at the speed of 10s per second. And it can not be stopped by tapping Play/Rewind/Fast forward button.
See video: "1013.mp4"
See log: "logcat_1013.txt"
Found time: 10:13

[2.Testing Steps]: 
1. Launch Video.
2. Play a video.
3. Long press forward button 5s.


[3.Expected Result]: 
3. The video will automatically fast forward at the speed of 10s per second. but  it should be stopped by tapping Play/Rewind/Fast forward button.

[4.Actual Result]: 
3. The video will automatically fast forward at the speed of 10s per second. And it can not be stopped by tapping Play/Rewind/Fast forward button.

[5.Reproduction build]: 
Flame 2.2 (affected):
Build ID               20150311002522
Gaia Revision          3f032238a52f08e4c2f68a47ad065a96eb22d470
Gaia Date              2015-03-11 00:28:07
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/004fa1cb1dd4
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150311.040546
Firmware Date          Wed Mar 11 04:05:57 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 (unaffected) :
Build ID               20150311010231
Gaia Revision          943c8b4039f59b08ba100390e164a076a20c892e
Gaia Date              2015-03-10 20:35:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/fd8e079d6335
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150311.043838
Firmware Date          Wed Mar 11 04:38:50 EDT 2015
Bootloader             L1TC000118D0

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

[7.TCID]: 
14734

Comment 1

[SandKing]

Attachment: Bug video: 1013.mp4

Comment 2

[William Hsu [:whsu]]

Hi, SandKing,

May I have your help?
Can it be reproduced on v2.1 build?

Comment 3

[SandKing]

Hi William,
This bug not exsits on 2.1 and 2.1s.

2.1 build(unaffected):
Build ID               20150312001452
Gaia Revision          db751d9c200dce41cd03a61665746d245739a175
Gaia Date              2015-03-11 17:36:11
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/28ffee0d5b0c
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150312.034011
Firmware Date          Thu Mar 12 03:40:22 EDT 2015
Bootloader             L1TC000118D0

2.1s build(unaffected):
Gaia-Rev        88bd440e58a1584d065c29cf7a3450d25e45017b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/eee5226038b3
Build-ID        20150312001454
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

Comment 4

[Eric Chang [:ericcc] [:echang]]

[Blocking Requested - why for this release]: Regression

Comment 5

[Blake Wu [:bwu][:blakewu]]

John, 
Could you or someone help look it from gaia part?

Comment 6

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

Russ,

May you help to look at this?

Comment 7

[Russ Nicoletti [:russn]]

What's happening is the order of the events the video app is getting are different between v2.2 and 3.0 (master). 

When the "long-press" seeking feature was implemented (2.0 I believe) the events received by the video app when the user long-pressed on the forward or rewind button was:
  'contextmenu' followed by 'touchend' when the user released the button. 

This is the same order/events received when running a master (3.0) build.

In 2.2, the events are: 
  'contextmenu', 'click' (target being the button being long-pressed -- seek-forward/rewind), 'touchend'

The video app does not account for the 'click' event coming after 'contextmenu' and before 'touchend' which results in the observed behavior. 

I could create a 2.2 video app patch but if we verify the 2.2 gecko behavior is incorrect I'd rather get that changed if possible. If it is difficult to do, I can create a 2.2 video app patch.

Sotaro, can you comment on the appropriate events emitted as a result of a long-press of a button?

Comment 8

[Sotaro Ikeda [:sotaro]]

russn, I tried several times on latest v2.2 flame-kk, but I could not reproduce the problem. Is the problem still happen on your side?

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> russn, I tried several times on latest v2.2 flame-kk, but I could not
> reproduce the problem. Is the problem still happen on your side?

Sorry, it was not latest code. On latest code, I confirmed the problem.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Russ Nicoletti [:russn] from comment #7)
> In 2.2, the events are: 
>   'contextmenu', 'click' (target being the button being long-pressed --
> seek-forward/rewind), 'touchend'
> 
> The video app does not account for the 'click' event coming after
> 'contextmenu' and before 'touchend' which results in the observed behavior. 

Touch event is not handled by media framework. It seems like more general dom area problem.

Comment 11

[Hema Koka [:hema]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> (In reply to Russ Nicoletti [:russn] from comment #7)
> > In 2.2, the events are: 
> >   'contextmenu', 'click' (target being the button being long-pressed --
> > seek-forward/rewind), 'touchend'
> > 
> > The video app does not account for the 'click' event coming after
> > 'contextmenu' and before 'touchend' which results in the observed behavior. 
> 
> Touch event is not handled by media framework. It seems like more general
> dom area problem.

Sotaro,

Who do you think can help with the touch event/dom issue with 2.2?

Thanks
Hema

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Hema Koka [:hema] from comment #11)
> 
> Sotaro,
> 
> Who do you think can help with the touch event/dom issue with 2.2?

It is not clear yet which part of the code change actually caused the regression. This bug seems like recent regression. Therefore, it might better to wait regressionwindow result.

Comment 14

[Brogan Zumwalt [Inactive]]

3.0 was at one time affected, but was fixed at some point. 2.2 is still affected and was never fixed. Three windows are being posted in this comment. They are (in order):

- Central regression window (last working, first broken)
- Reverse regression window for 3.0 in Mozilla-Inbound (when 3.0 was fixed)
- Regression window for mozilla-b2g37_v2_2-flame-kk-eng


Central regression window (last working, first broken):
Last working Central build:
Device: Flame 3.0
Build ID: 20150210130358
Gaia: 8c7865486a1b11076b849bbf8f7fccbaffbfafe7
Gecko: ee093ca70666
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

First broken Central build: 
Device:  Flame 3.0
BuildID: 20150211055858
Gaia: 462a2ef9e98134255c144e373c7392440e3ee03b
Gecko: 38058cb42a0e
Version: 38.0a1 (3.0)
Firmware: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Working Gaia with Broken Gecko issue DOES reproduce:
Gaia: 8c7865486a1b11076b849bbf8f7fccbaffbfafe7
Gecko: 38058cb42a0e

Working Gecko with Broken Gaia issue does NOT reproduce:
Gaia: 462a2ef9e98134255c144e373c7392440e3ee03b
Gecko: ee093ca70666


Mozilla-Central Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ee093ca70666&tochange=38058cb42a0e

Appears to have been caused by changes made in bug 1125422, fixed on 3.0 by one of the bugs in the mozilla-inbound pushlog below but never on 2.2



Reverse regression window for 3.0 in Mozilla-Inbound (when 3.0 was fixed):
Last broken Mozilla-Inbound build:
Device:  Flame 3.0
BuildID: 20150303000706
Gaia: c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko: 974ca22a87c6
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

First working Mozilla-Inbound build:
Device:  Flame 3.0
BuildID: 20150303003952
Gaia: c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko: 1fb224ec0020
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0


Broken Gaia with Working Gecko issue IS fixed:
Gaia: c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko: 1fb224ec0020

Broken Gecko with Working Gaia issue is NOT fixed:
Gaia: c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko: 974ca22a87c6


Mozilla-Inbound Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=974ca22a87c6&tochange=1fb224ec0020



Regression window for mozilla-b2g37_v2_2-flame-kk-eng:
Last working mozilla-b2g37_v2_2-flame-kk-eng build:
Device:  Flame 2.2
BuildID: 20150216113731
Gaia: ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gecko: d29b62824a19
Version: 37.0a2 (2.2)
Firmware: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First broken mozilla-b2g37_v2_2-flame-kk-eng build: 
Device: Flame 2.2
Build ID: 20150216114329
Gaia: ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gecko: 604fe707d582
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Working Gaia with Broken Gecko issue DOES reproduce:
Gaia: ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gecko: d29b62824a19

Working Gecko with Broken Gaia issue does NOT reproduce:
Gaia: ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gecko: 604fe707d582


Mozilla B2G37 v2,2 Flame KK Eng pushlog:
http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/pushloghtml?fromchange=d29b62824a19&tochange=604fe707d582

Comment 15

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

Kartikaya, can you take a look at this please? Looks like this was initially caused by the work done on bug 1125422. 

This was fixed on 3.0 but we are not sure which one of these bugs it was fixed on: 
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=974ca22a87c6&tochange=1fb224ec0020 

However, this still occurs on 2.2.

Comment 16

[Russ Nicoletti [:russn]]

Kats, if you confirm the issue is caused by the bug 1125422 changes, can you update this bug's product and component? Thanks.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Per comment 7 it does sound like a gecko side issue; I can investigate.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

I dug into this and it looks like on both 2.2 and master we dispatch the same events to the button, but a different order. On 2.2 we dispatch contextmenu,click,touchend whereas on master we dispatch contextmenu,touchend,click. The reason the ordering is different is that on master we detect an :active style on the element and so delay the dispatch of the click event at [1]. On 2.2 the C++ code is the same but we do NOT detect an :active style on the element, and so we don't delay the dispatch. I looked at the Gaia CSS code and they both seem to have :active style rules so I need to dig into why the active style check is failing on 2.2.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZEventState.cpp?rev=71c85590ab17#170

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Note also that on both 2.2 and master a correct fix would be to have the video app call preventDefault on the contextmenu event, which will prevent the click in both cases. This is a correct fix because the video app is handling the contextmenu event and so should prevent other code from also handling it.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

So I think the problem is that the ActiveElementManager receives notifications both via the touch events and the APZ transform notifications. The latter may be delayed because they are dispatched upon the APZC processing touch events, which doesn't happen right away if there are touch listeners registered. The former are not subject to this delay. Therefore the order in which AEM receives notifications is dependent on whether or not there are touch listeners registered. When there are touch listeners registered it misbehaves, which is what is happening on 2.2. Uplifting bug 1130011 and bug 1130016 would probably fix this on 2.2, but it would just paper over the AEM bug which we would need to fix regardless.

Comment 21

[Russ Nicoletti [:russn]]

Based on comment 19 and comment 20, I'm not sure where you think the code change should be. It seems you're saying there is an AEM fix that should be made that will resolve this. Btw, I have tried calling preventDefault on the contextmenu event in the video app but the result is the button that is long-pressed remains highlighted/active. In any event, there is a simple fix I can implement in 2.2 to ignore click events that come after contextmenu and before touchend. But if you're going to make the AEM change in 2.2 I won't submit the 2.2 video app patch.

What is your recommendation?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: video-longpress-2.2-apz-log

I have a fix and I think it's relatively safe but it might not fix all the possible bad scenarios with AEM. I'm attaching logs to explain what's happening. The log here is the one on 2.2. Note that the hit result in this log is always "2" (i.e. DispatchToContent). This means that the notifications at [1] gets delayed until after APZ receives the content response and processes the pending input block. In the meantime, however, the calls at [2] and [3] run. You can see this ordering in the log because the AEM log on line 39 comes after the AEM log on line 26.

This effectively leaves the mCanBePanSet flag true after the touch block is done, and this flag interferes with the next touch block when it's not supposed to. This cycle happens again when I tap again on line 168-184. And then I long-press on the fast-forward button. This time, the dangling mCanBePanSet flag has a worse effect, in that AEM thinks it's a multi-touch input, at line 268. This causes the AEM to abort and so when the active style check subsequently happens it fails.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZEventState.cpp?rev=78dcb3a426f2#354
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZEventState.cpp?rev=78dcb3a426f2#252
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZEventState.cpp?rev=78dcb3a426f2#284

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Attachment: video-longpress-master-apz-log

This is the corresponding log on master. Here there is no touch listener, so the hit result is always 1 and the notifications arrive in the order we expect. AEM's state stays consistent and tracks input blocks properly, never detects a multitouch when it shouldn't, and everything works great.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This is my fix; when we get the EndTouch state notification from APZ we just clear out the flag that was set with the StartTouch state notification. I don't know if we can do much about the fact that AEM is getting two distinct types of messages (which I tried to clarify by updating method names/comments) but we can try to make sure that each pair has a balanced effect. The actual fix is pretty small and should be safe enough to uplift. If we want to consider a more involved fix for master we can do that as well.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

(In reply to Russ Nicoletti [:russn] from comment #21)
> What is your recommendation?

I think we can fix this in Gecko and shouldn't need any Gaia-side fix. Thanks for trying the contextmenu preventDefault, I'm a little surprised that it leaves the item highlighted but that might also be a result of this AEM bug.

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8579564 [details] [diff] [review]
Patch

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

I *believe* this change is safe to make. My reasoning is that the added 'mCanBePanSet = false' statement when processing an EndTouch notification would only mess things up if said notification arrives before the SetTargetElement() call for touch-start event, and I don't believe that's possible.

That said, I agree that this code is fairly fragile due to AEM receiving two distinct types of notifications, and I think this is worth fixing properly (if not for 2.2, then at least for trunk).

How about the following as a proper solution:

  - Send input block ids along with the StartTouch and EndTouch notifications.
    We can add a second argument for this purpose.

  - Have each of SetTargetElement, HandleTouchStart, HandleTouchEnd, and
    HandleTouchEndEvent forward the input block id to the AEM.

  - Have AEM keep track of the id of the current input block it is processing.

  - Where appropriate, use the block id to detect and weed out notifications
    that don't pertain to the current block.

::: gfx/layers/apz/util/ActiveElementManager.h
@@ +35,4 @@
>     * Specify the target of a touch. Typically this should be called right
>     * before HandleTouchStart(), but we give callers the flexibility to specify
>     * the target later if they don't know it at the time they call
>     * HandleTouchStart().

I don't think the last sentence in this comment is accurate any more. It's true that HandleTouchStart() and SetTargetElement() can be called in either order, but the reason isn't that the calling code may not know what the target is - it's that the calling code may wait until a content response (or timeout) before calling HandleTouchStart().

Comment 27

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #26)
> I *believe* this change is safe to make. My reasoning is that the added
> 'mCanBePanSet = false' statement when processing an EndTouch notification
> would only mess things up if said notification arrives before the
> SetTargetElement() call for touch-start event, and I don't believe that's
> possible.

Agreed

> That said, I agree that this code is fairly fragile due to AEM receiving two
> distinct types of notifications, and I think this is worth fixing properly
> (if not for 2.2, then at least for trunk).

Agreed, and I think the solution you outlined makes sense. However I think that'll be a bigger change than I want to uplift to 2.2 without a good reason, so I've filed bug 1145090 to do it on master. If it turns out we need it on 2.2 to fix other user-encountered scenarios we can try to uplift it at that point.

> ::: gfx/layers/apz/util/ActiveElementManager.h
> @@ +35,4 @@
> >     * Specify the target of a touch. Typically this should be called right
> >     * before HandleTouchStart(), but we give callers the flexibility to specify
> >     * the target later if they don't know it at the time they call
> >     * HandleTouchStart().
> 
> I don't think the last sentence in this comment is accurate any more. It's
> true that HandleTouchStart() and SetTargetElement() can be called in either
> order, but the reason isn't that the calling code may not know what the
> target is - it's that the calling code may wait until a content response (or
> timeout) before calling HandleTouchStart().

I reworded this comment a little to better explain what's happening and landed the patch. We'll probably reword this comment again when we fix bug 1145090.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e62677019c7

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e62677019c7

Comment 29

[Coler]

This issue CAN be repro on Nexus 5_v2.2&3.0:
Reproduce rate:5/5

N5_3.0(affected):

Build ID               20150319160212
Gaia Revision          c39e15f631de80c69467fda0d4ea0bcda9e194ca
Gaia Date              2015-03-18 19:30:04
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/cbd0efcd976c
Gecko Version          39.0a1
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150319.192641
Firmware Date          Thu Mar 19 19:26:57 EDT 2015
Bootloader             HHZ12d

N5_2.2(affected):

Build ID               20150319002500
Gaia Revision          9043c11f699c15bb6072422d1dad6518d1b5ddda
Gaia Date              2015-03-19 01:40:44
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c0442d170bec
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150319.042430
Firmware Date          Thu Mar 19 04:24:46 EDT 2015
Bootloader             HHZ12d

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8579564 [details] [diff] [review]
Patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Exposed by bug 1125422
User impact if declined: long-pressing on the fastfwd or rewind buttons in the video player gets it stuck
Testing completed: tested locally on a 2.2 build
Risk to taking this patch (and alternatives if risky): fairly low risk. There's a few other options we have to fix the symptoms seen in this bug but they're all higher risk than this patch, I think.
String or UUID changes made by this patch: none

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/05390c499b08

Comment 32

[SandKing]

This issue has been verified successfully on Flame2.2

Flame 2.2 bulid:
Gaia-Rev        014d38f7ad3912b8b33cb08ce7535a5dc5aced59
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7a9f2a248e57
Build-ID        20150324002504
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150324.041652
FW-Date         Tue Mar 24 04:17:03 EDT 2015
Bootloader      L1TC000118D0

Comment 33

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 3.0

Holding down fast forward or rewind buttons while playing video moves in forward/reverse in 10 second increments, but stops when user releases fast forward or rewind button.

Device: Flame 3.0
Build ID: 20150428010206
Gaia: 0636405f0844bf32451a375b2d61a2b16fe33348
Gecko: caf25344f73e
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0