435422 - crash [@ nsScrollPortView::IncrementalScroll()]

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Mats Palmgren (inactive)]

crash [@ nsScrollPortView::IncrementalScroll()]

Currently at #7 among MacOSX top crashers for "Firefox 3.0" with 512
reported crashes in the past two weeks.

A few example crash IDs:
bp-132c7b66-2742-11dd-8954-001321b13766
bp-7fce9933-26d9-11dd-a0c8-001cc45a2c28
bp-6685d12e-2623-11dd-ba49-001cc45a2ce4


nsScrollPortView::IncrementalScroll	 mozilla/view/src/nsScrollPortView.cpp:729
nsTimerImpl::Fire	mozilla/xpcom/threads/nsTimerImpl.cpp:400
nsTimerEvent::Run	mozilla/xpcom/threads/nsTimerImpl.cpp:490
nsThread::ProcessNextEvent	mozilla/xpcom/threads/nsThread.cpp:510
NS_ProcessPendingEvents_P	nsThreadUtils.cpp:180
nsBaseAppShell::NativeEventCallback	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:121
nsAppShell::ProcessGeckoEvents	mozilla/widget/src/cocoa/nsAppShell.mm:302
CoreFoundation@0x21ef9	
CoreFoundation@0x21a35	
HIToolbox@0x9877	
HIToolbox@0x8eb8	
HIToolbox@0x8dd8	
AppKit@0x170e4	
AppKit@0x16cd6	
AppKit@0x10a63	
nsAppShell::Run	mozilla/widget/src/cocoa/nsAppShell.mm:591

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

I'm guessing the problem is that nsIWidget::Scroll() flushes an event
(see bug 402505) that destroys the frame (and view) that we're currently
scrolling and when we return to nsScrollPortView scroll code 'this' has
been deleted.

Comment 2

[Mats Palmgren (inactive)]

Nominating this low-risk crash fix for RC2.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we need this for RC2.

Mats, I'd like to know what is causing stuff to be torn down here. One possibility is that nsChildView::drawRect is flushing a paint event, which calls presShell->WillPaint via nsViewManager::DispatchEvent, which does a FlushNotifications which can delete frames and views in strange situations (e.g. block-in-inline situations). But that WillPaint call is guarded by mScrollCnt == 0. That should have been incremented by nsScrollPortView::ScrollTo (since CanScrollWithBitBlit always returns true on Mac) so that doesn't seems to be the problem. So what is the problem?

Comment 4

[Mike Shaver (:shaver emeritus)]

blocking- based on roc's assessment, and the request for deeper understanding.

Comment 5

[Mats Palmgren (inactive)]

The call chain I was hinting at (see attachment 291858 [details]):
nsScrollPortView::IncrementalScroll()
  nsScrollPortView::ScrollToImpl()
    nsScrollPortView::Scroll()
      nsChildView::Scroll()    (in widget/src/cocoa/nsChildView.mm)
        nsChildView::Resize()
          nsChildView::ReportSizeEvent()
            nsChildView::DispatchWindowEvent()
              nsChildView::DispatchEvent()
                ... destroys arbitrary layout things ...

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Interesting. But nsChildView::Scroll does
    nsRect bounds;
    kid->GetBounds(bounds);
    kid->Resize(bounds.x + aDx, bounds.y + aDy, bounds.width, bounds.height, PR_FALSE);

and then nsChildView::Resize does
  BOOL isResizing = (mBounds.width != aWidth || mBounds.height != aHeight);
so that should be false, and then
  if (isResizing)
    ReportSizeEvent();

so I don't see how we get that stack trace.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Generally we've tried to avoid make nsScrollPortView::ScrollTo a "can tear down the world" point, and I'd like to keep it that way, at least until Compositor makes scrolling asynchronous.

Comment 8

[Mats Palmgren (inactive)]

Ah, so the stack probably is:
    ...
      nsChildView::Scroll()
        nsChildView::Resize()
          nsChildView::ReportMoveEvent()
            nsChildView::DispatchWindowEvent()
              ...

  BOOL isMoving = (mBounds.x != aX || mBounds.y != aY);

which should be true.

  if (isMoving) {
    ReportMoveEvent();
    if (mOnDestroyCalled)
      return NS_OK;
  }

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsViewManager::DispatchEvent eats NS_MOVE events, so they shouldn't lead to any teardowns.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, nsWebShellWindow does handle NS_MOVE, but it doesn't do anything that looks unsafe:
http://mxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsWebShellWindow.cpp#328
and more importantly, we should not be finding *child* nsWebShellWindows.

Comment 11

[Mats Palmgren (inactive)]

Let's look at nsScrollPortView::ScrollToImpl() a bit:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/view/src/nsScrollPortView.cpp&rev=3.90&root=/cvsroot&mark=635-648,668-678,664#603

A plugin could be one of those listeners, right?
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsObjectFrame.cpp&rev=1.653&mark=3160,3185,3206,3207#3160

There's a sync Invalidate() there which leads to the scenario you
described in comment 3.

There's only one call to WillBitBlit() in nsScrollPortView and that's
in nsScrollPortView::Scroll(), so any NS_PAINTs before or after the
Scroll() call will not be protected by mScrollCnt, such as the one
in nsObjectFrame::ScrollPositionDidChange() above.

(FWIW, there's also some evidence the widget might already be destroyed
there: bug 269832.)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> Let's look at nsScrollPortView::ScrollToImpl() a bit:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/view/src/nsScrollPortView.cpp&rev=3.90&root=/cvsroot&mark=635-648,668-678,664#603
> 
> A plugin could be one of those listeners, right?
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsObjectFrame.cpp&rev=1.653&mark=3160,3185,3206,3207#3160
> 
> There's a sync Invalidate() there which leads to the scenario you
> described in comment 3.

Hmm. That is a plugin widget, so on non-Mac platforms that would trigger a paint event for the plugin which would be consumed by the plugin without reaching the view manager and triggering flushes. But on Mac, yeah, I think it could cause other painting to occur :-(. Repainting the entire window an extra time every time we scroll a page with a plugin in it is *horrible*! Josh, do we really need to do that? Can we at least make that Invalidate async?

Mats, does that making that invalidate async fix the bug? Because if it doesn't, we need another theory, or better still, actual data.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 15

[Mats Palmgren (inactive)]

> does that making that invalidate async fix the bug?

This bug does not have STR, so there's no way of knowing for sure.
I tried rev. 3 on OSX, scrolling pages with embedded Flash and
I didn't see any regressions with that anyway.
I think we should take both Patch 2 and 3.

I'm worried over the number of incidents, it's now 1117 incidents in
the past two weeks for "Firefox 3.0" alone.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 322890 [details] [diff] [review]
Patch rev. 2

Actually I'm not sure about this patch. The problem is that stuff gets torn down, frame code that calls this method is pretty sure to blow up anyway.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm doubtful patch 2 is actually going to help much.

Patch 3 probably needs more baking. 3.0.1 material.

Comment 18

[chris hofmann]

looks like this has moved up to #11 spot on the RC2 top crash list.  

Comment 19

[Mats Palmgren (inactive)]

For "Firefox 3.0" it's now at #5 on MacOSX and #31 overall.
What's interesting is that there are now crashes on Windows and Linux too:
(for past two weeks)
Win      OSX     Lin
13 	 1979 	 3 

Comment 20

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/7651d3bb0926

-> FIXED

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 322891 [details] [diff] [review]
Patch rev. 3

Been on mozilla-central for a long time, when the 1.9 branch opens for 1.9.0.2 checkins, we should take this.

Comment 22

[Mats Palmgren (inactive)]

Landed on CVS trunk for 1.9.0.2:
mozilla/layout/generic/nsObjectFrame.cpp 	1.654 

Comment 23

[Tony Chung [:tchung]]

can we get some STR to verify this bug?  Thanks.

Comment 24

[Mats Palmgren (inactive)]

There is no known STR.  The bug was filed based on crash data alone.
See comment 15.  Maybe there are some URLs or comments in the submitted
crash data that could be useful, but due to bug 444749 it's impossible
to tell right now.

Comment 26

[Samuel Sidler (old account; do not CC)]

Marking verified since this no longer appears in the topcrash list.

Comment 27

[Mats Palmgren (inactive)]

This crash still occurs but less frequent now, there's 174 reported crashes
in the past two weeks: 105 on Windows, 58 on Mac, 11 on Linux.
Someone reported it in bug 490461, I'm going to try reproducing that.