1606108 - Autoscroll works only once in iframe after bug 1597765 landed

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Denis Lisov]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

0. Enable autoscroll in preferences
1. Install the Brief feed reader addon: https://addons.mozilla.org/en-US/firefox/addon/brief/
2. Open (not in a private window) https://act.eff.org/action.atom and wait for the feed entries to show up (should be a few seconds)
3. Try to scroll the list by middle-clicking the left margin to use autoscroll for a few times

Actual results:

The first time autoscroll activates successfully and works correctly. However, after you stop it nothing happens when you middle-click to start autoscrolling again.

I've run mozregression and the regression range turned out to be
6:13.33 INFO: Last good revision: 2fa2cb1fe36bc4e85ae8920c366ef2749f182ffc
6:13.33 INFO: First bad revision: 2c56e1bfbff982873521af50eb62c7bf8f300cf1
6:13.33 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2fa2cb1fe36bc4e85ae8920c366ef2749f182ffc&tochange=2c56e1bfbff982873521af50eb62c7bf8f300cf1
i.e. only bug 1597765

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: auto scrolling broken

I can reproduce the issue on Nightly73.0a1 Windows10.

Steps To Reproduce:

1. Open data:text/html,<iframe src="https://www.wikipedia.org/" title="iframe Example 1" width="600" height="600"></iframe>
2. Try to scroll the iframe by middle-clicking, and cancel by middle-clicking
3. And then try again step 2

Comment 2

[Alice0775 White]

alexander, your patch seems to cause the regression, can you please look into this?

Comment 3

[Alice0775 White]

Another problem I found, if parent windows has scrollbar, the iframe autoscroll would not stop.

1. open data:text/html,<iframe src="https://www.wikipedia.org/" title="iframe Example 1" width="600" height="600"></iframe><div style="height:2000px" />
2. Try to scroll the iframe by middle-clicking, and cancel by middle-clicking
3. Mouse move over iframe

Actual Results:
Autoscroll marker is dismissed as expected at step 2.
However, the iframes continue to auto-scroll by mouse moving --- BUG

Comment 4

[alexander :surkov (:asurkov)]

Apparently Autoscroll:Stop message is not sent to a child actor [1], and thus AutoScroll fails to stop autoscrolling correctly. When sendMessageToActor attempts to send a message to a child browsing context, it quits on windowGlobal.isProcessRoot check. Neil, what this check is about and how to workaround it?

[1] https://searchfox.org/mozilla-central/source/toolkit/content/widgets/browser-custom-element.js#1626
[2] https://searchfox.org/mozilla-central/source/toolkit/content/widgets/browser-custom-element.js#2105

Comment 5

[alexander :surkov (:asurkov)]

p1 since make autoscroll useless

Comment 6

[Neil Deakin]

I suspect you actually are trying to have that message sent to all descendant frames and not all descendant processes. The 'all' flag of 'true' only sends the message to frames that have a different process than their parent. You might consider adding a flag or altering the existing one in some way to support sending to all child frames as well.

But I think you only need to send the message to the one browsing context that currently is using autoscroll? That isn't cached anywhere I guess?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Neil Deakin from comment #6)

But I think you only need to send the message to the one browsing context that currently is using autoscroll? That isn't cached anywhere I guess?

caching sounds like a good idea, will try this one

Comment 8

[alexander :surkov (:asurkov)]

Attachment: Bug 1606108 - Autoscroll works only once in iframe

Comment 9

[alexander :surkov (:asurkov)]

new test fails on linux, https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283215915&repo=try&lineNumber=1980, seems like we deal here with another bug. According to logs autoscroll starts, but doesn't actually scroll (https://firefoxci.taskcluster-artifacts.net/Z0u31pJ_ToioBW0YLWDzBQ/0/public/test_info//mozilla-test-fail-screenshot_bw2wTh.png) and never quits.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #9)

new test fails on linux, https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283215915&repo=try&lineNumber=1980, seems like we deal here with another bug. According to logs autoscroll starts, but doesn't actually scroll (https://firefoxci.taskcluster-artifacts.net/Z0u31pJ_ToioBW0YLWDzBQ/0/public/test_info//mozilla-test-fail-screenshot_bw2wTh.png) and never quits.

the failure is intermittent one, it turns out that scrollable area cannot be found (https://hg.mozilla.org/try/annotate/b26104f6301411fe1f52224106bb4f2bdbce1ec4/toolkit/actors/AutoScrollChild.jsm#l127). Here's relevant logs:

INFO - AutoScroll test: repeating autoscroll in iframe
INFO - GECKO(872) | console.log: "AutoScroll: mousedown"
INFO - GECKO(872) | console.log: "AutoScroll: start"
INFO - GECKO(872) | console.log: "Autoscroll: no scrollable, MaybeStartInParent"
INFO - Longer timeout required, waiting longer... Remaining timeouts: 1

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283673469&repo=try&lineNumber=1556-1561

Screenshot (https://firefoxci.taskcluster-artifacts.net/T_YUPuS0R_67g1fUdqnxvg/0/public/test_info/mozilla-test-fail-screenshot_fzi_cl.png) however shows that scrollbar is present.

Cc'ing Emilio for ideas, if I miss something evident here.

Gijs, given the intermittent nature of a new test and the regression badness, then should we perhaps take the patch without the test and file a followup for it?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Well, in https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283215915&repo=try&lineNumber=1980 we don't even get to MaybeStartInParent, right? But the test doesn't seem to be testing the STR in comment 2 anyhow? You're testing a non-scrollable iframe and scrollable root, and comment 2 has an scrollable iframe, and non-scrollable root.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Well, in https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283215915&repo=try&lineNumber=1980 we don't even get to MaybeStartInParent, right?

It appears that job doesn't have 'MaybeStartInParent' logging, https://hg.mozilla.org/try/rev/cb001b63ec3e6610c6ec416d87da13f98f77e49a

I added it later to this one https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=283673469&revision=29ad95f967c04a7d2a275290b4fe9269138569c8

But the test doesn't seem to be testing the STR in comment 2 anyhow? You're testing a non-scrollable iframe and scrollable root, and comment 2 has an scrollable iframe, and non-scrollable root.

It's a fair point, but apparently the bug is reproduable when the user starts scrolling inside iframe, not depending whether it is scrollable or its container is scrollable, I confirmed the test doesn't pass without the patch. I suppose I can add a second testcase, which is identical to the original report.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Ok, that should get to MaybeStartInParent then, right? the <iframe> is not scrollable. so it's the parent not receiving the message or something?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

Ok, that should get to MaybeStartInParent then, right? the <iframe> is not scrollable. so it's the parent not receiving the message or something?

nah, we shouldn't get into MaybeStartInParent, because it is a single process and thus we should never reach MaybeStartInParent, since we should have a scrollable area. But because (intermittently!) we find no scrollable area, then we attempt to make MaybeStartInParent, which does nothing. Here's relevant part of green log:

INFO - AutoScroll test: repeating autoscroll in iframe
INFO - GECKO(7416) | console.log: "AutoScroll: mousedown"
INFO - GECKO(7416) | console.log: "AutoScroll: start"
INFO - GECKO(7416) | console.log: "Autoscroll: started"
INFO - TEST-PASS | toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | Browser still focused after autoscroll started -
INFO - timestamp=53699.33460984567 firstTimestamp=53699.33460984567 timeCompensation=0
INFO - timestamp=53766.08311950262 firstTimestamp=53699.33460984567 timeCompensation=3.3374254828475385
INFO - timestamp=53793.08912948434 firstTimestamp=53699.33460984567 timeCompensation=4.687725981933545
INFO - timestamp=53861.64582370106 firstTimestamp=53699.33460984567 timeCompensation=8.115560692769577
INFO - GECKO(7416) | console.log: AutoScroll: stopScroll in browser CE
INFO - GECKO(7416) | console.log: "AutoScroll: stop"
INFO - TEST-PASS | toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js | Window for scrollable should have scrolled vertically

https://firefoxci.taskcluster-artifacts.net/CfQ4dCVjTu-tTkZfoTCdUw/0/public/logs/live_backing.log

Comment 15

[:Gijs (he/him)]

(In reply to alexander :surkov (:asurkov) from comment #10)

Gijs, given the intermittent nature of a new test and the regression badness, then should we perhaps take the patch without the test and file a followup for it?

That would also work for me, though note there are some other comments in phab on the patch. Perhaps the test needs to wait for layout to have run inside the iframe or something?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8340e873c59
Autoscroll works only once in iframe r=Gijs

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f8340e873c59

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 9118073 [details]
Bug 1606108 - Autoscroll works only once in iframe

Beta/Release Uplift Approval Request

• User impact if declined: autoscroll is broken when iframe or object are involved (see this bug, bug 1607927)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: refer to this bug description and bug 1607927
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): medium risk: changes in code having moderate tests coverage
• String changes made/needed: no

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9118073 [details]
Bug 1606108 - Autoscroll works only once in iframe

Fixes a couple different reported autoscroll regressions. Approved for 73.0b5.

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1c2ccb55ada3

Comment 24

[Negritas Sergiu, Desktop QA]

I have managed to reproduce the issue on Ubuntu 18.04 LTS using Fx 73.0a1(2019-12-27). I can confirm that the issue is fixed in latest nightly and also on the latest beta available in treeherder https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta on all OS platforms.