Bug 119677 – Page::mainFrame() should return a reference.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andreas Kling]

In practice, WebCore::Page always has a mainFrame(). There's a small window between Page construction and setMainFrame() where Page::m_mainFrame is null, but mainFrame() should never be called in that time.
We should make the method return a reference and remove that window, or at least guard it with an assertion.

[Andreas Kling]

Created attachment 208526 [details]
For EWS to make fun of

[Andreas Kling]

Created attachment 208527 [details]
For EWS to make fun of

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/bindings/js/PageScriptDebugServer.cpp', u'Source/WebCore/bindings/js/ScriptCachedFrameData.cpp', u'Source/WebCore/bindings/js/ScriptCachedFrameData.h', u'Source/WebCore/bindings/js/ScriptProfiler.cpp', u'Source/WebCore/bindings/js/ScriptState.cpp', u'Source/WebCore/css/MediaQueryEvaluator.cpp', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/history/CachedFrame.cpp', u'Source/WebCore/history/CachedFrame.h', u'Source/WebCore/history/CachedPage.cpp', u'Source/WebCore/history/PageCache.cpp', u'Source/WebCore/html/HTMLPlugInImageElement.cpp', u'Source/WebCore/html/ImageDocument.cpp', u'Source/WebCore/inspector/InspectorAgent.cpp', u'Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp', u'Source/WebCore/inspector/InspectorCanvasAgent.cpp', u'Source/WebCore/inspector/InspectorClient.cpp', u'Source/WebCore/inspector/InspectorController.cpp', u'Source/WebCore/inspector/InspectorDOMAgent.cpp', u'Source/WebCore/inspector/InspectorDebuggerAgent.cpp', u'Source/WebCore/inspector/InspectorFrontendClientLocal.cpp', u'Source/WebCore/inspector/InspectorFrontendHost.cpp', u'Source/WebCore/inspector/InspectorInputAgent.cpp', u'Source/WebCore/inspector/InspectorInstrumentation.cpp', u'Source/WebCore/inspector/InspectorOverlay.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/inspector/InspectorPageAgent.h', u'Source/WebCore/inspector/InspectorResourceAgent.cpp', u'Source/WebCore/inspector/InspectorTimelineAgent.cpp', u'Source/WebCore/inspector/PageDebuggerAgent.cpp', u'Source/WebCore/inspector/PageRuntimeAgent.cpp', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/loader/HistoryController.cpp', u'Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp', u'Source/WebCore/page/Chrome.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/ContextMenuController.h', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/FrameTree.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/Page.cpp', u'Source/WebCore/page/Page.h', u'Source/WebCore/page/PageGroup.cpp', u'Source/WebCore/page/PageGroupLoadDeferrer.cpp', u'Source/WebCore/page/PageSerializer.cpp', u'Source/WebCore/page/PageThrottler.cpp', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/SpatialNavigation.cpp', u'Source/WebCore/page/mac/DragControllerMac.mm', u'Source/WebCore/page/scrolling/ScrollingCoordinator.cpp', u'Source/WebCore/page/scrolling/blackberry/ScrollingCoordinatorBlackBerry.cpp', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm', u'Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp', u'Source/WebCore/platform/network/blackberry/SocketStreamHandleBlackBerry.cpp', u'Source/WebCore/plugins/DOMMimeType.cpp', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/storage/StorageEventDispatcher.cpp', u'Source/WebCore/svg/graphics/SVGImage.cpp', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp', u'Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp', u'Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp', u'Source/WebKit/gtk/webkit/webkitviewportattributes.cpp', u'Source/WebKit/gtk/webkit/webkitwebview.cpp', u'Source/WebKit/mac/WebCoreSupport/WebDragClient.mm', u'Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm', u'Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.mm', u'Source/WebKit/mac/WebView/WebFrameView.mm', u'Source/WebKit/mac/WebView/WebHTMLView.mm', u'Source/WebKit/mac/WebView/WebView.mm', u'Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp', u'Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp', u'Source/WebKit/win/WebCoreSupport/WebContextMenuClient.cpp', u'Source/WebKit/win/WebCoreSupport/WebDragClient.cpp', u'Source/WebKit/win/WebDropSource.cpp', u'Source/WebKit/win/WebView.cpp', u'Source/WebKit/wince/WebView.cpp', u'Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginView.cpp', u'Source/WebKit2/WebProcess/Storage/StorageAreaMap.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebContextMenuClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/mac/WebContextMenuClientMac.mm', u'Source/WebKit2/WebProcess/WebPage/FindController.cpp', u'Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp', u'Source/WebKit2/WebProcess/WebPage/WebInspector.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp', u'Source/WebKit2/WebProcess/WebPage/mac/LayerTreeHostMac.mm', u'Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeContext.mm', u'Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm', u'Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm']" exit_code: 1
Source/WebKit/win/WebCoreSupport/WebDragClient.cpp:93:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 107 files


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

[Early Warning System Bot]

Comment on attachment 208527 [details]
For EWS to make fun of

Attachment 208527 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1447226

[EFL EWS Bot]

Comment on attachment 208527 [details]
For EWS to make fun of

Attachment 208527 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1451069

[Andreas Kling]

Created attachment 208530 [details]
EWS mockery round 2

[Andreas Kling]

Created attachment 208531 [details]
EWS mockery round 2

[Early Warning System Bot]

Comment on attachment 208531 [details]
EWS mockery round 2

Attachment 208531 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1451074

[Andreas Kling]

Created attachment 208533 [details]
EWS funtime party 3

[Andreas Kling]

Created attachment 208535 [details]
Patch

[Antti Koivisto]

Comment on attachment 208535 [details]
Patch

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

rs=me

> Source/WebCore/ChangeLog:11
> +        In practice, Page always has a mainFrame(). There's a short window in time
> +        between construction and Frame::create() calling Page::setMainFrame() where
> +        m_page is null, but mainFrame() should not be used then. We guard against
> +        this with an assertion in mainFrame().

Would be nice to cleanup this awkward construction pattern.

[kov's GTK+ EWS bot]

Comment on attachment 208535 [details]
Patch

Attachment 208535 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1456041

[Andreas Kling]

Created attachment 209699 [details]
Patch for landing (if EWS likes)

Finally updated this now that Page creates the mainFrame().

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Source/WebCore/bindings/js/PageScriptDebugServer.cpp', u'Source/WebCore/bindings/js/ScriptProfiler.cpp', u'Source/WebCore/bindings/js/ScriptState.cpp', u'Source/WebCore/css/MediaQueryEvaluator.cpp', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/history/CachedFrame.cpp', u'Source/WebCore/history/CachedPage.cpp', u'Source/WebCore/history/PageCache.cpp', u'Source/WebCore/html/HTMLPlugInImageElement.cpp', u'Source/WebCore/html/ImageDocument.cpp', u'Source/WebCore/inspector/InspectorAgent.cpp', u'Source/WebCore/inspector/InspectorCanvasAgent.cpp', u'Source/WebCore/inspector/InspectorClient.cpp', u'Source/WebCore/inspector/InspectorController.cpp', u'Source/WebCore/inspector/InspectorDOMAgent.cpp', u'Source/WebCore/inspector/InspectorFrontendClientLocal.cpp', u'Source/WebCore/inspector/InspectorFrontendHost.cpp', u'Source/WebCore/inspector/InspectorInputAgent.cpp', u'Source/WebCore/inspector/InspectorInstrumentation.cpp', u'Source/WebCore/inspector/InspectorOverlay.cpp', u'Source/WebCore/inspector/InspectorPageAgent.cpp', u'Source/WebCore/inspector/PageRuntimeAgent.cpp', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/loader/HistoryController.cpp', u'Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp', u'Source/WebCore/page/Chrome.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/FrameTree.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/Page.cpp', u'Source/WebCore/page/Page.h', u'Source/WebCore/page/PageGroup.cpp', u'Source/WebCore/page/PageGroupLoadDeferrer.cpp', u'Source/WebCore/page/PageSerializer.cpp', u'Source/WebCore/page/PageThrottler.cpp', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/SpatialNavigation.cpp', u'Source/WebCore/page/mac/DragControllerMac.mm', u'Source/WebCore/page/scrolling/ScrollingCoordinator.cpp', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm', u'Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp', u'Source/WebCore/plugins/DOMMimeType.cpp', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/TextAutosizer.cpp', u'Source/WebCore/storage/StorageEventDispatcher.cpp', u'Source/WebCore/svg/graphics/SVGImage.cpp', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp', u'Source/WebKit/efl/ewk/ewk_view.cpp', u'Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp', u'Source/WebKit/gtk/webkit/webkitviewportattributes.cpp', u'Source/WebKit/gtk/webkit/webkitwebframe.cpp', u'Source/WebKit/gtk/webkit/webkitwebview.cpp', u'Source/WebKit/mac/WebCoreSupport/WebDragClient.mm', u'Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm', u'Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.mm', u'Source/WebKit/mac/WebView/WebFrame.mm', u'Source/WebKit/mac/WebView/WebFrameView.mm', u'Source/WebKit/mac/WebView/WebHTMLView.mm', u'Source/WebKit/mac/WebView/WebView.mm', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp', u'Source/WebKit/win/WebView.cpp', u'Source/WebKit/wince/WebView.cpp', u'Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp', u'Source/WebKit2/WebProcess/Plugins/PluginView.cpp', u'Source/WebKit2/WebProcess/Storage/StorageAreaMap.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebContextMenuClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/mac/WebContextMenuClientMac.mm', u'Source/WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm', u'Source/WebKit2/WebProcess/WebPage/FindController.cpp', u'Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp', u'Source/WebKit2/WebProcess/WebPage/WebInspector.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp', u'Source/WebKit2/WebProcess/WebPage/mac/LayerTreeHostMac.mm', u'Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeContext.mm', u'Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm', u'Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm']" exit_code: 1
Source/WebKit/gtk/webkit/webkitwebview.cpp:4393:  Declaration has space between type name and * in Frame *frame  [whitespace/declaration] [3]
Total errors found: 1 in 98 files


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

[Early Warning System Bot]

Comment on attachment 209699 [details]
Patch for landing (if EWS likes)

Attachment 209699 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1583089

[Early Warning System Bot]

Comment on attachment 209699 [details]
Patch for landing (if EWS likes)

Attachment 209699 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1592076

[EFL EWS Bot]

Comment on attachment 209699 [details]
Patch for landing (if EWS likes)

Attachment 209699 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1588094

[kov's GTK+ EWS bot]

Comment on attachment 209699 [details]
Patch for landing (if EWS likes)

Attachment 209699 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1580112

[EFL EWS Bot]

Comment on attachment 209699 [details]
Patch for landing (if EWS likes)

Attachment 209699 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1578118

[Andreas Kling]

Created attachment 209702 [details]
Patch for landing (come on, EWS, let's be friends)

[Early Warning System Bot]

Comment on attachment 209702 [details]
Patch for landing (come on, EWS, let's be friends)

Attachment 209702 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1592081

[Early Warning System Bot]

Comment on attachment 209702 [details]
Patch for landing (come on, EWS, let's be friends)

Attachment 209702 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1584107

[EFL EWS Bot]

Comment on attachment 209702 [details]
Patch for landing (come on, EWS, let's be friends)

Attachment 209702 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1586098

[EFL EWS Bot]

Comment on attachment 209702 [details]
Patch for landing (come on, EWS, let's be friends)

Attachment 209702 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1601002

[Andreas Kling]

Created attachment 209704 [details]
Patch for landing (ews pls)

[Early Warning System Bot]

Comment on attachment 209704 [details]
Patch for landing (ews pls)

Attachment 209704 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1586102

[Early Warning System Bot]

Comment on attachment 209704 [details]
Patch for landing (ews pls)

Attachment 209704 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1589098

[Andreas Kling]

Created attachment 209705 [details]
Patch for landing (totally...)

[Early Warning System Bot]

Comment on attachment 209705 [details]
Patch for landing (totally...)

Attachment 209705 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1585114

[Early Warning System Bot]

Comment on attachment 209705 [details]
Patch for landing (totally...)

Attachment 209705 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1585117

[Andreas Kling]

Created attachment 209707 [details]
Patch for landing (!)

[Andreas Kling]

Committed r154658: <http://trac.webkit.org/changeset/154658>

[Ryosuke Niwa]

Windows build fixed in http://trac.webkit.org/changeset/154659 and http://trac.webkit.org/changeset/154659