Bug 26326 – REGRESSION: When the main page (ScrollView) has a custom scrollbar, it crashes on destruction.

• Component: CSS
• Status: RESOLVED FIXED

[David Levin]

Original chromium bug: http://code.google.com/p/chromium/issues/detail?id=13693

This won't repro in Safari on OSX because it doesn't allow custom scrollbar for the main window, but it does repro in Safari on Windows (and in Google Chrome).

Repro steps:
1. Visit http://trac.webkit.org/export/41842/trunk/LayoutTests/scrollbars/overflow-scrollbar-combinations.html
2. Shrink the window so that a veritical scroll bar appears.
3. Refresh (or visit any other site, etc.)
crash

(Simple) fix and layout test coming shortly.

[David Levin]

In case anyone wants to compare this to crashes that they've seen (or search for crashes using function names), here's a stack for where the crash occurs:

[animationcontroller.cpp:449] - WebCore::AnimationController::cancelAnimations(WebCore::RenderObject *)
     [renderobject.cpp:1825] - WebCore::RenderObject::destroy()
          [renderbox.cpp:95] - WebCore::RenderBox::destroy()
   [renderscrollbar.cpp:219] - WebCore::RenderScrollbar::updateScrollbarPart(WebCore::ScrollbarPart,bool)
   [renderscrollbar.cpp:132] - WebCore::RenderScrollbar::updateScrollbarParts(bool)
    [renderscrollbar.cpp:54] - WebCore::RenderScrollbar::setParent(WebCore::ScrollView *)
         [scrollview.cpp:73] - WebCore::ScrollView::removeChild(WebCore::Widget *)
         [scrollview.cpp:96] - WebCore::ScrollView::setHasVerticalScrollbar(bool)
         [frameview.cpp:159] - WebCore::FrameView::~FrameView()
     [chrome.dll+0x0007f634] - WebCore::FrameView::`vector deleting destructor'(unsigned int)
              [refptr.h:122] - WTF::RefPtr<WebCore::FrameView>::operator=(WebCore::FrameView *)
             [frame.cpp:237] - WebCore::Frame::setView(WebCore::FrameView *)
     [webframe_impl.cc:1444] - WebFrameImpl::CreateFrameView()
      [frameloader.cpp:2943] - WebCore::FrameLoader::transitionToCommitted(WTF::PassRefPtr<WebCore::CachedPage>)
      [frameloader.cpp:2809] - WebCore::FrameLoader::commitProvisionalLoad(WTF::PassRefPtr<WebCore::CachedPage>)
    [documentloader.cpp:339] - WebCore::DocumentLoader::commitIfReady()

[David Levin]

A few more notes:

In safari, it appears to only repro on a refresh (not navigation) and it also repro's for iframes with custom scrollbars.

[David Levin]

Created attachment 31224 [details]
Here's a layout test to repro the crash.

[David Levin]

Created attachment 31225 [details]
Here's a patch to make the crash repro on OSX.

[David Levin]

Created attachment 31226 [details]
Failed attempt at fix.

"Failed attempt at fix" was my first attempt to fix the bug.

The problem is that even if I guard the call to animation() in RenderObject, it still calls

    arenaDelete(renderArena(), this);

and the renderArena (from document) has been deleted and is 0 at this point due to Document::detach() being called, so it feels like something worse is going on and perhaps more should be cleaned up in Document::detach().

At the moment, I could do some fix for this but not being familiar with the code I'm not sure what is most appropriate.

Next step: Ask dhyatt :)

[Adam Roben (:aroben)]

<rdar://problem/6972721>

[David Levin]

I just found out about a website from http://code.google.com/p/chromium/issues/detail?id=13630 that exhibits this crash:

  http://www.apple.com/iphone/iphone-3g-s/

On Safari on Windows, visit this website and refresh.

[David Levin]

Created attachment 31305 [details]
Proposed fix.

I have a few concerns about my patch, but I put it together as a place to start.

Here are my concerns:
1. I don't know if there is another function that I should call in FrameView.  (It doesn't look like there is one.)

2. I don't know if this function should do more.  For example, ~FrameView does resetScrollbars right before the calls to setHas*Scrollbar(false);  
Should it do even more of what ~FrameView does?

3. onDocumentDetaching doesn't seem like a great name.  detach() seemed to be typical but I wasn't sure what that function mean in general so I didn't know if this fit the pattern or not.

[Dave Hyatt]

You don't want to always destroy the scrollbars.  You only want to nuke them if the document supplied them.  If they were custom basically.

onDocumentDetaching could be renamed to detachCustomScrollbars.

[Dave Hyatt]

Oh, also if the scrollbars are supplied by the ownerElement's document rather than your own, then you don't need to destroy them either.

[Dave Hyatt]

Comment on attachment 31305 [details]
Proposed fix.

Minusing based on above comments.

[David Levin]

Created attachment 31397 [details]
Proposed fix.

Regarding this comment "Oh, also if the scrollbars are supplied by the ownerElement's document rather than your own, then you don't need to destroy them either"

I should have asked in irc, but I wasn't quite sure how that translated to code.  I did my best guess by adding the renderBox check.  Let me know if this isn't correct (and hopefully what it should be).

[Dave Hyatt]

Comment on attachment 31397 [details]
Proposed fix.

The RenderObject.cpp change seems unintentional.  Everything else is perfect.

r=me.

[David Levin]

As discussed in irc, the RenderObject.cpp change is described in the changelog and David Hyatt was fine with leaving that part of the change in.

Committed as http://trac.webkit.org/changeset/44751.

[David Levin]

Comment on attachment 31397 [details]
Proposed fix.

Clearing the r+ since this caused a layout test on windows to start crashing.

[David Levin]

Reverted with http://trac.webkit.org/changeset/44755 due to crashing layout test on windows.

[David Levin]

Created attachment 31566 [details]
Proposed fix.

I had to move the call to FrameView::detachCustomScrollbars in Document::detach to ensure that the renderBox was still set on the body element when this method was called.

I've tried this out on windows and it works fine.  (I thought I tested the last patch on windows but I must have missed something when doing the revisions.)

[Eric Seidel (no email)]

Comment on attachment 31566 [details]
Proposed fix.

Looks great!

[David Levin]

Committed as http://trac.webkit.org/changeset/44940