299384 - Crash when using the scrollbar while page is (re)loading

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Uri Bernstein]

This is probably related to bug 298677, but I'm reporting it as a separate bug
since it is a crash.

Steps to reproduce:
1. Go to a a page which is long enough to display a vertical scrollbar (such as
the default DeerPark homepage).
2. Hit the "reload" button
3. Quickly (before the page reloads) use the mouse to grab the scrollbar and
move it up and down. Keep doing this for about 5 seconds.
4. You will usually see the effect of bug 298677, followed by a crash.

I'll attach a crash log immediately

Comment 1

[Uri Bernstein]

Attachment: Apple crash log

Comment 2

[Uri Bernstein]

Asking for blocking 1.8b3 (is it too late?)
Also, I'm on OS X 10.4.1, and I'm seeing the bug at least since the 2005-06-27
build.

Comment 3

[Ria Klaassen (not reading all bugmail)]

This does not affect WinXP; WFM. 
As long as Firefox is waiting I can grab the scrollbar and move it, but when the
data transfer begins the cursor releases the scrollbar. No crash.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050701
Firefox/1.0+ ID:2005070107

Comment 4

[Uri Bernstein]

Unsurprisingly, this regressed between 2005-06-21 and 2005-06-22.

Comment 5

[Simon Fraser [no longer active]]

Attachment: Patch to fix crash

This patch adds a check on mParent in the scrollbar action proc, bailing if the
parent is null (which happens during teardown on pageload). It's just a crash
fix: it doesn't break you out of the tracking loop (e.g. by posting a fake
mouse up event).

Comment 6

[Mark Mentovai]

Attachment: With PostEvent

I came up with two proposed solutions to this bug.  One looks just like Simon's
right down to the placement and newlines, but with a PostEvent(mouseUp,0). 
That's what this patch is.  The other implements a Destroy method that sets a
new mGoingAway variable, and tests mGoingAway instead of parent.  These both
work pretty well, but I'm still able to crash about 2% of the time.

The crashes aren't related to the event being slipped in.  Doing it with the
mouse up event is much nicer, because it allows the disconnected scrollbar to
be cleaned up even if the user has fallen asleep and is still trying to
manipulate it.

The very infrequent crashes still need attention.  This is better than nothing.
 Something, sometimes, is getting invalidated before parent.

#0  0x04049e64 in nsBaseWidget::GetRenderingContext (this=0xef9f310) at
/Users/mark/lizard/trunk/mozilla/widget/src/xpwidgets/nsBaseWidget.cpp:605
#1  0x04042274 in nsWindow::PaintUpdateRectProc (message=62224, rgn=0x2dcc14,
inDirtyRect=0xbfffdf9c, refCon=0xef9f310) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsWindow.cpp:1298
#2  0x0404467c in nsWindow::HandleUpdateEvent (this=0xef9f310,
regionToValidate=0x2dcc4c) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsWindow.cpp:1520
#3  0x0404427c in nsWindow::Update (this=0xef9f310) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsWindow.cpp:1259
#4  0x0404389c in nsWindow::Invalidate (this=0xef9f310, aRect=@0xb8239a0,
aIsSynchronous=1) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsWindow.cpp:1058
#5  0x0404112c in nsWindow::Invalidate (this=0x6a680000,
aIsSynchronous=-1073750116) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsWindow.cpp:1025
#6  0x04046bf0 in nsNativeScrollbar::SetPosition (this=0xef9f310, aPos=3001364)
at /Users/mark/lizard/trunk/mozilla/widget/src/mac/nsNativeScrollbar.cpp:428
#7  0x04046f50 in nsNativeScrollbar::UpdateContentPosition (this=0xef9f310,
inNewPos=252) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsNativeScrollbar.cpp:266
#8  0x04047410 in nsNativeScrollbar::DoScrollAction (this=0xef9f310, part=129)
at /Users/mark/lizard/trunk/mozilla/widget/src/mac/nsNativeScrollbar.cpp:224
#9  0x04047598 in nsNativeScrollbar::ScrollActionProc (ctrl=0xef9f310,
part=129) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsNativeScrollbar.cpp:132
#10 0x931dfd58 in HIView::DragLiveIndicator ()

#0  0x0203de88 in nsBaseWidget::GetRenderingContext (this=0xfd0b560) at
/Users/mark/lizard/trunk/mozilla/widget/src/xpwidgets/nsBaseWidget.cpp:605
#1  0x02036274 in nsWindow::PaintUpdateRectProc (message=46432, rgn=0x2dc9b4,
inDirtyRect=0xbfffe1cc, refCon=0xfd0b560) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsWindow.cpp:1298
#2  0x0203867c in nsWindow::HandleUpdateEvent (this=0xfd0b560,
regionToValidate=0x2dcc58) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsWindow.cpp:1520
#3  0x0203827c in nsWindow::Update (this=0xfd0b560) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsWindow.cpp:1259
#4  0x0203b4cc in nsNativeScrollbar::DoScrollAction (this=0xef69e70, part=129)
at /Users/mark/lizard/trunk/mozilla/widget/src/mac/nsNativeScrollbar.cpp:250
#5  0x0203b5bc in nsNativeScrollbar::ScrollActionProc (ctrl=0xfd0b560,
part=129) at
/Users/mark/lizard/trunk/mozilla/widget/src/mac/nsNativeScrollbar.cpp:139
#6  0x931dfd58 in HIView::DragLiveIndicator ()

Comment 7

[Mark Mentovai]

Confirming same infrequent crash when PostEvent is not used - should go with
PostEvent, it's nicer.

Comment 8

[Mark Mentovai]

Attachment: Fix rare crashing

What I've done here is prevent all occurrences of the crash by tracking
destruction as necessary in all affected objects.

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 188046 [details] [diff] [review]
Fix rare crashing

> Index: mozilla/widget/src/mac/nsNativeScrollbar.cpp
> ===================================================================

>  //
> +// Destroy
> +//
> +// Now you're gone, gone, gone, whoa-oh...
> +//
> +NS_IMETHODIMP
> +nsNativeScrollbar::Destroy()
> +{
> +  if (mMouseDownInScroll)
> +  {
> +    PostEvent(mouseUp, 0);
> +  }
> +  mDestroyed = PR_TRUE;
> +  return NS_OK;
> +}

Should this call Destroy() on its superclass? I would think so.


>  nsNativeScrollbar::DoScrollAction(ControlPartCode part)
>  {
>    PRUint32 oldPos, newPos;
>    PRUint32 incr;
>    PRUint32 visibleImageSize;
> +
> +  if ( mDestroyed )
> +    return;

Convention is no spaces around ()

> Index: mozilla/widget/src/mac/nsNativeScrollbar.h
> ===================================================================

> @@ -90,15 +92,16 @@
>  
>    nsIContent*       mContent;          // the content node that affects the scrollbar's value
>    nsIScrollbarMediator* mMediator;     // for scrolling with outliners
>    nsISupports*      mScrollbar;        // for calling into the mediator
>    
>    PRUint32          mMaxValue;
>    PRUint32          mVisibleImageSize;
>    PRUint32          mLineIncrement;
>    PRBool            mMouseDownInScroll;
>    ControlPartCode   mClickedPartCode;
> +  PRBool            mDestroyed;
>  };

Do we need another 'mDestroyed'? nsWindow has mDestroyCalled, and
nsBaseWidget has mOnDestroyCalled. I think we need some cleanup here,
but for now, maybe just reuse nsWindows's mDestroyCalled.

> Index: mozilla/widget/src/xpwidgets/nsBaseWidget.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/widget/src/xpwidgets/nsBaseWidget.cpp,v
> retrieving revision 1.133
> diff -u -1 -0 -r1.133 nsBaseWidget.cpp
> --- mozilla/widget/src/xpwidgets/nsBaseWidget.cpp	1 Jul 2005 04:29:42 -0000	1.133
> +++ mozilla/widget/src/xpwidgets/nsBaseWidget.cpp	2 Jul 2005 17:32:29 -0000
> @@ -595,20 +595,23 @@
>  //-------------------------------------------------------------------------
>  //
>  // Create a rendering context from this nsBaseWidget
>  //
>  //-------------------------------------------------------------------------
>  nsIRenderingContext* nsBaseWidget::GetRenderingContext()
>  {
>    nsresult                      rv;
>    nsCOMPtr<nsIRenderingContext> renderingCtx;
>  
> +  if (mOnDestroyCalled)
> +    return nsnull;
> +

Does this ever get hit with the other changes in this patch?


>    rv = mContext->CreateRenderingContextInstance(*getter_AddRefs(renderingCtx));
>    if (NS_SUCCEEDED(rv)) {
>      rv = renderingCtx->Init(mContext, this);
>      if (NS_SUCCEEDED(rv)) {
>        nsIRenderingContext *ret = renderingCtx;
>        /* Increment object refcount that the |ret| object is still a valid one
>         * after we leave this function... */
>        NS_ADDREF(ret);
>        return ret;
>      }
> @@ -660,20 +663,21 @@
>  }
>  
>  
>  //-------------------------------------------------------------------------
>  //
>  // Destroy the window
>  //
>  //-------------------------------------------------------------------------
>  void nsBaseWidget::OnDestroy()
>  {
> +  mOnDestroyCalled = PR_TRUE;

The other platforms set mOnDestroyCalled from their nsBaseWidget subclasses.
Maybe we should follow suit (removing our redundant destroyed flags).

Comment 10

[Mark Mentovai]

Attachment: Cleanup

re overiding Destroy() method:
> Should this call Destroy() on its superclass? I would think so.

I thought so too, but it caused crashes.  I see what it is now, though: I was
skipping right to nsBaseWidget.

> Convention is no spaces around ()

I found examples of both in the file, and wasn't sure what local convention
prevailed.  Removed.

> Do we need another 'mDestroyed'? nsWindow has mDestroyCalled, and
> nsBaseWidget has mOnDestroyCalled. I think we need some cleanup here,
> but for now, maybe just reuse nsWindows's mDestroyCalled.
[...]
> The other platforms set mOnDestroyCalled from their nsBaseWidget subclasses.
> Maybe we should follow suit (removing our redundant destroyed flags).

Using mOnDestroyCalled.

re gating off nsBaseWidget::GetRenderingContext():
> Does this ever get hit with the other changes in this patch?

I know, it doesn't seem like it would.	It does, with the first backtrace from
comment 6.  It doesn't help to check mOnDestroyCalled in nsWindow::Update.

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 188057 [details] [diff] [review]
Cleanup

>Index: mozilla/widget/src/mac/nsNativeScrollbar.cpp
>===================================================================

> //
>+// Destroy
>+//
>+// Now you're gone, gone, gone, whoa-oh...
>+//
>+NS_IMETHODIMP
>+nsNativeScrollbar::Destroy()
>+{
>+  if (mMouseDownInScroll)
>+  {
>+    PostEvent(mouseUp, 0);
>+  }
>+  nsMacControl::Destroy();
>+  return NS_OK;

  return nsMacControl::Destroy(); ?

>Index: mozilla/widget/src/mac/nsWindow.cpp
>===================================================================

> NS_IMETHODIMP nsWindow::Destroy()
> {
>-	if (mDestroyCalled)
>+	if (mOnDestroyCalled)
> 		return NS_OK;
>-	mDestroyCalled = PR_TRUE;
> 
> 	nsBaseWidget::OnDestroy();
> 	nsBaseWidget::Destroy();
>+	mOnDestroyCalled = PR_TRUE;
> 	mParent = 0;

The other widget versions set mOnDestroyCalled before calling
nsBaseWidget::OnDestroy(), and I think we should too.

Fix tthat, and sr=sfraser

Comment 12

[Josh Aas]

Comment on attachment 188057 [details] [diff] [review]
Cleanup

works for me

Comment 13

[Mark Mentovai]

Attachment: v5, smfr's comments + missing file

nsNativeControl::Destroy():
> return nsMacControl::Destroy(); ?

Yes.

nsWindow::Destroy():

> The other widget versions set mOnDestroyCalled before calling
> nsBaseWidget::OnDestroy(), and I think we should too.

I did it late for a reason, but if it's done early elsewhere, I've got no
problem doing it early here.  Integrated.

I left out the diff to nsWindow.h the last revision.  mDestroyCalled is no
longer used anywhere, it's been removed from the class.  Maybe keep this in
mind for the Cocoa widgets?

Awaiting a+ for this 1.8b3 blocker.

Comment 14

[Mark Mentovai]

Checked in.

Comment 15

[Wayne Woods]

Guys, I suspect the fix for this regression caused bug 301567. Could you please
check it out and see?
cheers