1043689 - Difficult or impossible to exit the edit icons screen

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

When editing the icons on the homescreen I find it essentially impossible to exit the screen by tapping the done button in the top right.  Gregor and I reproduced this pretty easily on our debug Gecko builds.  He suspects it might be a race condition.

I also see "WARNING: Unhandled state upon single touch start: file ../../../gfx/layers/apz/src/GestureEventListener.cpp, line 136" in logcat.

Comment 1

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

I can look into this tomorrow but if you happen to catch it in a debugger before I get to it, the value of mState at that point would be useful to know.

Comment 2

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

I'm not able to reproduce the non-working "done" button on latest master (m-c as of this morning). I can reproduce a different issue though:

- Long-press an icon to go into edit mode
- Hit the done button to go back
- Long-press an icon again <-- this will not go into edit mode

With these STR I see the warning from comment 0 get triggered, it's happening because the long-press event is cancelled by content, and so the touch-block (which includes the touch-up event) doesn't get delivered to the GEL. This leaves it in the long-tap state when the next touch-start comes in and so it doesn't process that properly. It handles this case gracefully by resetting the state so the next touch-down should get handled fine.

I'll put up a patch for this issue and hope it fixes the issue described in comment 0 as well.

Comment 3

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

Attachment: Patch

When processing a prevent-defaulted block of input events, clear the state in the GEL.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/789665f36a74

Comment 5

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

Oh I can repro the original issue on a Hamachi. I debugged it and it doesn't appear to be APZ-related. I see TabChild::FireSingleTapEvent getting called as expected when I tap on the "done" button. It might be a gaia bug; will check.

Comment 6

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

Kevin, this looks like a homescreen bug. I definitely see the click event getting dispatched to the content, and the click handler is running, which fires the hashchange event. But window.app.grid._grid.dragdrop.inEditMode is always false for some reason, so window.app.grid._grid.dragdrop.exitEditMode() never gets called. If I call it manually from the console it exits the mode.

Comment 7

[Kevin Grandon :kgrandon]

Ok, I will dig in. The gaia code here looks extremely simple, so I'm not sure what we could be doing wrong, but I wonder if we could be preventing some touch event from propagating.

We add the edit mode listener here: https://github.com/mozilla-b2g/gaia/blob/bda8c70178259368a13485d8b1ac755370ac9f2a/apps/verticalhome/js/app.js#L20

This should fire a hashchange event and exit edit mode here: https://github.com/mozilla-b2g/gaia/blob/bda8c70178259368a13485d8b1ac755370ac9f2a/apps/verticalhome/js/app.js#L195

Comment 8

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

I stuck a breakpoint inside the enterEditMode() function in grid_dragdrop.js, and the "this" object in there was not the same as the window.app.grid._grid.dragdrop object. So this is definitely a bug in some JS code somewhere. I'll leave it to you :)

Comment 9

[Kevin Grandon :kgrandon]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> I stuck a breakpoint inside the enterEditMode() function in
> grid_dragdrop.js, and the "this" object in there was not the same as the
> window.app.grid._grid.dragdrop object. So this is definitely a bug in some
> JS code somewhere. I'll leave it to you :)

enterEditMode() should really not matter here, the exit code should be independent. I'm looking into this, but I don't really see how that this is possible.

Also having a hard time reproducing on my flame.

Comment 10

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

I only saw it on hamachi, not on flame. I can look into it more tomorrow if you can't repro.

Comment 11

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

On my Hamachi around 25 dragdrop instances are created at [1]. Only one of them will actually process the contextmenu event because the handler does a stopImmediatePropagation at [2]. So only one of the dragdrop instances goes into edit mode, and it's got a 1/25 chance of being the one that the grid_view keeps a handle to.

It seems that the stopImmediatePropagation line was removed very recently in bug 1043293 (I don't have that change locally) but that might fix this issue. However it won't stop all 25 dragdrop instances from being created, which is kind of a waste. I stuck a | if (!dragdrop) | guard *inside* the LazyLoader callback at [1] and that fixed the problem for me locally.

Up to you how you want to fix it.

[1] https://github.com/mozilla-b2g/gaia/blob/dd7e13eb8c6a50a72b447c92eddbe92f6fc655c5/shared/elements/gaia_grid/js/grid_view.js#L403
[2] https://github.com/mozilla-b2g/gaia/blob/dd7e13eb8c6a50a72b447c92eddbe92f6fc655c5/shared/elements/gaia_grid/js/grid_dragdrop.js#L438

Comment 12

[Kevin Grandon :kgrandon]

Nice work Kats. Let's see what bug 1043293 does to this.

Comment 13

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

It might fix it, but there's still a leak here. The 24 extra DragDrop instances don't get GC'd because they're registered as event listeners and never unregister themselves.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/789665f36a74

Comment 15

[Kevin Grandon :kgrandon]

Attachment: Pull request - Safeguard dragdrop intiitalization

Hey Chris - Any chance you could help me review this simple patch? I just want to safeguard and make sure that dragdrop is only instantiated once. I was unable to reproduce this bug, but this should help. Thanks!

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 8466562 [details]
Pull request - Safeguard dragdrop intiitalization

LGTM.

Comment 17

[Kevin Grandon :kgrandon]

Thanks for the review! Master: https://github.com/mozilla-b2g/gaia/commit/a78866090e213c8321a5d19e01475276727affb6

Comment 18

[Kevin Grandon :kgrandon]

[Blocking Requested - why for this release]: This should block 2.0 both as a potential solution for bug 1047639 and the fact that it could be a pretty bad race condition. I was unable to reproduce this on a flame device, but apparently people could reproduce it on hamachis.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/789665f36a74

Comment 20

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

Note only the gaia patch needs going into 2.0; the gecko patch won't apply there.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #18)
> [Blocking Requested - why for this release]: This should block 2.0 both as a
> potential solution for bug 1047639 and the fact that it could be a pretty
> bad race condition. I was unable to reproduce this on a flame device, but
> apparently people could reproduce it on hamachis.

Sounds good - blocking to help resolve bug 1047639.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/bc629048b01c1cbf71a66386b2b9b2ccbf592ebc