Bug 167830 – WebContent process repeatedly jetsams on BuzzFeed's Another Round page

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ryosuke Niwa]

Visit https://www.buzzfeed.com/anotherround on iOS.
It always jetsams :(

[Ryosuke Niwa]

<rdar://problem/30187368>

[Ryosuke Niwa]

Created attachment 300600 [details]
Fixes the bug

[Ryosuke Niwa]

I don't know this code can be tested. Any advice will be appreciated.

[WebKit Commit Bot]

Attachment 300600 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayerCompositor.cpp:2760:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 300600 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=300600&action=review

Did you test whether scrolling iframes with position:fixed content works on Mac with this change? I suspect you'll get bad repaints because FrameView::useSlowRepaints isn't set up to handle non-composited fixed in WK2.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2758
> +    RELEASE_LOG(Compositing, "Layer = %p layerBounds (x=%f, y=%f, w=%f, h=%f) viewBounds (x=%f, y=%f, w=%f, h=%f)", &layer,
> +                layerBounds.x().toFloat(), layerBounds.y().toFloat(), layerBounds.width().toFloat(), layerBounds.height().toFloat(),
> +                viewBounds.x().toFloat(), viewBounds.y().toFloat(), viewBounds.width().toFloat(), viewBounds.height().toFloat());

I don't think you want this logging in release.

[Simon Fraser (smfr)]

(In reply to comment #3)
> I don't know this code can be tested. Any advice will be appreciated.

Yes, you can dump the layer tree (see layerTreeAsText).

[Ryosuke Niwa]

(In reply to comment #5)
> Comment on attachment 300600 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300600&action=review
> 
> Did you test whether scrolling iframes with position:fixed content works on
> Mac with this change? I suspect you'll get bad repaints because
> FrameView::useSlowRepaints isn't set up to handle non-composited fixed in
> WK2.

It looks like this is working just fine. I'll add a test anyway. Now I realize that we probably want to do this only on the layer of the flattened iframe, and but not on a random stuff inside another scrollable element inside any iframe.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2758
> > +    RELEASE_LOG(Compositing, "Layer = %p layerBounds (x=%f, y=%f, w=%f, h=%f) viewBounds (x=%f, y=%f, w=%f, h=%f)", &layer,
> > +                layerBounds.x().toFloat(), layerBounds.y().toFloat(), layerBounds.width().toFloat(), layerBounds.height().toFloat(),
> > +                viewBounds.x().toFloat(), viewBounds.y().toFloat(), viewBounds.width().toFloat(), viewBounds.height().toFloat());
> 
> I don't think you want this logging in release.

Oops, removed.

[Ryosuke Niwa]

Ugh... that doesn't quite work :(

[Ryosuke Niwa]

It looks like we can't test this change without modifying layerTreeAsText or adding a new flag :( There's no difference in layerTreeAsText regardless of which existing flag I use.

[Ryosuke Niwa]

In fact, LAYER_TREE_INCLUDES_TILE_CACHES is just as useless. Somehow WTR/DRT claims the tile doesn't exit when running the test even though I can see the tile getting created on the device :(

[Ryosuke Niwa]

Created attachment 300790 [details]
Test case (attempt; doesn't work)

[Ryosuke Niwa]

Created attachment 300791 [details]
Fixes the bug

[Simon Fraser (smfr)]

LAYER_TREE_INCLUDES_TILE_CACHES dumps TiledBacking data, which only kicks in when layers get large (over 2K on a side).

Did the commit that added the "backing store attached" logic add any tests?

[Simon Fraser (smfr)]

Comment on attachment 300791 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=300791&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:383
> +    // Force the coverage rect to be updated since setVisibleAndCoverageRects exits early when intersectsCoverageRect didn't change.

I don't think the comment is necessary, actually. It's easy to deduce why CoverageRectChanged needs to be set when looking at the code, and this means it could go in the initializer.

[Build Bot]

Comment on attachment 300791 [details]
Fixes the bug

Attachment 300791 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3018585

New failing tests:
compositing/masks/solid-color-masked.html
pageoverlay/overlay-remove-reinsert-view.html

[Build Bot]

Created attachment 300798 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Hm... these test failures look related to my change but I can't reproduce them locally.

[Ryosuke Niwa]

Created attachment 300847 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 300847 [details]
Patch for landing

Let's wait for EWS again and see if the test failure persists.

[Ryosuke Niwa]

Oh, I don't have that overlay test :( Updating the checkout...

[Build Bot]

Comment on attachment 300847 [details]
Patch for landing

Attachment 300847 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3021668

New failing tests:
pageoverlay/overlay-remove-reinsert-view.html

[Build Bot]

Created attachment 300853 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Okay, I'm rebaselining the test after talking with Simon. This test is kind of testing my fix I suppose. It's still sad that we can't directly test this case.

[Ryosuke Niwa]

Committed r211845: <http://trac.webkit.org/changeset/211845>

[Brent Fulgham]

(In reply to comment #3)
> I don't know this code can be tested. Any advice will be appreciated.

It turns out this broke Windows pretty significantly. I'm investigating why.

[Simon Fraser (smfr)]

It looks like Per already did: bug 168654