Closed Bug 299384 Opened 16 years ago Closed 16 years ago

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

Categories

(Core Graveyard :: Widget: Mac, defect)

PowerPC
macOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: uriber, Assigned: mark)

References

()

Details

(Keywords: crash, regression)

Attachments

(2 files, 4 obsolete files)

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
Attached file Apple crash log
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.
Flags: blocking1.8b3?
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
Unsurprisingly, this regressed between 2005-06-21 and 2005-06-22.
Severity: normal → critical
Flags: blocking1.8b3? → blocking1.8b3+
Attached patch Patch to fix crash (obsolete) — Splinter Review
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).
Attachment #188004 - Flags: superreview?(jhpedemonte)
Attachment #188004 - Flags: review?(joshmoz)
Attached patch With PostEvent (obsolete) — Splinter Review
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 ()
Attachment #188007 - Flags: superreview?(sfraser_bugs)
Attachment #188007 - Flags: review?(joshmoz)
Confirming same infrequent crash when PostEvent is not used - should go with
PostEvent, it's nicer.
Attached patch Fix rare crashing (obsolete) — Splinter Review
What I've done here is prevent all occurrences of the crash by tracking
destruction as necessary in all affected objects.
Assignee: joshmoz → mark
Attachment #188004 - Attachment is obsolete: true
Attachment #188007 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #188046 - Flags: superreview?(sfraser_bugs)
Attachment #188046 - Flags: review?(joshmoz)
Attachment #188007 - Flags: superreview?(sfraser_bugs)
Attachment #188007 - Flags: review?(joshmoz)
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).
Attachment #188046 - Flags: superreview?(sfraser_bugs) → superreview-
Attachment #188046 - Flags: review?(joshmoz)
Attached patch Cleanup (obsolete) — Splinter Review
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.
Attachment #188046 - Attachment is obsolete: true
Attachment #188057 - Flags: superreview?(sfraser_bugs)
Attachment #188057 - Flags: review?(joshmoz)
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
Attachment #188057 - Flags: superreview?(sfraser_bugs) → superreview+
Comment on attachment 188057 [details] [diff] [review]
Cleanup

works for me
Attachment #188057 - Flags: review?(joshmoz) → review+
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.
Attachment #188057 - Attachment is obsolete: true
Attachment #188076 - Flags: approval1.8b3?
Attachment #188076 - Flags: approval1.8b3? → approval1.8b3+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Blocks: 269388
Attachment #188004 - Flags: superreview?(jhpedemonte)
Attachment #188004 - Flags: review?(joshmoz)
Guys, I suspect the fix for this regression caused bug 301567. Could you please
check it out and see?
cheers
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.