Interesting!  I wonder if this is related to  issue 730103 ?  I noticed that I'm able to scroll the inner iframe (at least on ChromeOS with trackpad scrolling) if I start the arrow over it, but not if it starts over the main frame or the outer iframe.

For step 2, of the repro steps, I'm using http://csreis.github.io/tests/cross-site-iframe.html, and then typing the following into DevTools:
navFrame("https://csreis.github.io/tests/cross-site-iframe-simple.html")

Maybe check if this is resolved by the WheelScrollLatchingAndAsyncWheelEvents field trial, as with  issue 730103 ?

Huh.  Comment 1 seems to be limited to ChromeOS trackpad scrolling?  I see it on 61.0.3163.123 (ChromeOS Stable) and 64.0.3270.0 (ChromeOS Dev).

I can confirm that the inner iframe doesn't scroll at all on 63.0.3239.52 (Windows Beta) or 64.0.3273.3 (Windows Canary).

Adding sahel@, just in case the scroll latching work helps at all with this issue. (Would be worth confirming.)

This seems to happen with and without scroll latching.

I notice that adding a wheel listener to the body of the inner iframe causes it to be scrollable again.

After some thought, and taking into account #c5, I think this is a ScrollingCoordinator issue. Specifically, we aren't setting non-fast scrollable regions properly for the local subframe, since OOPIFs don't have a (properly working) ScrollingCoordinator.

The event goes to the OOPIF's layer tree in the compositor, and assuming the local subframe doesn't get its own composited layer, then the OOPIF's compositor doesn't know how to fast-path scroll the subframe (or at least doesn't know to bounce it back to the main thread where it can be handled).

Adding a JS handler *forces* the wheel event to the main thread, thus insuring it gets handled properly.

Here's the related bug:

https://bugs.chromium.org/p/chromium/issues/detail?id=680606

Comments 6-7: Do you think there's anything we can do in the short term?  As I understand it, fixing issue 680606 is a fairly large task.

Not entirely sure. Two options (possibly neither trivial) are:

1) Get all iframes in OOPIFs promoted to their own compositor layers. Then fast path scrolling would be fine. But this might not be enough, e.g. it turns out a scrollable div inside an oopif also does not scroll ... again because it's not marked as a non-fast scrollable region. So we'd need to promote all scrollable elements to layers ... probably not feasible.

2) Mark the entire oopif layer as non-fast scrollable region ... but this turns off fast-path scrolling for all oopifs ... it would be (hopefully) a short-term mitigation.

3) Perhaps the best option is to refactor ScrollCoordinator to extract the Non-Fast Scrollable regions code, and then make it work for OOPIFs too. I think the answer to 680606 was always going to require 'breaking up' ScrollingCoordinator, so this might be a (somewhat smaller) first-step towards that?

(2) is the quickest, but (3) is the best.

Thanks James!

Since this affects basically anything scrollable within any OOPIF (divs, local subframes), it seems like something we should try to get a fix in for in M64.  Option 2 seems like the most expedient way to do that (turning off fast-path scrolling for OOPIFs).  Then we can do option 3 to turn the fast path back on as soon as we can, but without the time pressure of the branch on Nov 30.

Do you have a sense for how much slower the slow path is?  I imagine the tradeoff here is that scrolling the OOPIF itself will get a bit slower than today, but anything inside the OOPIF will be possible to scroll (unlike today).

I don't suppose it's possible to limit the fix to the case that the OOPIF has something scrollable inside it in the first place?  That might allow most OOPIFs to stay on the fast path, if we can detect it.

creis@ - limiting to the case of only using the slow-scroll path when there's something scrollable inside in the first place is roughly the same task as (3).

kenrb@ - I'd like to see if splitting the non-fast scrollable region computation out of ScrollingCoordinator is manageable (after taking a look through the code, I think it may well be), so I'm happy to take this on to see if I can come up with something that works.

I was wondering if it might be simpler just to pass a pointer to the local frame root into ScrollingCoordinator::UpdateAfterCompositingChangeIfNeeded(), SetShouldHandleScrollGestureOnMainThreadRegion(), etc, so they stop trying to do everything on the main frame. The only problem there is that there are dirty bits in the ScrollingCoordinator state that they check, which would have to be replicated for each local frame root.

I agree that option (3) is the right one, and doesn't look like more than a few days of work. (1) would be bad for memory usage and (2) doesn't entirely work.

Re c#12 - That's exactly my plan for (at least) the first pass at all this; and you're right about the dirty-bits being the potentially trickier part of all this ... but otherwise it looks like it can pretty much all work on the LocalRoot for the OOPIF. We may have a bit of wiggle room, in that if some of the invalidations are hard, we can see how much performance we'd need to sacrifice to be conservative in our setting of those bits in some cases.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede

commit 99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Fri Dec 01 14:44:43 2017

Refactor ScrollingCoordinator to use Frames for NonFastScrollableRegions

The net effect of this CL is to allow scrollable iframes/divs within
an OOPIF subframe, and which do not have their own composited layer for
scrolling, to be able to scroll.

This CL converts ScrollingCoordinator::UpdateAfterCompositingChangeIfNeeded
to take a LocalFrameView* instead of just defaulting to the main frame's
view, so that changes for OOPIF subframes can also be accomodated. All
relevant call sites are updated, and a no-longer-needed hack regarding
NonFastScrollableRegions is removed from RenderWidgetCompositor as a result.

Bug:  787498 
Change-Id: I608cfcdc7f7a4edd6469f9c8fed41d9ac0cf9706
Reviewed-on: https://chromium-review.googlesource.com/786307
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520934}
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/content/renderer/gpu/render_widget_compositor.cc
[add] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/content/test/data/tall_page_with_local_iframe.html
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/testing/buildbot/filters/mus.content_browsertests.filter
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/testing/buildbot/filters/viz.content_browsertests.filter
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/exported/WebPluginContainerImpl.cpp
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/frame/LocalFrameView.h
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp
[modify] https://crrev.com/99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede/third_party/WebKit/Source/core/testing/Internals.cpp

This should be fixed now on ToT, but M64 branched at r520840 so this will need to be merged.

This has baked in canary for a couple of days now, no issues apparent.

Approving merge to M64 Chrome OS.

Approving merge for Desktop as well. Branch:3282. 
Can we please have this tested and verified in next week's Dev? Since this seems to be a large refactor, my general concern is if there could be other scrolling related regressions introduced. Let's monitor closely next week. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/558f04e532921d27530d354393b55ab54f94683f

commit 558f04e532921d27530d354393b55ab54f94683f
Author: W. James MacLean <wjmaclean@chromium.org>
Date: Fri Dec 08 20:50:40 2017

Refactor ScrollingCoordinator to use Frames for NonFastScrollableRegions

The net effect of this CL is to allow scrollable iframes/divs within
an OOPIF subframe, and which do not have their own composited layer for
scrolling, to be able to scroll.

This CL converts ScrollingCoordinator::UpdateAfterCompositingChangeIfNeeded
to take a LocalFrameView* instead of just defaulting to the main frame's
view, so that changes for OOPIF subframes can also be accomodated. All
relevant call sites are updated, and a no-longer-needed hack regarding
NonFastScrollableRegions is removed from RenderWidgetCompositor as a result.

TBR=wjmaclean@chromium.org

(cherry picked from commit 99c4ba42a125fee6f4b0c7c86cdda9cc1b175ede)

Bug:  787498 
Change-Id: I608cfcdc7f7a4edd6469f9c8fed41d9ac0cf9706
Reviewed-on: https://chromium-review.googlesource.com/786307
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#520934}
Reviewed-on: https://chromium-review.googlesource.com/818118
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#104}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/content/renderer/gpu/render_widget_compositor.cc
[add] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/content/test/data/tall_page_with_local_iframe.html
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/testing/buildbot/filters/mus.content_browsertests.filter
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/testing/buildbot/filters/viz.content_browsertests.filter
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/exported/WebPluginContainerImpl.cpp
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/frame/LocalFrameView.h
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp
[modify] https://crrev.com/558f04e532921d27530d354393b55ab54f94683f/third_party/WebKit/Source/core/testing/Internals.cpp

Verified the issue on windows 10 , Mac OS 10.12.6 and ubuntu 14.04 using chrome M64 #64.0.3282.24 and observed that inner iframe is scrolled when scrolled using mouse scroll.

Attaching screencast for reference.

Adding TE-Verified labels.

Thanks!

Deleted: 787498.mp4 243 KB

	787498.mp4 243 KB View Download