1512838 - Resolution doesn't get applied after one touch pinch

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

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

Fennec nightly, do a one touch pinch on a bugzilla page. It remains blurry. Looks like the main thread doesn't take the new resolution, because pinching afterwards resets the zoom back to what it was before the one touch pinch.

Comment 1

[Botond Ballo [:botond]]

Not a recent regression: issue goes at least as far back as Firefox 62.

Comment 2

[Botond Ballo [:botond]]

And Firefox 55, which is around the time one-touch pinch was introduced.

Comment 3

[Botond Ballo [:botond]]

Though it's not the case that one-touch pinch is just completely broken, as e.g. on planet.mozilla.org it works fine (increasing resolution and all).

Comment 4

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

Maybe we're trying to apply the zoom on the scrollable div instead of the RCD.

Comment 6

[csheany]

Bug 1111333 - the original for reference

Comment 8

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

The patch for this is simple enough. Time to write some tests! rolls up sleeves

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=a4c9cd8a8b46bf1d972cf5bcab97c55a1e7ecfb7

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=7b1f6beb0fcf5379d72c5912d8f02733f4b469c1 is better

Comment 11

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

Attachment: Bug 1512838 - Redirect one-touch-pinch scale gestures to the root content APZC. r?botond

Comment 12

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

Attachment: Bug 1512838 - Skip over empty transaction buckets in isLayerized. r?botond

Sometimes we can get empty transactions after a scrollframe is
layerized. In such cases the isLayerized check would incorrectly detect
the scrollframe as not being layerized because it would only look at the
data for the empty transaction.

Depends on D23494

Comment 13

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

Attachment: Bug 1512838 - Add mochitests to exercise the one-touch-pinch code. r?botond

The helper_basic_onetouchpinch.html is basically a copy of
helper_basic_zoom.html with a few things changed (most importantly, the
touch event sequence).

Depends on D23495

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/754f833aaa41
Redirect one-touch-pinch scale gestures to the root content APZC. r=botond
https://hg.mozilla.org/integration/autoland/rev/6bd80d61cee8
Skip over empty transaction buckets in isLayerized. r=botond
https://hg.mozilla.org/integration/autoland/rev/589f41b2e253
Add mochitests to exercise the one-touch-pinch code. r=botond

Comment 15

[Natalia Csoregi [:nataliaCs]]

Backed out for memory leaks.

backout: https://hg.mozilla.org/integration/autoland/rev/8f2a6dbf221ca51fcb80f1a94eea41d377a0c91c

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linux%2Cx64%2Casan%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux64-asan%2Fopt-mochitest-e10s-8%2Cm-e10s%288%29&revision=589f41b2e2536f28eba24afdab49916635a9bf4e

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=234318839&repo=autoland&lineNumber=3087

Comment 16

[Botond Ballo [:botond]]

Huh. That seems to be a legitimate bug in nsDOMWindowUtils::SetDisplayPortForElement, introduced in this commit:

• We query an element property here and keep a raw pointer to it.
• We overwrite the property here, which frees the old property object.
• We subsequently dereference the pointer pointing to the old data here (this is the part that was added in the change mentioned above).

Comment 17

[Timothy Nikkel (:tnikkel)]

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

Huh. That seems to be a legitimate bug in
nsDOMWindowUtils::SetDisplayPortForElement, introduced in [this
commit](https://hg.mozilla.org/mozilla-central/rev/
484e07a716f1aac33b4fd9ee246c7eab8b504523):

-> bug 1535862.

Comment 18

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

Thanks for the quick diagnosis and fix! I'll reland these patches on Monday.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aededeb1f164
Redirect one-touch-pinch scale gestures to the root content APZC. r=botond
https://hg.mozilla.org/integration/autoland/rev/729056f6dc85
Skip over empty transaction buckets in isLayerized. r=botond
https://hg.mozilla.org/integration/autoland/rev/cb58de9731cb
Add mochitests to exercise the one-touch-pinch code. r=botond

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/aededeb1f164
https://hg.mozilla.org/mozilla-central/rev/729056f6dc85
https://hg.mozilla.org/mozilla-central/rev/cb58de9731cb