1686002 - Bing image search locks up GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[codrut.topliceanu]

From https://github.com/mozilla-mobile/fenix/issues/17108

Steps to reproduce:

1. Go to bing.com
2. Search for something
3. Click on IMAGE to search for images

Expected:
-> IMAGES page should load

Actual:
-> Keyboard opens up for a split second then the whole GeckoView locks up, touch events not doing anything.

Reproduced in Fenix and GeckoView Example.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Attachment: bing2.mp4

Here is a video of the problem.

Comment 2

[Eugen Sawin [:esawin]]

This consistently reproduces with GVE.

Makoto, I'm seeing some activity in GeckoEditable after clicking on the image category. It seems to be the last Gecko activity before the session goes into a bad state. Do you see anything wrong there?

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

Kevin or Eugen, could you attach logs?

Comment 4

[Makoto Kato [:m_kato]]

Kats, do you still active? This seems to be regression by Bug 1668870. ui.touch.radius.enabled=false resolves this issue.

Comment 5

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

I can reproduce the problem, but it seems like a rather strange effect of bug 1668870. I generally have very limited time to work on Mozilla stuff these days, but I can try to take a look. I can't guarantee any sort of timeliness, so if this requires a quick resolution I'd recommend just turning off the pref until we can figure out what's going on.

Comment 6

[Botond Ballo [:botond]]

Preliminary theory based on observing the symptoms:

• The touch events intended for the "Image" link get re-targeted to the search bar above.
• However, something else that normally would happen if you actually tapped the search bar, does not happen. (Not sure what.)
• The browser gets stuck in some sort of unexpected state (maybe keyboard related).

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)

• The touch events intended for the "Image" link get re-targeted to the search bar above.

I've confirmed this much.

I can also see that the proximate cause of the "whole GeckoView locks up, touch events not doing anything" is that we start re-targeting all touch events to a large frame whose size is the whole viewport.

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)

I can also see that the proximate cause of the "whole GeckoView locks up, touch events not doing anything" is that we start re-targeting all touch events to a large frame whose size is the whole viewport.

That large frame is created by web content:

<div id="b_lbOverlay" style="width: 100%; height: 100%;"></div>

with some of the styles being:

element {
    width: 100%;
    height: 100%;
}
#b_lbOverlay {
    position: fixed;
    top: 0;
    left: 0;
    background: #000;
    opacity: 0;
    transition: opacity 300ms;
    -webkit-transition: opacity 300ms;
    -ms-transition: opacity 300ms;
    -moz-transition: opacity 300ms;
    z-index: 1001;
    display: block;
}

and it does have a click listener.

So, it looks like it's the site that gets stuck in an unexpected state where this large, high-z-index (but invisible) overlay is present and eats the events.

Comment 9

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

One possible fix might be to have GetTouchableAncestor also return when the element is "clickable". So the code here would get extracted into a boolean-returning helper, and this check would also call that helper. The effect would be that instead of ignoring the clickable-ness of the actual target and finding a nearby element with a touch listener, we would use the actual target under the finger. This reduces the impact of the touch-radius pref and might fix this regression.

Comment 10

[Botond Ballo [:botond]]

Upon reloading the page, the offending element is still present in the DOM, but its size is width: 0px; height: 0px;.

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)

One possible fix might be to have GetTouchableAncestor also return when the element is "clickable". So the code here would get extracted into a boolean-returning helper, and this check would also call that helper. The effect would be that instead of ignoring the clickable-ness of the actual target and finding a nearby element with a touch listener, we would use the actual target under the finger. This reduces the impact of the touch-radius pref and might fix this regression.

Thanks for the suggestion! In this case, the div containing the "Image" link doesn't have a click listener, but the link itself is an <a> element which is considered clickable by the event retargeting code, so that sounds like it should work. I'll give it a try.

Comment 12

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

My suggestion was slightly wrong, I realized. The goal is to avoid retargeting the touch if the thing it lands on is clickable, but what I suggested wouldn't accomplish that. Instead you'd want to avoid calling GetClosest entirely if the initial target is clickable.

Comment 13

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

The implementation for what I had in mind turned out to be even simpler since we have existing code that does the same for mouse events. https://treeherder.mozilla.org/jobs?repo=try&revision=aab484f809b5986396947ed50bb28221840b329a

I verified locally that this passes test_event_target_radius.html (on desktop) but I was having trouble with my local Android build so I haven't yet checked that it actually fixes the bug.

Comment 14

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

The GVE build in the try push above seems to fix the bug. I couldn't reproduce the problem as originally described in a GVE from m-c but when I follow the STR clicking on the images tab has no effect at all. With the GVE build in the try push it works as expected.

I'll try to write a test for this although I don't know how representative it will be of the real world scenario.

Comment 15

[Botond Ballo [:botond]]

I wrote up the same fix (I had overlooked the try link in comment 13, oops), and I can both repro the original issue and confirm that the fix addresses it.

I'll try to write a test for this although I don't know how representative it will be of the real world scenario.

Much appreciated!

Comment 16

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

https://treeherder.mozilla.org/jobs?repo=try&revision=5e2343337cfeff88d586d7cb403e1f0ef6c04b86

Comment 17

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

Attachment: Bug 1686002 - Don't retarget touches if they land inside a clickable element. r?botond

Touches that land on a descendant of a clickable element should
not get retargeted to other nearby elements because the descendant
itself can react to those touch events.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05dd3bdbd228
Don't retarget touches if they land inside a clickable element. r=botond

Comment 19

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/05dd3bdbd228