When loading a new page in the MiniBrowser with fixed layout enabled, an assert is triggered in FrameView::scheduleRelayout(). This happens because the view is currently being set and is 0 until all properties are set.
(In reply to comment #0)
> When loading a new page in the MiniBrowser with fixed layout enabled, an assert is triggered in FrameView::scheduleRelayout(). This happens because the view is currently being set and is 0 until all properties are set.
Could you paste a stacktrace on this assert? (shouldnt constructing the view be sync while scheduling the layout is async? -so by the time the timer hits, the view is all set.)
(In reply to comment #2)
> (In reply to comment #0)
> > When loading a new page in the MiniBrowser with fixed layout enabled, an assert is triggered in FrameView::scheduleRelayout(). This happens because the view is currently being set and is 0 until all properties are set.
> Could you paste a stacktrace on this assert? (shouldnt constructing the view be sync while scheduling the layout is async? -so by the time the timer hits, the view is all set.)
It is not called by timer but triggered directly by Frame calling first setView(0) and then setFixedLayoutSize which triggers contentsResized. The real view is only set later. I can post the backtrace tomorrow.
#1 0x00007ffff2051fbe in WebCore::FrameView::scheduleRelayout (this=0x5c56e0) at /src/webkit/Source/WebCore/page/FrameView.cpp:2409
#2 0x00007ffff23524ee in WebCore::RenderObject::scheduleRelayout (this=0x5dbe08) at /src/webkit/Source/WebCore/rendering/RenderObject.cpp:2728
#3 0x00007ffff23482e2 in WebCore::RenderObject::markContainingBlocksForLayout (this=0x5dbe08, scheduleRelayout=true, newRoot=0x0) at /src/webkit/Source/WebCore/rendering/RenderObject.cpp:738
#4 0x00007ffff1b77f6c in WebCore::RenderObject::setNeedsLayout (this=0x5dbe08, needsLayout=true, markParents=WebCore::MarkContainingBlockChain) at /src/webkit/Source/WebCore/rendering/RenderObject.h:1205
#5 0x00007ffff2052732 in WebCore::FrameView::setNeedsLayout (this=0xc6ab80) at /src/webkit/Source/WebCore/page/FrameView.cpp:2522
#6 0x00007ffff2051630 in WebCore::FrameView::contentsResized (this=0xc6ab80) at /src/webkit/Source/WebCore/page/FrameView.cpp:2197
#7 0x00007ffff2169c10 in WebCore::ScrollView::setUseFixedLayout (this=0xc6ab80, enable=true) at /src/webkit/Source/WebCore/platform/ScrollView.cpp:289
#8 0x00007ffff2040d6c in WebCore::Frame::createView (this=0x5ba650, viewportSize=..., backgroundColor=..., transparent=false, fixedLayoutSize=..., fixedVisibleContentRect=..., useFixedLayout=true,
horizontalScrollbarMode=WebCore::ScrollbarAlwaysOff, horizontalLock=true, verticalScrollbarMode=WebCore::ScrollbarAlwaysOff, verticalLock=true) at /src/webkit/Source/WebCore/page/Frame.cpp:781
#9 0x00007ffff17d7260 in WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage (this=0x5b4008) at /src/webkit/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1209
#10 0x00007ffff1f8e089 in WebCore::FrameLoader::transitionToCommitted (this=0x5ba6d8, cachedPage=...) at /src/webkit/Source/WebCore/loader/FrameLoader.cpp:1877
#11 0x00007ffff1f8d4c2 in WebCore::FrameLoader::commitProvisionalLoad (this=0x5ba6d8) at /src/webkit/Source/WebCore/loader/FrameLoader.cpp:1719
#12 0x00007ffff1f730cd in WebCore::DocumentLoader::commitIfReady (this=0xced830) at /src/webkit/Source/WebCore/loader/DocumentLoader.cpp:323
#13 0x00007ffff1f74f16 in WebCore::DocumentLoader::commitLoad (this=0xced830,
Comment on attachment 207332[details]
Patch
This seems like the wrong fix. It seems like setUseFixedLayout or setContentSize should instead just bail early in this case.
(In reply to comment #5)
> (From update of attachment 207332[details])
> This seems like the wrong fix. It seems like setUseFixedLayout or setContentSize should instead just bail early in this case.
There is no setContentSize call, and setUseFixedLayout() is in ScrollView and currently not virtual.
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 207332[details] [details])
> > This seems like the wrong fix. It seems like setUseFixedLayout or setContentSize should instead just bail early in this case.
>
> There is no setContentSize call, and setUseFixedLayout() is in ScrollView and currently not virtual.
I was considering moving it lower to setNeedsLayout(), so that needslayout just bails if we don't even have the first layout yet.
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 207332[details] [details] [details])
> > > This seems like the wrong fix. It seems like setUseFixedLayout or setContentSize should instead just bail early in this case.
> >
> > There is no setContentSize call, and setUseFixedLayout() is in ScrollView and currently not virtual.
>
> I was considering moving it lower to setNeedsLayout(), so that needslayout just bails if we don't even have the first layout yet.
What i think the problem here is that the ScrollView::setUseFixedLayout() tries to do too much. The contentsResized() call looks to me something that patches up some unrelated issue, which does not happen necessarily, for example when the view was just created.
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (From update of attachment 207332[details] [details] [details] [details])
> > > > This seems like the wrong fix. It seems like setUseFixedLayout or setContentSize should instead just bail early in this case.
> > >
> > > There is no setContentSize call, and setUseFixedLayout() is in ScrollView and currently not virtual.
> >
> > I was considering moving it lower to setNeedsLayout(), so that needslayout just bails if we don't even have the first layout yet.
> What i think the problem here is that the ScrollView::setUseFixedLayout() tries to do too much. The contentsResized() call looks to me something that patches up some unrelated issue, which does not happen necessarily, for example when the view was just created.
The call to contentsResized() was introduced in by the patch for bug #107922. I think the confusion here is from the fact that contentsResized() is used to trigger layout when layout size changes (when contents is being resized). The method name just wasn't updated. Consider it layoutSizeChanged().
Comment on attachment 207654[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=207654&action=review
Fixing the name confusion is a great idea, but I still think that setUseFixedLayout() should just be a setter and setFixedLayoutSize() should trigger needslayout, where we actually _set_ the size of the canvas to layout the content on. So i'd first fix the call order at Frame::createView(), turn the feature on first and set the size right after. Although, looking at transitionToCommittedForNewPage() (which calls createView()), it actually passes IntSize() as the size for the fixed layout which makes me wonder whether we need this change at all. I suspect that the actual size for the layout gets set somewhere later and this is just some leftover and should be removed.
> Source/WebCore/page/FrameView.cpp:2197
> +void FrameView::layoutSizeChanged()
I'd call it fixedLayoutSizeChanged() to make the usecase clear.
> Source/WebCore/page/FrameView.cpp:2205
> }
So we are introducing a virtual function on ScrollView just to call setNeedsLayout()?
> Source/WebCore/platform/ScrollView.cpp:249
> +
You change functionality here. Previously with fixed layout on, ScrollableArea::contentAreaResized() wasn't called (if (!m_useFixedLayout && oldRect.size() != newRect.size())).
I'd early return instead.
(In reply to comment #16)
> (From update of attachment 207654[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207654&action=review
>
> > Source/WebCore/page/FrameView.cpp:2197
> > +void FrameView::layoutSizeChanged()
>
> I'd call it fixedLayoutSizeChanged() to make the usecase clear.
>
It is also called when fixed layout is off by contentsAreaResized(), this is the standard desktop browser case.
> > Source/WebCore/page/FrameView.cpp:2205
> > }
>
> So we are introducing a virtual function on ScrollView just to call setNeedsLayout()?
Yes. Ideally we should move all the layoutSizes stuff to FrameView as ScrollView does not handle layout, but that would be a major refactoring. So for now we need these virtual methods for the layout related code in ScrollView to access the implementation in FrameView.
>
> > Source/WebCore/platform/ScrollView.cpp:249
> > +
>
> You change functionality here. Previously with fixed layout on, ScrollableArea::contentAreaResized() wasn't called (if (!m_useFixedLayout && oldRect.size() != newRect.size())).
> I'd early return instead.
There shouldn't be a real functionality change. m_useFixedLayout is now tested in contentsResized(). Though it may affect one more update of the contentArea in ScrollAnimator, but it probably should have anyway.
Created attachment 207799[details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Created attachment 207815[details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
(In reply to comment #22)
> Created an attachment (id=207835) [details]
> Patch
I couldn't find any reply to this part of my comment, so I assume you missed it.
"Fixing the name confusion is a great idea, but I still think that setUseFixedLayout() should just be a setter and setFixedLayoutSize() should trigger needslayout, where we actually _set_ the size of the canvas to layout the content on. So i'd first fix the call order at Frame::createView(), turn the feature on first and set the size right after. Although, looking at transitionToCommittedForNewPage() (which calls createView()), it actually passes IntSize() as the size for the fixed layout which makes me wonder whether we need this change at all. I suspect that the actual size for the layout gets set somewhere later and this is just some leftover and should be removed."
(In reply to comment #23)
> (In reply to comment #22)
> > Created an attachment (id=207835) [details] [details]
> > Patch
>
> I couldn't find any reply to this part of my comment, so I assume you missed it.
>
> "Fixing the name confusion is a great idea, but I still think that setUseFixedLayout() should just be a setter and setFixedLayoutSize() should trigger needslayout, where we actually _set_ the size of the canvas to layout the content on. So i'd first fix the call order at Frame::createView(), turn the feature on first and set the size right after. Although, looking at transitionToCommittedForNewPage() (which calls createView()), it actually passes IntSize() as the size for the fixed layout which makes me wonder whether we need this change at all. I suspect that the actual size for the layout gets set somewhere later and this is just some leftover and should be removed."
I considered that a separate investigation. First I want to have WebKit2 actually working again, since it has been not working for several days now.
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Created an attachment (id=207835) [details] [details] [details]
> > > Patch
> >
> > I couldn't find any reply to this part of my comment, so I assume you missed it.
> >
> > "Fixing the name confusion is a great idea, but I still think that setUseFixedLayout() should just be a setter and setFixedLayoutSize() should trigger needslayout, where we actually _set_ the size of the canvas to layout the content on. So i'd first fix the call order at Frame::createView(), turn the feature on first and set the size right after. Although, looking at transitionToCommittedForNewPage() (which calls createView()), it actually passes IntSize() as the size for the fixed layout which makes me wonder whether we need this change at all. I suspect that the actual size for the layout gets set somewhere later and this is just some leftover and should be removed."
>
You can already change the order now. setFixedLayoutSize will also trigger needsLayout if useFixedLayout is true. I disagree that useFixedLayout shouldn't cause needsLayout since it obviously need to when fixed layout is disabled.
(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > (In reply to comment #22)
> > > > Created an attachment (id=207835) [details] [details] [details] [details]
> > > > Patch
> > >
> > > I couldn't find any reply to this part of my comment, so I assume you missed it.
> > >
> > > "Fixing the name confusion is a great idea, but I still think that setUseFixedLayout() should just be a setter and setFixedLayoutSize() should trigger needslayout, where we actually _set_ the size of the canvas to layout the content on. So i'd first fix the call order at Frame::createView(), turn the feature on first and set the size right after. Although, looking at transitionToCommittedForNewPage() (which calls createView()), it actually passes IntSize() as the size for the fixed layout which makes me wonder whether we need this change at all. I suspect that the actual size for the layout gets set somewhere later and this is just some leftover and should be removed."
> >
>
> You can already change the order now. setFixedLayoutSize will also trigger needsLayout if useFixedLayout is true. I disagree that useFixedLayout shouldn't cause needsLayout since it obviously need to when fixed layout is disabled.
The emphasis was more on that the 'setFixedLayoutSize' call seems unnecessary at this point and probably no need to issue needsLayout when the feature is turned on (only when the size is set). So this stacktrace looks invalid and fixing it seems to be the wrong thing to do.
(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #23)
> > > > (In reply to comment #22)
> > > > > Created an attachment (id=207835) [details] [details] [details] [details] [details]
> > > > > Patch
> > > >
> > > > I couldn't find any reply to this part of my comment, so I assume you missed it.
> > > >
> > > > "Fixing the name confusion is a great idea, but I still think that setUseFixedLayout() should just be a setter and setFixedLayoutSize() should trigger needslayout, where we actually _set_ the size of the canvas to layout the content on. So i'd first fix the call order at Frame::createView(), turn the feature on first and set the size right after. Although, looking at transitionToCommittedForNewPage() (which calls createView()), it actually passes IntSize() as the size for the fixed layout which makes me wonder whether we need this change at all. I suspect that the actual size for the layout gets set somewhere later and this is just some leftover and should be removed."
> > >
> >
> > You can already change the order now. setFixedLayoutSize will also trigger needsLayout if useFixedLayout is true. I disagree that useFixedLayout shouldn't cause needsLayout since it obviously need to when fixed layout is disabled.
> The emphasis was more on that the 'setFixedLayoutSize' call seems unnecessary at this point and probably no need to issue needsLayout when the feature is turned on (only when the size is set). So this stacktrace looks invalid and fixing it seems to be the wrong thing to do.
Well, needsLayout is not set when the size is set, because useFixedLayout is not yet set.
Created attachment 207856[details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Created attachment 207860[details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
Created attachment 208164[details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
Created attachment 208206[details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Created attachment 208351[details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Created attachment 208392[details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
2013-07-23 09:02 PDT, Allan Sandfeld Jensen
2013-07-29 06:00 PDT, Allan Sandfeld Jensen
2013-07-29 06:23 PDT, Allan Sandfeld Jensen
2013-07-29 07:22 PDT, Allan Sandfeld Jensen
2013-07-30 19:58 PDT, Build Bot
2013-07-31 00:55 PDT, Build Bot
2013-07-31 04:35 PDT, Allan Sandfeld Jensen
2013-07-31 10:22 PDT, Build Bot
2013-07-31 10:31 PDT, Build Bot
2013-08-05 04:57 PDT, Allan Sandfeld Jensen
2013-08-05 21:54 PDT, Build Bot
2013-08-06 11:11 PDT, Build Bot
2013-08-08 05:20 PDT, Allan Sandfeld Jensen
2013-08-08 09:24 PDT, Build Bot
2013-08-08 20:05 PDT, Build Bot
2013-08-09 01:14 PDT, Allan Sandfeld Jensen