Bug 23973 – Chromium crashes at times when the view is being closed.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ananta Iyengar]

We don't have a definite reproducible case for this crash. While the crash occurs in Chromium, the callstack suggests that this could in other webkit browsers as well. The callstack is as below:-

This occurs when the view is being closed and the scrollcontents method in the ScrollView object is called. It attempts to call the repaint method on the HostWindow object returned via FrameView::hostWindow. This method can return NULL in scenarios where the frame is NULL, or if the underlying page is destroyed. I think that a NULL check for the return value of the FrameView::hostWindow call is warranted. 

I will submit a patch for this in a bit.

Thread 1 *CRASHED* (EXCEPTION_ACCESS_VIOLATION @0x00000000)

0x62d3789e	 [chrome.dll	 - scrollview.cpp:438]	 WebCore::ScrollView::scrollContents(WebCore::IntSize const &)
0x62d37831	 [chrome.dll	 - scrollview.cpp:420]	 WebCore::ScrollView::updateScrollbars(WebCore::IntSize const &)
0x62d37009	 [chrome.dll	 - scrollview.cpp:116]	 WebCore::ScrollView::setScrollbarModes(WebCore::ScrollbarMode,WebCore::ScrollbarMode)
0x62d5d8e4	 [chrome.dll	 - frameview.cpp:231]	 WebCore::FrameView::resetScrollbars()
0x62d5d7f8	 [chrome.dll	 - frameview.cpp:197]	 WebCore::FrameView::~FrameView()
0x62d5d6d6	 [chrome.dll	 + 0x003ed6d6]	 WebCore::FrameView::`vector deleting destructor'(unsigned int)
0x62e309d8	 [chrome.dll	 - renderpart.cpp:56]	 WebCore::RenderPart::~RenderPart()
0x62e30998	 [chrome.dll	 + 0x004c0998]	 WebCore::RenderPart::`vector deleting destructor'(unsigned int)
0x62d6885f	 [chrome.dll	 - renderobject.cpp:2670]	 WebCore::RenderObject::arenaDelete(WebCore::RenderArena *,void *)
0x62dadd8b	 [chrome.dll	 - renderwidget.cpp:213]	 WebCore::RenderWidget::deref(WebCore::RenderArena *)
0x62dadabc	 [chrome.dll	 - renderwidget.cpp:103]	 WebCore::RenderWidget::destroy()
0x62d18d19	 [chrome.dll	 - node.cpp:1060]	 WebCore::Node::detach()
0x62d55b4c	 [chrome.dll	 - containernode.cpp:593]	 WebCore::ContainerNode::detach()
0x62d55b4c	 [chrome.dll	 - containernode.cpp:593]	 WebCore::ContainerNode::detach()
0x62d55b4c	 [chrome.dll	 - containernode.cpp:593]	 WebCore::ContainerNode::detach()
0x62d55b4c	 [chrome.dll	 - containernode.cpp:593]	 WebCore::ContainerNode::detach()
0x62cfd86b	 [chrome.dll	 - document.cpp:1412]	 WebCore::Document::detach()
0x62ce59d4	 [chrome.dll	 - frame.cpp:217]	 WebCore::Frame::setView(WebCore::FrameView *)
0x62cf23a2	 [chrome.dll	 - frameloader.cpp:3474]	 WebCore::FrameLoader::detachFromParent()
0x62cf22d8	 [chrome.dll	 - frameloader.cpp:3452]	 WebCore::FrameLoader::frameDetached()
0x62cb57a4	 [chrome.dll	 - webview_impl.cc:776]	 WebViewImpl::Close()
0x62984cb2	 [chrome.dll	 - render_widget.cc:639]	 RenderWidget::Close()
0x62984236	 [chrome.dll	 - render_widget.cc:225]	 RenderWidget::OnClose()
0x629803d7	 [chrome.dll	 - ipc_message.h:125]	 IPC::Message::Dispatch<RenderWidget>(IPC::Message const *,RenderWidget *,void ( RenderWidget::*)(void))
0x62984007	 [chrome.dll	 - render_widget.cc:157]	 RenderWidget::OnMessageReceived(IPC::Message const &)
0x62979fc3	 [chrome.dll	 - render_view.cc:400]	 RenderView::OnMessageReceived(IPC::Message const &)
0x62997262	 [chrome.dll	 - message_router.cc:39]	 MessageRouter::RouteMessage(IPC::Message const &)
0x6299723c	 [chrome.dll	 - message_router.cc:30]	 MessageRouter::OnMessageReceived(IPC::Message const &)
0x62976ba4	 [chrome.dll	 - render_thread.cc:174]	 RenderThread::OnMessageReceived(IPC::Message const &)
0x62ae6dab	 [chrome.dll	 - task.h:312]	 RunnableMethod<SafeBrowsingService,void ( SafeBrowsingService::*)(SafeBrowsingService::BlockingPageParam const &),Tuple1<SafeBrowsingService::BlockingPageParam> >::Run()
0x62b56ebc	 [chrome.dll	 - message_loop.cc:308]	 MessageLoop::RunTask(Task *)
0x62b56ef3	 [chrome.dll	 - message_loop.cc:316]	 MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &)
0x62b57081	 [chrome.dll	 - message_loop.cc:408]	 MessageLoop::DoWork()
0x62b6dccf	 [chrome.dll	 - message_pump_default.cc:50]	 base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
0x62b56d8d	 [chrome.dll	 - message_loop.cc:197]	 MessageLoop::RunInternal()
0x62b56d5a	 [chrome.dll	 - message_loop.cc:180]	 MessageLoop::RunHandler()
0x62b56cfd	 [chrome.dll	 - message_loop.cc:154]	 MessageLoop::Run()
0x62b5cfdf	 [chrome.dll	 - thread.cc:153]	 base::Thread::ThreadMain()
0x62b5a8a9	 [chrome.dll	 - platform_thread_win.cc:26]	 `anonymous namespace'::ThreadFunc(void *)
0x76fb4910	 [kernel32.dll	 + 0x00044910]	 BaseThreadInitThunk
0x7711e4b5	 [ntdll.dll	 + 0x0003e4b5]	 __RtlUserThreadStart
0x7711e488	 [ntdll.dll	 + 0x0003e488]	 _RtlUserThreadStart

[Ananta Iyengar]

Created attachment 27699 [details]
Initial attempt at a patch for this issue

[Jon@Chromium]

I need to get this reviewed so we can (hopefully) close the bug.

Also reported in http://code.google.com/p/chromium/issues/detail?id=6319

[Darin Fisher (:fishd, Google)]

Comment on attachment 27699 [details]
Initial attempt at a patch for this issue

>Index: WebCore/platform/ScrollView.cpp
...
> void ScrollView::scrollContents(const IntSize& scrollDelta)
> {
>+    if (!hostWindow())
>+      return;
>+

nit: the return statement should be indented by 4 spaces.  R- because
of this.

ScrollView appears to be designed with the assumption that the hostWindow
will never be null, but I can see from the callstack that assumption seems
to be invalid.  So, I think your solution is probably the best we can do.

Are any of the other ScrollView methods reachable from ~FrameView?

[Ananta Iyengar]

Created attachment 28024 [details]
Updated patch with the indentation fixed.

The functions in ScrollView.cpp reachable from ~FrameView are as below:-
1. ScrollView::setScrollbarModes which calls into updateScrollbars and ends up   
   in scrollcontents where hostwindow is dereferenced.

2. ScrollView::setHasHorizontalScrollbar and 
   ScrollView::setHasVerticalScrollbar.
   These functions internally call ScrollView::removeChild. They don't reference
   hostWindow().

[Darin Fisher (:fishd, Google)]

Comment on attachment 28024 [details]
Updated patch with the indentation fixed.

LGTM

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/41260