Bug 148690 - http/tests/navigation/anchor-frames-same-origin.html is flaky
Summary: http/tests/navigation/anchor-frames-same-origin.html is flaky
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-01 14:51 PDT by Chris Dumez
Modified: 2018-11-05 15:07 PST (History)
3 users (show)

See Also:


Attachments
Temporary workaround (1.90 KB, patch)
2015-09-01 22:16 PDT, Chris Dumez
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-09-01 14:51:46 PDT
http/tests/navigation/anchor-frames-same-origin.html passes when run after http/tests/navigation/anchor-frames-cross-origin.html (case a) but fails when run on its own (case b).

The test uses the following frame structure:
- main Frame
-- child Frame (100x100px)
--- grand-child frame (8000x8000px)
----- anchor is at (0, 2000) px

When debugging it, I found that when we call FrameView::maintainScrollPositionAtAnchor(), we are in the following state:
- case a: Layout is done (renderView->needsLayout() returns false) so we call FrameView::scrollToAnchor() right away.
- case b: We need a layout, so we call layout() and only call FrameView::scrollToAnchor() after layout is done in performPostLayoutTasks().

scrollToAnchor() calls RenderLayer::scrollRectToVisible(rect) with the visible rect of the anchor we need to scroll to. This is the code that will actually scroll the view if needed and well as its parents. The logic looks like this:
===
                frameView.setScrollPosition(IntPoint(xOffset, yOffset));
                if (frameView.safeToPropagateScrollToParent()) {
                    parentLayer = ownerElement->renderer()->enclosingLayer();
                    // FIXME: This doesn't correctly convert the rect to
                    // absolute coordinates in the parent.
                    newRect.setX(rect.x() - frameView.scrollX() + frameView.x());
                    newRect.setY(rect.y() - frameView.scrollY() + frameView.y());
                    fprintf(stderr, "safeToPropagateScrollToParent, newRect: %d, %d, frameView.scrollY(): %d\n", newRect.x().toInt(), newRect.y().toInt(), frameView.scrollY());
                }
===

In case a, when we compute the parent's new Rect, we end up with 2346px as height because frameView.scrollY() returns 0 (as expected given that the content height is smaller than the frame height).
However, in case b, the new Rect has a height of 0 because frameView.scrollY() returns 2008.

For some reason, in case b, FrameView::maximumScrollPosition() returns [77, 2142] instead of [0, 0] as in case a.

Render tree in case a when scrollToAnchor() is called:
(R)elative/A(B)solute/Fi(X)ed/Stick(Y) positioned, (O)verflow clipping, (A)nonymous, (G)enerated, (F)loating, has(L)ayer, (C)omposited, (D)irty layout, Dirty (S)tyle
---G-L- --* RenderView  (0.00, 0.00) (8000.00, 8000.00) renderer->(0x11629fbe0)
-----L- --    HTML RenderBlock  (0.00, 0.00) (8000.00, 2034.00) renderer->(0x1163bdc60) node->(0x1163e52a0)
------- --      BODY RenderBody  (8.00, 8.00) (7984.00, 2018.00) renderer->(0x1163bdd10) node->(0x116335a80)
------- --        DIV RenderBlock  (0.00, 0.00) (7984.00, 2000.00) renderer->(0x1163bddc0) node->(0x116335ae0)
--AG--- --        RenderBlock  (0.00, 2000.00) (7984.00, 18.00) renderer->(0x1163bde70)
------- --          RootInlineBox  (0.00, 0.00) (265.88, 18.00) (0x1163f1b40) renderer->(0x1163bde70)
------- --            InlineTextBox  (0.00, 0.00) (261.88, 18.00) (0x116331160) run(0, 39) "This is an anchor point named "anchor1""
------- --            InlineTextBox  (261.88, 0.00) (4.00, 18.00) (0x1163311b8) run(0, 1) "."
------- --          A RenderInline renderer->(0x116343600) node->(0x1163ed5a0)
------- --            #text RenderText renderer->(0x1163a1dc0) node->(0x11634bea0) length->(39) "This is an anchor point named "anchor1""
------- --          #text RenderText renderer->(0x1163a1f20) node->(0x11634bee8) length->(4) ".\n\n\n"

====

Render tree in case b when scrollToAnchor is called:
(R)elative/A(B)solute/Fi(X)ed/Stick(Y) positioned, (O)verflow clipping, (A)nonymous, (G)enerated, (F)loating, has(L)ayer, (C)omposited, (D)irty layout, Dirty (S)tyle
---G-L- --* RenderView  (0.00, 0.00) (0.00, 0.00) renderer->(0x1152ecbe0)
-----L- --    HTML RenderBlock  (0.00, 0.00) (0.00, 2142.00) renderer->(0x1153c6840) node->(0x1153ac4e0)
------- --      BODY RenderBody  (8.00, 8.00) (0.00, 2126.00) renderer->(0x1153c68f0) node->(0x1153ac5a0)
------- --        DIV RenderBlock  (0.00, 0.00) (0.00, 2000.00) renderer->(0x1153c69a0) node->(0x1153ac600)
--AG--- --        RenderBlock  (0.00, 2000.00) (0.00, 126.00) renderer->(0x1153c6a50)
------- --          RootInlineBox  (0.00, 0.00) (28.45, 18.00) (0x1153f1aa0) renderer->(0x1153c6a50)
------- --            InlineTextBox  (0.00, 0.00) (28.45, 18.00) (0x1153460b0) run(0, 4) "This"
------- --          RootInlineBox  (0.00, 18.00) (10.67, 18.00) (0x1153f1b40) renderer->(0x1153c6a50)
------- --            InlineTextBox  (0.00, 18.00) (10.67, 18.00) (0x115346108) run(5, 7) "is"
------- --          RootInlineBox  (0.00, 36.00) (15.10, 18.00) (0x1153f1be0) renderer->(0x1153c6a50)
------- --            InlineTextBox  (0.00, 36.00) (15.10, 18.00) (0x115346160) run(8, 10) "an"
------- --          RootInlineBox  (0.00, 54.00) (43.53, 18.00) (0x1153f1c80) renderer->(0x1153c6a50)
------- --            InlineTextBox  (0.00, 54.00) (43.53, 18.00) (0x1153461b8) run(11, 17) "anchor"
------- --          RootInlineBox  (0.00, 72.00) (32.89, 18.00) (0x1153f1d20) renderer->(0x1153c6a50)
------- --            InlineTextBox  (0.00, 72.00) (32.89, 18.00) (0x115346210) run(18, 23) "point"
------- --          RootInlineBox  (0.00, 90.00) (42.65, 18.00) (0x1153f1dc0) renderer->(0x1153c6a50)
------- --            InlineTextBox  (0.00, 90.00) (42.65, 18.00) (0x115346268) run(24, 29) "named"
------- --          RootInlineBox  (0.00, 108.00) (68.59, 18.00) (0x1153f1e60) renderer->(0x1153c6a50)
------- --            InlineTextBox  (0.00, 108.00) (64.59, 18.00) (0x1153462c0) run(30, 39) ""anchor1""
------- --            InlineTextBox  (64.59, 108.00) (4.00, 18.00) (0x115346318) run(0, 1) "."
------- --          A RenderInline renderer->(0x11534c060) node->(0x1153ede88)
------- --            #text RenderText renderer->(0x115380f20) node->(0x115355dc8) length->(39) "This is an anchor point named "anchor1""
------- --          #text RenderText renderer->(0x115380f78) node->(0x115355e10) length->(4) ".\n\n\n"
Comment 1 Chris Dumez 2015-09-01 14:56:24 PDT
It looks like it fails when run on its own on WebKit2 as well.
Comment 2 Chris Dumez 2015-09-01 15:01:10 PDT
Passes:
Tools/Scripts/run-webkit-tests --release http/tests/navigation/anchor-frames-cross-origin.html  http/tests/navigation/anchor-frames-same-origin.html

Fails:
Tools/Scripts/run-webkit-tests --release http/tests/navigation/anchor-frames-same-origin.html
Comment 3 Chris Dumez 2015-09-01 22:16:32 PDT
Created attachment 260408 [details]
Temporary workaround
Comment 4 Chris Dumez 2015-09-01 22:17:18 PDT
Comment on attachment 260408 [details]
Temporary workaround

Zalan, what do you think? Can we land this in the mean time?
Comment 5 Chris Dumez 2015-09-02 10:06:39 PDT
Committed r189250: <http://trac.webkit.org/changeset/189250>
Comment 6 Chris Dumez 2015-09-02 10:07:18 PDT
Reopening as it is a temporary workaround.