Skip to content

Commit

Permalink
Merge r182374 - FrameView code uses page() without null checking
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=143425
rdar://problem/18920601

Reviewed by Anders Carlsson.

While we don't have tests that cover this, we are seeing crashes coming in
that indicate the shouldEnableSpeculativeTilingDuringLoading function is
being called when the page is null. This patch adds null checks to all the
places in FrameView that use page() without doing null checking.

* page/FrameView.cpp:
(WebCore::FrameView::layout): If page is null, don't try to do the
auto-sizing logic that involves the textAutosizingWidth value from the page.
(WebCore::FrameView::setFixedVisibleContentRect): Get settings from the
frame rather than the page to avoid possible null-dereference.
(WebCore::FrameView::scrollPositionChanged): Check the page for null when
getting the event throttling delay.
(WebCore::FrameView::updateLayerFlushThrottling): Check the page for null,
and return early if it is null.
(WebCore::shouldEnableSpeculativeTilingDuringLoading): Check the page for
null, and return false if it is null.
(WebCore::FrameView::performPostLayoutTasks): Guard the code that calls
didLayout on the page client by a check if the page is null.
(WebCore::FrameView::pagination): Don't call Page::pagination on a null
page here.
(WebCore::FrameView::visibleContentScaleFactor): Use a scale factor of 1
if the page is null.
(WebCore::FrameView::setVisibleScrollerThumbRect): Don't call through to
the page client if the page is null.
(WebCore::FrameView::scrollbarStyleChanged): Ditto.
(WebCore::FrameView::setScrollPinningBehavior): Check the page for null
before asking it for the scrolling coordinator.
  • Loading branch information
@darinadler @carlosgcampos
darinadler authored and carlosgcampos committed Apr 13, 2015
1 parent 59def07 commit 19325f7
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 22 deletions.
36 changes: 36 additions & 0 deletions Source/WebCore/ChangeLog
View file
@@ -1,3 +1,39 @@
2015-04-05 Darin Adler <darin@apple.com>

FrameView code uses page() without null checking
https://bugs.webkit.org/show_bug.cgi?id=143425
rdar://problem/18920601

Reviewed by Anders Carlsson.

While we don't have tests that cover this, we are seeing crashes coming in
that indicate the shouldEnableSpeculativeTilingDuringLoading function is
being called when the page is null. This patch adds null checks to all the
places in FrameView that use page() without doing null checking.

* page/FrameView.cpp:
(WebCore::FrameView::layout): If page is null, don't try to do the
auto-sizing logic that involves the textAutosizingWidth value from the page.
(WebCore::FrameView::setFixedVisibleContentRect): Get settings from the
frame rather than the page to avoid possible null-dereference.
(WebCore::FrameView::scrollPositionChanged): Check the page for null when
getting the event throttling delay.
(WebCore::FrameView::updateLayerFlushThrottling): Check the page for null,
and return early if it is null.
(WebCore::shouldEnableSpeculativeTilingDuringLoading): Check the page for
null, and return false if it is null.
(WebCore::FrameView::performPostLayoutTasks): Guard the code that calls
didLayout on the page client by a check if the page is null.
(WebCore::FrameView::pagination): Don't call Page::pagination on a null
page here.
(WebCore::FrameView::visibleContentScaleFactor): Use a scale factor of 1
if the page is null.
(WebCore::FrameView::setVisibleScrollerThumbRect): Don't call through to
the page client if the page is null.
(WebCore::FrameView::scrollbarStyleChanged): Ditto.
(WebCore::FrameView::setScrollPinningBehavior): Check the page for null
before asking it for the scrolling coordinator.

2015-04-01 Zalan Bujtas <zalan@apple.com>

Lots of time spent querying table cell borders, when there are none.
Expand Down
65 changes: 43 additions & 22 deletions Source/WebCore/page/FrameView.cpp
View file
Expand Up @@ -1316,13 +1316,14 @@ void FrameView::layout(bool allowSubtree)

root->layout();
#if ENABLE(IOS_TEXT_AUTOSIZING)
float minZoomFontSize = frame().settings().minimumZoomFontSize();
float visWidth = frame().page()->textAutosizingWidth();
if (minZoomFontSize && visWidth && !root->view().printing()) {
root->adjustComputedFontSizesOnBlocks(minZoomFontSize, visWidth);
bool needsLayout = root->needsLayout();
if (needsLayout)
root->layout();
if (Page* page = frame().page()) {
float minimumZoomFontSize = frame().settings().minimumZoomFontSize();
float textAutosizingWidth = page->textAutosizingWidth();
if (minimumZoomFontSize && textAutosizingWidth && !root->view().printing()) {
root->adjustComputedFontSizesOnBlocks(minimumZoomFontSize, textAutosizingWidth);
if (root->needsLayout())
root->layout();
}
}
#endif
#if ENABLE(TEXT_AUTOSIZING)
Expand Down Expand Up @@ -2078,7 +2079,7 @@ void FrameView::setFixedVisibleContentRect(const IntRect& visibleContentRect)
ScrollView::setFixedVisibleContentRect(visibleContentRect);
if (offset != scrollOffset()) {
updateLayerPositionsAfterScrolling();
if (frame().page()->settings().acceleratedCompositingForFixedPositionEnabled())
if (frame().settings().acceleratedCompositingForFixedPositionEnabled())
updateCompositingLayersAfterScrolling();
IntPoint newPosition = scrollPosition();
scrollAnimator()->setCurrentPosition(scrollPosition());
Expand Down Expand Up @@ -2117,7 +2118,8 @@ void FrameView::scrollPositionChangedViaPlatformWidgetImpl(const IntPoint& oldPo

void FrameView::scrollPositionChanged(const IntPoint& oldPosition, const IntPoint& newPosition)
{
std::chrono::milliseconds throttlingDelay = frame().page()->chrome().client().eventThrottlingDelay();
Page* page = frame().page();
auto throttlingDelay = page ? page->chrome().client().eventThrottlingDelay() : std::chrono::milliseconds::zero();

if (throttlingDelay == std::chrono::milliseconds::zero()) {
m_delayedScrollEventTimer.stop();
Expand Down Expand Up @@ -2385,13 +2387,16 @@ void FrameView::loadProgressingStatusChanged()

void FrameView::updateLayerFlushThrottling()
{
Page* page = frame().page();
if (!page)
return;

ASSERT(frame().isMainFrame());
auto& page = *frame().page();

LayerFlushThrottleState::Flags flags = determineLayerFlushThrottleState(page);
LayerFlushThrottleState::Flags flags = determineLayerFlushThrottleState(*page);

// See if the client is handling throttling.
if (page.chrome().client().adjustLayerFlushThrottling(flags))
if (page->chrome().client().adjustLayerFlushThrottling(flags))
return;

for (Frame* frame = m_frame.get(); frame; frame = frame->tree().traverseNext(m_frame.get())) {
Expand All @@ -2416,7 +2421,8 @@ void FrameView::adjustTiledBackingCoverage()

static bool shouldEnableSpeculativeTilingDuringLoading(const FrameView& view)
{
return view.isVisuallyNonEmpty() && !view.frame().page()->progress().isMainLoadProgressing();
Page* page = view.frame().page();
return page && view.isVisuallyNonEmpty() && !page->progress().isMainLoadProgressing();
}

void FrameView::enableSpeculativeTilingIfNeeded()
Expand Down Expand Up @@ -2950,8 +2956,10 @@ void FrameView::performPostLayoutTasks()
// Only send layout-related delegate callbacks synchronously for the main frame to
// avoid re-entering layout for the main frame while delivering a layout-related delegate
// callback for a subframe.
if (frame().isMainFrame())
frame().page()->chrome().client().didLayout();
if (frame().isMainFrame()) {
if (Page* page = frame().page())
page->chrome().client().didLayout();
}
#endif

#if ENABLE(FONT_LOAD_EVENTS)
Expand Down Expand Up @@ -3236,8 +3244,10 @@ const Pagination& FrameView::pagination() const
if (m_pagination != Pagination())
return m_pagination;

if (frame().isMainFrame())
return frame().page()->pagination();
if (frame().isMainFrame()) {
if (Page* page = frame().page())
return page->pagination();
}

return m_pagination;
}
Expand Down Expand Up @@ -3385,15 +3395,23 @@ float FrameView::visibleContentScaleFactor() const
if (!frame().isMainFrame() || !frame().settings().delegatesPageScaling())
return 1;

return frame().page()->pageScaleFactor();
Page* page = frame().page();
if (!page)
return 1;

return page->pageScaleFactor();
}

void FrameView::setVisibleScrollerThumbRect(const IntRect& scrollerThumb)
{
if (!frame().isMainFrame())
return;

frame().page()->chrome().client().notifyScrollerThumbIsVisibleInRect(scrollerThumb);
Page* page = frame().page();
if (!page)
return;

page->chrome().client().notifyScrollerThumbIsVisibleInRect(scrollerThumb);
}

ScrollableArea* FrameView::enclosingScrollableArea() const
Expand Down Expand Up @@ -3491,7 +3509,8 @@ void FrameView::scrollbarStyleChanged(int newStyle, bool forceUpdate)
if (!frame().isMainFrame())
return;

frame().page()->chrome().client().recommendedScrollbarStyleDidChange(newStyle);
if (Page* page = frame().page())
page->chrome().client().recommendedScrollbarStyleDidChange(newStyle);

if (forceUpdate)
ScrollView::scrollbarStyleChanged(newStyle, forceUpdate);
Expand Down Expand Up @@ -4642,8 +4661,10 @@ void FrameView::setScrollPinningBehavior(ScrollPinningBehavior pinning)
{
m_scrollPinningBehavior = pinning;

if (ScrollingCoordinator* scrollingCoordinator = frame().page()->scrollingCoordinator())
scrollingCoordinator->setScrollPinningBehavior(pinning);
if (Page* page = frame().page()) {
if (auto* scrollingCoordinator = page->scrollingCoordinator())
scrollingCoordinator->setScrollPinningBehavior(pinning);
}

updateScrollbars(scrollOffset());
}
Expand Down

0 comments on commit 19325f7

Please sign in to comment.