Bug 26326 - REGRESSION: When the main page (ScrollView) has a custom scrollbar, it crashes on destruction.
Summary: REGRESSION: When the main page (ScrollView) has a custom scrollbar, it crashe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Windows Vista
: P1 Normal
Assignee: David Levin
URL: http://trac.webkit.org/export/41842/t...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-06-11 11:05 PDT by David Levin
Modified: 2009-06-22 10:20 PDT (History)
5 users (show)

See Also:


Attachments
Here's a layout test to repro the crash. (1.81 KB, patch)
2009-06-12 18:00 PDT, David Levin
no flags Details | Formatted Diff | Diff
Here's a patch to make the crash repro on OSX. (1.01 KB, patch)
2009-06-12 18:00 PDT, David Levin
no flags Details | Formatted Diff | Diff
Failed attempt at fix. (1.90 KB, patch)
2009-06-12 18:11 PDT, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (5.68 KB, patch)
2009-06-15 14:07 PDT, David Levin
hyatt: review-
Details | Formatted Diff | Diff
Proposed fix. (8.19 KB, patch)
2009-06-16 21:31 PDT, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (8.27 KB, patch)
2009-06-19 15:29 PDT, David Levin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-06-11 11:05:43 PDT
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.
Comment 1 David Levin 2009-06-11 11:37:04 PDT
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()
Comment 2 David Levin 2009-06-11 17:21:49 PDT
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.

Comment 3 David Levin 2009-06-12 18:00:08 PDT
Created attachment 31224 [details]
Here's a layout test to repro the crash.
Comment 4 David Levin 2009-06-12 18:00:58 PDT
Created attachment 31225 [details]
Here's a patch to make the crash repro on OSX.
Comment 5 David Levin 2009-06-12 18:11:05 PDT
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 :)
Comment 6 Adam Roben (:aroben) 2009-06-15 10:48:40 PDT
<rdar://problem/6972721>
Comment 7 David Levin 2009-06-15 11:58:14 PDT
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.
Comment 8 David Levin 2009-06-15 14:07:12 PDT
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.
Comment 9 Dave Hyatt 2009-06-15 14:16:12 PDT
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.
Comment 10 Dave Hyatt 2009-06-15 14:16:42 PDT
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.
Comment 11 Dave Hyatt 2009-06-15 14:17:08 PDT
Comment on attachment 31305 [details]
Proposed fix.

Minusing based on above comments.
Comment 12 David Levin 2009-06-16 21:31:23 PDT
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).
Comment 13 Dave Hyatt 2009-06-16 21:44:34 PDT
Comment on attachment 31397 [details]
Proposed fix.

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

r=me.
Comment 14 David Levin 2009-06-16 21:59:26 PDT
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.
Comment 15 David Levin 2009-06-17 00:43:28 PDT
Comment on attachment 31397 [details]
Proposed fix.

Clearing the r+ since this caused a layout test on windows to start crashing.
Comment 16 David Levin 2009-06-17 00:43:58 PDT
Reverted with http://trac.webkit.org/changeset/44755 due to crashing layout test on windows.
Comment 17 David Levin 2009-06-19 15:29:44 PDT
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.)
Comment 18 Eric Seidel (no email) 2009-06-21 23:33:59 PDT
Comment on attachment 31566 [details]
Proposed fix.

Looks great!
Comment 19 David Levin 2009-06-22 10:20:56 PDT
Committed as http://trac.webkit.org/changeset/44940