Bug 111074 – REGRESSION(r141450): failed ASSERT in FrameView::scheduleRelayout()

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Alberto Garcia]

I have a crash because of this failed assertion in
FrameView::scheduleRelayout():

ASSERT(m_frame->view() == this)

This seems to have been introduced in r141450, which was the fix for
bug 107922. I cannot reproduce the crash after reverting that patch.

The call to frameView->setUseFixedLayout(useFixedLayout) in
Frame::createView() triggers a call to scheduleRelayout() while m_view
is 0.

Here's the backtrace:

#0  WebCore::FrameView::scheduleRelayout (this=0x80a2d18)
    at /home/berto/devel/code/webkit/Source/WebCore/page/FrameView.cpp:2312
#1  0x7c8ec55a in WebCore::RenderObject::scheduleRelayout (this=0x8233890)
    at /home/berto/devel/code/webkit/Source/WebCore/rendering/RenderObject.cpp:2650
#2  0x7c8e46ce in WebCore::RenderObject::markContainingBlocksForLayout (
    this=0x8233890, scheduleRelayout=true, newRoot=0x0)
    at /home/berto/devel/code/webkit/Source/WebCore/rendering/RenderObject.cpp:709
#3  0x78e2b104 in WebCore::RenderObject::setNeedsLayout (this=0x8233890,
    needsLayout=true, markParents=WebCore::MarkContainingBlockChain)
    at /home/berto/devel/code/webkit/Source/WebCore/rendering/RenderObject.h:1191
#4  0x7c626a1a in WebCore::FrameView::setNeedsLayout (this=0x82b8b88)
    at /home/berto/devel/code/webkit/Source/WebCore/page/FrameView.cpp:2439
#5  0x7c625b5e in WebCore::FrameView::contentsResized (this=0x82b8b88)
    at /home/berto/devel/code/webkit/Source/WebCore/page/FrameView.cpp:2122
#6  0x7c6b02ec in WebCore::ScrollView::setUseFixedLayout (this=0x82b8b88,
    enable=true)
    at /home/berto/devel/code/webkit/Source/WebCore/platform/ScrollView.cpp:324
#7  0x7c61875e in WebCore::Frame::createView (this=0x80a6f38,
    viewportSize=..., backgroundColor=..., transparent=false,
    fixedReportedSize=..., fixedLayoutSize=..., fixedVisibleContentRect=...,
    useFixedLayout=true, horizontalScrollbarMode=WebCore::ScrollbarAlwaysOff,
    horizontalLock=true, verticalScrollbarMode=WebCore::ScrollbarAlwaysOff,
    verticalLock=true)
    at /home/berto/devel/code/webkit/Source/WebCore/page/Frame.cpp:804
...

[Alexandre Elias]

Looks like the problem is the FrameView has not yet been attached to the Frame.  Does it fix the problem if in Frame::createView, you move the three fixedLayout...() calls after setView(frameView)?  That would be my proposed fix.

By the way, what port is this?

[Alberto Garcia]

(In reply to comment #1)
> Looks like the problem is the FrameView has not yet been attached to
> the Frame.  Does it fix the problem if in Frame::createView, you
> move the three fixedLayout...() calls after setView(frameView)?
> That would be my proposed fix.

Hmmm... that doesn't seem to solve the problem.

Now the new FrameView is attached to the frame, but during
FrameView::setNeedsLayout() the renderView's setNeedsLayout() is also
called.

That ends up in RenderObject::scheduleRelayout(), the pointer to the
FrameView there is different from the one we just created, so the
assertion also fails.

#0  0x7c826352 in WebCore::FrameView::scheduleRelayout (this=0x80a2c20)
    at /home/berto/devel/code/webkit/Source/WebCore/page/FrameView.cpp:2313
#1  0x7caec582 in WebCore::RenderObject::scheduleRelayout (this=0x8247360)
    at /home/berto/devel/code/webkit/Source/WebCore/rendering/RenderObject.cpp:2650
#2  0x7cae46f6 in WebCore::RenderObject::markContainingBlocksForLayout (
    this=0x8247360, scheduleRelayout=true, newRoot=0x0)
    at /home/berto/devel/code/webkit/Source/WebCore/rendering/RenderObject.cpp:709
#3  0x7952b104 in WebCore::RenderObject::setNeedsLayout (this=0x8247360,
    needsLayout=true, markParents=WebCore::MarkContainingBlockChain)
    at /home/berto/devel/code/webkit/Source/WebCore/rendering/RenderObject.h:1191
#4  0x7c826a42 in WebCore::FrameView::setNeedsLayout (this=0x8468210)
    at /home/berto/devel/code/webkit/Source/WebCore/page/FrameView.cpp:2438
#5  0x7c825bae in WebCore::FrameView::contentsResized (this=0x8468210)
    at /home/berto/devel/code/webkit/Source/WebCore/page/FrameView.cpp:2124
#6  0x7c8b0314 in WebCore::ScrollView::setUseFixedLayout (this=0x8468210,
    enable=true)
    at /home/berto/devel/code/webkit/Source/WebCore/platform/ScrollView.cpp:324
#7  0x7c8187b4 in WebCore::Frame::createView (this=0x80a6cf0,
    viewportSize=..., backgroundColor=..., transparent=false,
    fixedReportedSize=..., fixedLayoutSize=..., fixedVisibleContentRect=...,
    useFixedLayout=true, horizontalScrollbarMode=WebCore::ScrollbarAlwaysOff,
    horizontalLock=true, verticalScrollbarMode=WebCore::ScrollbarAlwaysOff,
    verticalLock=true)
    at /home/berto/devel/code/webkit/Source/WebCore/page/Frame.cpp:808
#8  0x7955706c in WebCore::FrameLoaderClientBlackBerry::transitionToCommittedForNewPage (this=0x8091518)
    at /home/berto/devel/code/webkit/Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:470
#9  0x7c74064c in WebCore::FrameLoader::transitionToCommitted (this=0x80a6d40,
    cachedPage=...)
    at /home/berto/devel/code/webkit/Source/WebCore/loader/FrameLoader.cpp:1893

> By the way, what port is this?

BlackBerry

[Alexandre Elias]

So far I haven't been able to figure out via code inspection why the RenderView's frame may be different, so I'm not sure about the root cause.  But to address this regression, one thing we can do is delete the "contentsResized();" call from "ScrollView::setUseFixedLayout()".  I only need to keep the other one in "ScrollView::setFixedLayoutSize()".  Could you check that fixes it?

[Alexandre Elias]

On second thought, before we do that, a better fix would be to add "if (!frame()->view()) return;" to "contentsResized()".  I believe it should work if you move back the calls in Frame::createView() to their original place.  Please give that a try.

[Alberto Garcia]

Created attachment 191168 [details]
Patch

(In reply to comment #4)
> On second thought, before we do that, a better fix would be to add
> "if (!frame()->view()) return;" to "contentsResized()".

Hey, this seems to solve the problem, thanks!

[James Robinson]

Comment on attachment 191168 [details]
Patch

What's the testcase?

[Xan Lopez]

(In reply to comment #6)
> (From update of attachment 191168 [details])
> What's the testcase?

Maybe Alexandre can suggest what kind of test we could write for this particular regression.

[Alexandre Elias]

Seems impossible to test with a layout test given that it only happens in the Frame constructor with port-specific arguments.  If Blackberry has a unit test system that runs on EWS, you can construct a Frame there, but AFAIK you don't, so I would be fine with submitting as is.

[Darin Adler]

Comment on attachment 191168 [details]
Patch

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

review- because this is too mysterious to do without a comment, and is probably not exactly the right fix

> Source/WebCore/page/FrameView.cpp:2061
> +    if (!frame()->view())
> +        return;

The strange thing about this is that we could add a check like this to all FrameView functions. We need a rule to tell us which ones need this. It’s particularly important because you are not contributing a test for this, so no one would be able to tell functions that need this kind of check from functions that don’t.

Further, it’s non-obvious why taking a round trip to check for null is the right thing here. This needs a comment, not just the code, because it’s non-obvious. Further, maybe it should be a check for frame()->view() == this instead of frame()->view() being non-null.

What’s special about contentsResized that means this is needed? Maybe the check belongs at the caller instead of inside this function?

[Darin Adler]

Comment on attachment 191168 [details]
Patch

A step in the right direction, but still needs work.

[Alexandre Elias]

There's a similar if() statement in FrameView::visibleContentsResized() for the same reason (ends up getting called from Frame::createView), with a comment.  So this fix is at least consistent with what was there already (I agree it's somewhat strange though).

[Alberto Garcia]

(In reply to comment #9)
> review- because this is too mysterious to do without a comment, and
> is probably not exactly the right fix

I think your objections are reasonable.

If I find more information that can make us come up with a better fix
I'll post it here.

One thing to note, though, is that if I compile with assertions
disabled then everything seems to work normally.

Maybe that is a false assertion in FrameView::scheduleRelayout() ?

[Simon Fraser (smfr)]

I don't think it's a false assertion. It implies that something happened that is unexpected.

[Chris Dumez]

FYI, The same assertion is easily reproduced in EFL's MiniBrowser if we use CacheModelPrimaryWebBrowser cache model. We currently don't get the assertion with CacheModelDocumentViewer cache model.

[Chris Dumez]

Created attachment 192962 [details]
Alternative patch

This patch fixes the assertion hit for me.

[Alexandre Elias]

Seems reasonable to me (although I'm not a WebKit reviewer).

Since this approaches fixes the underlying problem, please also delete the "if (!frame()->view()) return;" statement in FrameView::visibleContentsResized() which was a workaround for the same issue.

[Simon Fraser (smfr)]

Comment on attachment 192962 [details]
Alternative patch

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

> Source/WebCore/ChangeLog:11
> +        Update RenderView so it retrieves the FrameView from the Document instead
> +        of storing it as a member upon construction. This makes sure that the
> +        FrameView returned by the RenderView always matches the one of the
> +        Document.

But this implies that the RenderView can retrieve a different FrameView at different times, which is totally unexpected. I think this change is just papering over the cracks. We need to understand why the wrong FrameView is retrieved when the assertion happens.

[Mikhail Pozdnyakov]

*** Bug 112921 has been marked as a duplicate of this bug. ***

[Mikhail Pozdnyakov]

(In reply to comment #17)
> (From update of attachment 192962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192962&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Update RenderView so it retrieves the FrameView from the Document instead
> > +        of storing it as a member upon construction. This makes sure that the
> > +        FrameView returned by the RenderView always matches the one of the
> > +        Document.
> 
> But this implies that the RenderView can retrieve a different FrameView at different times, which is totally unexpected. I think this change is just papering over the cracks. We need to understand why the wrong FrameView is retrieved when the assertion happens.

The wrong frame view is retrieved because FrameView is created several times for the same frame. The frame however always has the same document instance which has the same renderer (RenderView) and RenderView has the same pointer to FrameView (which is out-dated).
 
There are several ways to fix it, and I am not sure which is the best now. Do we really need different FrameView instances for the same frame? Cannot we re-use the existing one instead of re-creating it? And if we do need to re-create FrameView, we probably need to re-create some other objects (document renderer at least because it contains invalid data otherwise).

(I was uploading the exactly same patch to bug#112921, but noticed that the current bug is already addressing the issue :) ).

[Mikhail Pozdnyakov]

Created attachment 196476 [details]
another alternative

[Simon Fraser (smfr)]

Comment on attachment 196476 [details]
another alternative

This is wrong too; you're throwing away a layout that should happen at some point. I think the real issue is the call to setUseFixedLayout() on the page that's going into the page cache. Why does that happen?

[Simon Fraser (smfr)]

So http://trac.webkit.org/changeset/141450 caused setUseFixedLayout() to trigger layout, which seems to be the cause of this bug.

[Alexandre Elias]

(In reply to comment #22)
> So http://trac.webkit.org/changeset/141450 caused setUseFixedLayout() to trigger layout, which seems to be the cause of this bug.

Yes, in #3 I suggested deleting the call in setUseFixedLayout().  This seems reasonable assuming setUseFixedLayout isn't used in a dynamic manner (which no one does to my knowledge).

[Mikhail Pozdnyakov]

(In reply to comment #23)
> (In reply to comment #22)
> > So http://trac.webkit.org/changeset/141450 caused setUseFixedLayout() to trigger layout, which seems to be the cause of this bug.
> 
> Yes, in #3 I suggested deleting the call in setUseFixedLayout().  This seems reasonable assuming setUseFixedLayout isn't used in a dynamic manner (which no one does to my knowledge).

Unfortunately setUseFixedLayout is used in dynamic manner in WK2 (pls. see void WebPage::setUseFixedLayout(bool fixed) from ./Source/WebKit2/WebProcess/WebPage/WebPage.cpp )

[Mikhail Pozdnyakov]

(In reply to comment #23)
> (In reply to comment #22)
> > So http://trac.webkit.org/changeset/141450 caused setUseFixedLayout() to trigger layout, which seems to be the cause of this bug.
> 
> Yes, in #3 I suggested deleting the call in setUseFixedLayout().  This seems reasonable assuming setUseFixedLayout isn't used in a dynamic manner (which no one does to my knowledge).

The real problem is that frame's document is not updated by the time re-layout is triggered. Checking document for being in cache before re-layout seems to be an appropriate workaround (for me at least :) ). 

The ideal solution would be changing frame loader logic so that the view is re-created after the appropriate document had been set, but this looks like a quite significant change of the commonly used (and very complex!) code with a huge risk of various regressions.