Bug 60618 - WebView::performLayeredWindowUpdate() crashes with NULL pointer
Summary: WebView::performLayeredWindowUpdate() crashes with NULL pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-11 03:26 PDT by Heiner Wolf
Modified: 2011-08-08 12:41 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.24 KB, patch)
2011-08-05 15:50 PDT, Brent Fulgham
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Heiner Wolf 2011-05-11 03:26:14 PDT
m_backingStoreBitmap can be NULL causing ->handle() to fail. This happens when I resize (quickly?) to (0,0). I am using a sizer-div and move it quickly to the topleft corner of the window and beyond. The sizer-div calls ::MoveWindow(). The crash does not always happen. Maybe it's a timing issue. If I resize slowly, then it does not crash. This lets me assume, that it only happens, when there comes a second ::MoveWindow(width=0,height=0) after sizing to (0,0). Maybe m_backingStoreBitmap is discarded for size (0,0). WebView::performLayeredWindowUpdate() should probably have a if(m_backingStoreBitmap!=NULL), anyway.

http://svn.webkit.org/repository/webkit/trunk
Revision: 85004

Method:
  void WebView::performLayeredWindowUpdate()
crashes when accessing
  m_backingStoreBitmap->handle()
with 
  m_backingStoreBitmap = 0


Stack trace:
>	WebKit_debug.dll!WebCore::RefCountedGDIHandle<HBITMAP__ *>::handle()  Line 51 + 0x3 bytes	C++
 	WebKit_debug.dll!WebView::performLayeredWindowUpdate()  Line 1001 + 0x12 bytes	C++
 	WebKit_debug.dll!WebView::WebViewWndProc(HWND__ * hWnd=0x000b0b78, unsigned int message=15, unsigned int wParam=0, long lParam=0)  Line 2130	C++
 	user32.dll!76bcc4e7() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	
 	user32.dll!76bc5f9f() 	
 	user32.dll!76bcc590() 	
 	user32.dll!76bc4f0e() 	
 	user32.dll!76bc4f7d() 	
 	ntdll.dll!77816fee() 	
 	user32.dll!76bc4ec3() 	
 	user32.dll!76bc1be4() 	
 	user32.dll!76bbffcd() 	
 	WebKit_debug.dll!WebView::repaint(const WebCore::IntRect & windowRect={...}, bool contentChanged=false, bool immediate=true, bool repaintContentOnly=false)  Line 756 + 0xf bytes	C++
 	WebKit_debug.dll!WebChromeClient::invalidateWindow(const WebCore::IntRect & windowRect={...}, bool immediate=true)  Line 470	C++
 	WebKit_debug.dll!WebCore::Chrome::invalidateWindow(const WebCore::IntRect & updateRect={...}, bool immediate=true)  Line 71 + 0x20 bytes	C++
 	WebKit_debug.dll!WebCore::ScrollView::scrollContents(const WebCore::IntSize & scrollDelta={...})  Line 663 + 0x3f bytes	C++
 	WebKit_debug.dll!WebCore::ScrollView::scrollTo(const WebCore::IntSize & newOffset={...})  Line 366	C++
 	WebKit_debug.dll!WebCore::FrameView::scrollTo(const WebCore::IntSize & newOffset={...})  Line 2109	C++
 	WebKit_debug.dll!WebCore::ScrollView::setScrollOffset(const WebCore::IntPoint & offset={...})  Line 351 + 0x17 bytes	C++
 	WebKit_debug.dll!WebCore::ScrollableArea::setScrollOffsetFromAnimation(const WebCore::IntPoint & offset={...})  Line 133 + 0x13 bytes	C++
 	WebKit_debug.dll!WebCore::ScrollAnimator::notityPositionChanged()  Line 130	C++
 	WebKit_debug.dll!WebCore::ScrollAnimator::scrollToOffsetWithoutAnimation(const WebCore::FloatPoint & offset={...})  Line 80 + 0xf bytes	C++
 	WebKit_debug.dll!WebCore::ScrollableArea::scrollToOffsetWithoutAnimation(const WebCore::FloatPoint & offset={...})  Line 97 + 0x21 bytes	C++
 	WebKit_debug.dll!WebCore::ScrollView::updateScrollbars(const WebCore::IntSize & desiredOffset={...})  Line 592	C++
 	WebKit_debug.dll!WebCore::ScrollView::setScrollPosition(const WebCore::IntPoint & scrollPoint={...})  Line 402	C++
 	WebKit_debug.dll!WebCore::FrameView::setScrollPosition(const WebCore::IntPoint & scrollPoint={...})  Line 1457	C++
 	WebKit_debug.dll!WebCore::RenderLayer::scrollRectToVisible(const WebCore::IntRect & rect={...}, bool scrollToAnchor=false, const WebCore::ScrollAlignment & alignX={...}, const WebCore::ScrollAlignment & alignY={...})  Line 1482	C++
 	WebKit_debug.dll!WebCore::RenderLayer::autoscroll()  Line 1586	C++
 	WebKit_debug.dll!WebCore::RenderBox::autoscroll()  Line 660	C++
 	WebKit_debug.dll!WebCore::EventHandler::autoscrollTimerFired(WebCore::Timer<WebCore::EventHandler> * __formal=0x032a42b0)  Line 798 + 0x21 bytes	C++
 	WebKit_debug.dll!WebCore::Timer<WebCore::EventHandler>::fired()  Line 100 + 0x23 bytes	C++
 	WebKit_debug.dll!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 112 + 0xf bytes	C++
 	WebKit_debug.dll!WebCore::ThreadTimers::sharedTimerFired()  Line 91	C++
 	WebKit_debug.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd=0x00040a4c, unsigned int message=49797, unsigned int wParam=0, long lParam=0)  Line 103 + 0x8 bytes	C++
Comment 1 Adam Roben (:aroben) 2011-05-11 10:21:24 PDT
Heiner, is this happening in an application that is using layered window mode? If so, which application?
Comment 2 Brent Fulgham 2011-05-11 23:26:05 PDT
Hi Adam,

(In reply to comment #1)
> Heiner, is this happening in an application that is using layered window mode? If so, which application?

I asked Heiner to file this bug.  It's happening in an application he's working on, and I wanted to open an issue to track a correction for this.
Comment 3 Adam Roben (:aroben) 2011-05-12 06:43:55 PDT
Sounds good. I just wanted to make sure we weren't going down the layered window path by accident.
Comment 4 Heiner Wolf 2011-05-12 06:55:11 PDT
I am working on my own application using WebKit.dll, etc. It is based on Brent's recent transparency patch. The transparency code is very new, has not seen may users, so it's no surprise, that some edge cases come up.
Comment 5 Brent Fulgham 2011-08-05 15:50:45 PDT
Created attachment 103124 [details]
Patch
Comment 6 Adam Roben (:aroben) 2011-08-08 08:38:46 PDT
Comment on attachment 103124 [details]
Patch

When do we expect this case to get hit? It would be worth mentioning it in the ChangeLog and/or a comment.
Comment 7 Brent Fulgham 2011-08-08 11:06:13 PDT
(In reply to comment #6)
> (From update of attachment 103124 [details])
> When do we expect this case to get hit? It would be worth mentioning it in the ChangeLog and/or a comment.

The 'ensureBackingStore' method will not produce a backing store in cases where the window rect is zero height or zero width.  I think Heiner ran into this during certain resize operations; it may be that a paint message is getting processed after the window rect has collapsed to zero height (or width).

Based on his earlier comments (2011-05-11 03:26:14), it seems to trigger when the window is sized to (0,0) (which we know destroys the backing store), followed by a MoveWindow, which must be generating a paint.

It might be that this could be addressed by avoiding the second paint message getting to the 'performLayeredWindowUpdate', but it seemed easiest to just check for NULL backing store and bail out early if necessary.

Do you think deeper analysis of the drawing logic is warranted here?
Comment 8 Adam Roben (:aroben) 2011-08-08 11:08:39 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 103124 [details] [details])
> > When do we expect this case to get hit? It would be worth mentioning it in the ChangeLog and/or a comment.
> 
> The 'ensureBackingStore' method will not produce a backing store in cases where the window rect is zero height or zero width.  I think Heiner ran into this during certain resize operations; it may be that a paint message is getting processed after the window rect has collapsed to zero height (or width).
> 
> Based on his earlier comments (2011-05-11 03:26:14), it seems to trigger when the window is sized to (0,0) (which we know destroys the backing store), followed by a MoveWindow, which must be generating a paint.

Makes sense.

> It might be that this could be addressed by avoiding the second paint message getting to the 'performLayeredWindowUpdate', but it seemed easiest to just check for NULL backing store and bail out early if necessary.
> 
> Do you think deeper analysis of the drawing logic is warranted here?

I suggest we do as little work as possible when the window is 0-sized. The null-check seems fine.
Comment 9 Brent Fulgham 2011-08-08 12:41:39 PDT
Committed r92621: <http://trac.webkit.org/changeset/92621>