1131695 - Some buttons become unresponsive in gaia header

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Noemí Freire (:noemi)]

We have seen that some buttons become unresponsive in gaia header, some examples:
1- Being in Call log (having some entries), try to access Settings (edit mode) several times. Please see https://www.youtube.com/watch?v=kywIIuckiLA, in this case, the first attempt fails, it is necessary to click again on the "Settings" button to access the edit mode window.
2- Being in Contacts app and having some Contacts, try to access Settings several times, you will see that at some point Settings or Done button become unresponsive, it is necessary to click again on Settings or Done buttons to move on.
3- Being in Messages app, try to access Settings within Messages app several times, at some point Settings button becomes unresponsive, it is necessary to click again on Settings button to move on.

Environmental variables:
Flame device
Build Id: 20150210155432
Gecko: 35d3936
Gaia: 0707fcc
Platform version: 37.0a2

We haven NOT seen this wrong behavior in master. 

qawanted to confirm this error and to see which branches are affected. Thanks!

Comment 1

[Brogan Zumwalt [Inactive]]

Issue does NOT reproduce in Flame 2.1

Gaia header buttons do not become unresponsive in Call log, Contacts, Messages, or any other app in Flame 2.1

Device: Flame 2.1
Build ID: 20150210002200
Gaia: 7dd130a312f12c89b2d41948f8557effa56afbf6
Gecko: 2de03dfa9aac
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Issue may reproduce in 3.0

Could not reproduce any of the examples using 3.0 (in 2.2 the examples provided readily reproduce), but the back button in Settings app experienced some similar issues ONLY when the page was still populating. 

If user launches messages app, taps ellipsis icon, selects settings, then quickly taps the back button when the Messaging Settings page launches, the back button in the header is unresponsive for a short while. Unsure if this is same issue as it appears to be very specific and none of the examples from comment 0 reproduce in 3.0. Leaving 3.0 as unaffected for the time being.

Device: Flame 3.0
Build ID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Comment 2

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

This is a bad end user experience. The button should work when tapped. Nominating 2.2?

Comment 3

[Julien Wajsberg [:julienw]]

We just landed a very big change in gaia-header (bug 1112131). Could QA check whether the issue happens with/without this patch?

I just merged to v2.2 so I don't think the patch is in any v2.2 nightly build yet. So maybe that's why Noemi sees the issue in v2.2 but not master.

Comment 4

[Julien Wajsberg [:julienw]]

(In reply to Brogan Zumwalt [:BroganZ] from comment #1)
> Issue may reproduce in 3.0
> 
> Could not reproduce any of the examples using 3.0 (in 2.2 the examples
> provided readily reproduce), but the back button in Settings app experienced
> some similar issues ONLY when the page was still populating. 
> 
> If user launches messages app, taps ellipsis icon, selects settings, then
> quickly taps the back button when the Messaging Settings page launches, the
> back button in the header is unresponsive for a short while. Unsure if this
> is same issue as it appears to be very specific and none of the examples
> from comment 0 reproduce in 3.0. Leaving 3.0 as unaffected for the time
> being.

Yeah, this is bug 1125601. So let's leave this out of this bug :)


I actually also wonder if this bug is not about the rocketbar as well. Like the "event fluffing" mechanism would choose something in the rocketbar instead of the gaia header button.

Etienne, would you remember something about this, that would be fixed in master but not in v2.2?

Comment 5

[Lance（leave from Mozilla）]

mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
Build ID               20150129162506
Gaia Revision          6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gaia Date              2015-01-29 06:34:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9c8b785973bf
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150129.202729
Firmware Date          Thu Jan 29 20:27:40 EST 2015
Bootloader             L1TC000118D0

First Broken Environmental Variables:
Device: Flame
Build ID               20150129183930
Gaia Revision          6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gaia Date              2015-01-29 06:34:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/242eb3edf1bf
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150129.202729
Firmware Date          Thu Jan 29 20:27:40 EST 2015
Bootloader             L1TC000118D0

Last Working gaia / First Broken gecko - Issue DOES occur
Gaia: 6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gecko: 242eb3edf1bf

First Broken gaia / Last Working gecko - Issue does NOT occur
Gaia: 6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gecko: 9c8b785973bf

Comment 6

[Julien Wajsberg [:julienw]]

Given the regression window, this is not rocketbar related.

Comment 7

[Julien Wajsberg [:julienw]]

Gecko pushlog: http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/pushloghtml?fromchange=9c8b785973bf&tochange=242eb3edf1bf

Comment 8

[Julien Wajsberg [:julienw]]

Given the pushlog, this could be bug 1116588, given that maybe we trigger the edge gestures trigger when we tap on the button.

Kats, what do you think?

Comment 9

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

That's quite possible. If that is case then it might be fixed by bug 1128672 in master. I will request uplift on that bug shortly, but it needs to get uplifted with a bunch of other stuff. Marking it as a dependency for now and parking this bug with me.

Comment 10

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

So I'm able to reproduce this problem on a local 2.2 build, and the patch from bug 1128672 doesn't entirely fix it. The problem appears to be that when the touch_forwarder.js sends the mouse event using iframe.sendMouseEvent to the child process, that goes through [1] and that never accounts for the 30-pixel offset to the child process root layer. So all the mouseevents sent by sendMouseEvent (and sendTouchEvent) are going to be off by 30 pixels, always. In fact I'm amazed that we haven't run into this until now!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js?rev=bccb6cedee61#548

Comment 11

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

Oh actually sendTouchEvent doesn't have this problem if APZ is enabled on the child process, because then it uses TabParent.injectTouchEvent. So that probably explains why it's been obscured. Still, I would expect this bug to manifest in 2.1 as well and I'm not really sure why it supposedly doesn't.

Comment 12

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

Attachment: WIP

For example, this fixes the problem (when combined with bug 1128672) but obviously isn't something we can land. If necessary we can extract that 30 from the tabparent, but I have a different idea that I would like to explore first.

Comment 13

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

The window for this issue may have been found incorrectly.  The builds used seem to be working with gaia and gecko from different branches which might have caused false positives when finding last working and first broken builds.  Rechecking this window might be the correct course of action.

Comment 14

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

Julien appears to be correct here.

bug 1116588 seems to be the cause of this issue.

Mozilla-inbound regression window:

Last Working Environmental Variables:
Environmental Variables:
Device: Flame 3.0
BuildID: 20150126062631
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: 0bec74187553
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 Environmental Variables:
Environmental Variables:
Device: Flame 3.0
BuildID: 20150126065334
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: babd56077826
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Last Working Gecko & First Broken Gaia - issue DOES occur
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: babd56077826

Last Working Gaia & First Broken Gecko - issue does NOT occur
Gaia: 793773bb2944b42a85dd160049e605cbd880c4da
Gecko: 0bec74187553

Gaia pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0bec74187553&tochange=babd56077826

Comment 15

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

Attachment: Part 1 - Refactor to get the CSS -> device pixel scale

Comment 16

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

Attachment: Part 2 - Ensure the child process offset is accounted for in BrowserElementParent.js

Comment 17

[Brogan Zumwalt [Inactive]]

Issue still occurs in latest Flame 2.2

Clearing QAWanted as issue still occurs and bug 1112131 is a gaia fix while this bug is a gecko issue.

Confirming that comment 14 is calling this is a gecko issue that occurs on first broken gecko and the pushlog provided is a gecko, and not a gaia pushlog.

Device: Flame 2.2
Build ID: 20150212002504
Gaia: 791e53728cd8018f1d7cf7efe06bbeb1179f0370
Gecko: 5dec207fcbeb
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 18

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

I'm able to reproduce this on master as well, fwiw. That's expected given the root cause.

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 8563411 [details] [diff] [review]
Part 1 - Refactor to get the CSS -> device pixel scale

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

r=me with the question resolved.

::: dom/ipc/TabParent.cpp
@@ +1168,5 @@
> +  nsIPresShell* shell = (doc ? doc->GetShell() : nullptr);
> +  nsPresContext* ctx = (shell ? shell->GetPresContext() : nullptr);
> +  return LayoutDeviceToCSSScale(ctx
> +    ? (float)ctx->AppUnitsPerDevPixel() / nsPresContext::AppUnitsPerCSSPixel()
> +    : 0.0f);

Are you sure you want 0.0 here, and not 1.0 ?

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8563412 [details] [diff] [review]
Part 2 - Ensure the child process offset is accounted for in BrowserElementParent.js

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

I'm not a big fan of the code duplication in BrowserElementParent.js
What about doing something like that instead:

let offsets;
let tabParent = this._frameLoader.tabParent;
if (tabParent) {
      let offsetX = {};
      let offsetY = {};
      tabParent.getChildProcessOffset(offsetX, offsetY);
      offsets = { x: offsetX.value, y: offsetY.value };
      x += offsets.x;
      y += offsets.y;
}

.....

if (offsets) {
  for (let i = 0; i < touchesX.length; i++) {
    touchesX[i] += offsets.x;
  }
  for (let i = 0; i < touchesY.length; i++) {
    touchesY[i] += offsets.y;
  }
}

Comment 22

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

(In reply to Fabrice Desré [:fabrice] from comment #19)
> 
> Are you sure you want 0.0 here, and not 1.0 ?

Yes, that way on failure the offset is dropped entirely. That's what the code did before and I didn't want to change the error-condition behaviour.

I'll refactor the second patch to reduce duplication.

Comment 23

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

Attachment: Part 2 - Ensure the child process offset is accounted for in BrowserElementParent.js (v2)

Comment 24

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6823625f2042
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/13af514d6aea

Comment 25

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2d6f67c231 for mochitest-1 failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=6595909&repo=mozilla-inbound

Comment 26

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

Attachment: Part 2 - Ensure the child process offset is accounted for in BrowserElementParent.js (v3)

The test is wrong; it sends coordinates at (10,10) in the top-level window's coordinate space which end up being negative in the child iframe coordinate space. For some reason mouse events don't check for that but touch events do, and so the touch events get discarded in the child process. Adjusting the coordinates in the test so that they fall inside the iframe fixes it for me locally.

Try push to verify: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6739e695a60f

Comment 27

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

The updated try push failed also. I did another push with extra logging and it looks like on the tryserver machines the child process offset is -189, -373 and the touch coordinates are at 276,108. So the y-coord still ends up negative which will cause the event to get dropped and the test to fail.

Comment 28

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

I ended up having to call the tabParent method in the test as well to make this work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c01b3b816443

Comment 29

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/516a170b8fda
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/969d634fe396

Comment 30

[Carsten Book [:Tomcat]]

sorry had to back this out for perma test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6670856&repo=mozilla-inbound

Comment 31

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

Third time's the charm. Let's see how this try run does:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9e96f3235a2

Comment 32

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

That was green, so I landed.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9aeb417a0af8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b41c6cb9d022

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9aeb417a0af8
https://hg.mozilla.org/mozilla-central/rev/b41c6cb9d022

Comment 34

[Julien Wajsberg [:julienw]]

Kats, we'll need an approval here :)
Thanks !

Comment 35

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

Comment on attachment 8563411 [details] [diff] [review]
Part 1 - Refactor to get the CSS -> device pixel scale

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 #): bug 1116588
User impact if declined: when clicking on items near the left or right edge of the screen, the click is off by the height of the status bar
Testing completed: locally, on m-c
Risk to taking this patch (and alternatives if risky): low risk as this code only run in a small set of cases.
String or UUID changes made by this patch: none

Comment 36

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

Ah, sorry - the part 2 patch does bump the UUID in dom/interfaces/base/nsITabParent.idl!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4f6175ab42cb
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ac09ec5062b7

Comment 38

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

Fixup for build bustage: https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5c20fbeb5f8b

Comment 39

[SandKing]

Attachment: Verified video: verify.mp4

This issue has been verified successfully on Flame2.2&3.0,
Verify video: "verify.mp4"

Flame 2.2 build:
Build ID               20150302002504
Gaia Revision          77609916ca5ab721150fab2b7bc5c37f43ee3a5a
Gaia Date              2015-02-27 16:35:06
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/27ab8aa34201
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150302.042723
Firmware Date          Mon Mar  2 04:27:34 EST 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150302010223
Gaia Revision          f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f
Gaia Date              2015-02-27 15:48:31
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eea6188b9b05
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150302.043726
Firmware Date          Mon Mar  2 04:37:37 EST 2015
Bootloader             L1TC000118D0