322084 - Crash with nsAutoScrollTimer [@ nsSelection::ConstrainFrameAndPointToAnchorSubtree]

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Eli Friedman]

Crash with testcase I will post.

Steps for testcase:
1. Scroll down to bottom of div
2. Click button
3. Start selection next to button
4. Drag up

The testcase detects this occurance, removes the div from the document, and starts a loop which allocates a bunch of memory.  The crash occurs in the autoscroll code, which runs off a timer.

Stack trace in my trunk build:
#0  0x10676f81 in nsSelection::ConstrainFrameAndPointToAnchorSubtree (
    this=0xd6bc920, aPresContext=0xc3a7c40, aFrame=0x40008, aPoint=@0xd6e0610,
    aRetFrame=0x22ebb4, aRetPoint=@0x22ebd8)
    at c:/mozilla/mozilla/layout/generic/nsIFrame.h:546
#1  0x10679237 in nsSelection::HandleDrag (this=0xd6bc920,
    aPresContext=0xc3a7c40, aFrame=0x40008, aPoint=@0xd6e0610)
    at c:/mozilla/mozilla/layout/generic/nsSelection.cpp:2446
#2  0x10b0a460 in nsAutoScrollTimer::Notify (this=0xd6e05f0, timer=0xd6e0b40)
    at c:/mozilla/mozilla/layout/generic/nsSelection.cpp:621
#3  0x0087cc45 in nsTimerImpl::Fire (this=0xd6e0b40)
    at c:/mozilla/mozilla/xpcom/threads/nsTimerImpl.cpp:403
#4  0x0087d686 in nsTimerManager::FireNextIdleTimer (this=0x87dd850)
    at c:/mozilla/mozilla/xpcom/threads/nsTimerImpl.cpp:634
#5  0x069e3207 in nsAppShell::GetNativeEvent (this=0xc4fb1c8,
    aRealEvent=@0x22ed80, aEvent=@0x22ed84)
    at c:/mozilla/mozilla/widget/src/windows/nsAppShell.cpp:196
#6  0x08cf72ce in nsXULWindow::ShowModal (this=0xd671f00)
    at c:/mozilla/mozilla/xpfe/appshell/src/nsXULWindow.cpp:399
#7  0x08cf480b in nsContentTreeOwner::ShowAsModal (this=0xd664660)
    at c:/mozilla/mozilla/xpfe/appshell/src/nsContentTreeOwner.cpp:430
#8  0x07bf5073 in nsWindowWatcher::OpenWindowJS (this=0x21217e0,
    aParent=0xc53eef0,
    aUrl=0x7c39ca0 "chrome://global/content/commonDialog.xul",
---Type <return> to continue, or q <return> to quit---
    aName=0x7c39b4f "_blank",
    aFeatures=0x7c39b2c "centerscreen,chrome,modal,titlebar", aDialog=1,
    argc=1, argv=0xd791c78, _retval=0x22f2e8)
    at c:/mozilla/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher
.cpp:822
#9  0x07bf398f in nsWindowWatcher::OpenWindow (this=0x21217e0,
    aParent=0xc53eef0,
    aUrl=0x7c39ca0 "chrome://global/content/commonDialog.xul",
    aName=0x7c39b4f "_blank",
    aFeatures=0x7c39b2c "centerscreen,chrome,modal,titlebar",
    aArguments=0xc60cbd8, _retval=0x22f2e8)
    at c:/mozilla/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher
.cpp:473
#10 0x07bfbaf3 in nsPromptService::DoDialog (this=0xd5ab438,
    aParent=0xc53eef0, aParamBlock=0xc60cbd8,
    aChromeURL=0x7c39ca0 "chrome://global/content/commonDialog.xul")
    at ../../../../dist/include/xpcom/nsCOMPtr.h:1149
#11 0x07bfa876 in nsPromptService::ConfirmEx (this=0xd5ab438,
    parent=0xc53eef0, dialogTitle=0xd658340, text=0xd6a4b10,
    buttonFlags=32639, button0Title=0xd66c078, button1Title=0xd624e28,
    button2Title=0x0, checkMsg=0xd66d070, checkValue=0x22f5a0,
    buttonPressed=0x22f5a4)
    at c:/mozilla/mozilla/embedding/components/windowwatcher/src/nsPromptService
---Type <return> to continue, or q <return> to quit---
.cpp:78
#12 0x07bf259b in nsPrompt::ConfirmEx (this=0xd6b91c0, dialogTitle=0xd658340,
    text=0xd6a4b10, buttonFlags=32639, button0Title=0xd66c078,
    button1Title=0xd624e28, button2Title=0x0, checkMsg=0xd66d070,
    checkValue=0x22f5a0, buttonPressed=0x22f5a4)
    at c:/mozilla/mozilla/embedding/components/windowwatcher/src/nsPrompt.cpp:28
4
#13 0x109535bc in nsJSContext::DOMBranchCallback (cx=0xc53f0d8,
    script=0xd5ec1a0) at ../../../dist/include/string/nsTString.h:610
#14 0x00594922 in js_Interpret (cx=0xc53f0d8,
    pc=0xd5ec251 "\006 &#945;&#9500;&#9617;c\003g\nc\r\017&#9474;f\003g\nc\r&#9558;\f&#9569;c\003g\nc\rc\020g\003g\
nc!&#9508;1#\b\034 &#9568;&#9617;&#9577;`\a&#9488;\t&#9565;\n", result=0x22f8f4)
    at c:/mozilla/mozilla/js/src/jsinterp.c:2197
#15 0x005927be in js_Invoke (cx=0xc53f0d8, argc=1, flags=2)
    at c:/mozilla/mozilla/js/src/jsinterp.c:1253
#16 0x00592a6a in js_InternalInvoke (cx=0xc53f0d8, obj=0xc2c2b28,
    fval=204223200, flags=0, argc=1, argv=0xd6c6aa8, rval=0x22fb20)
    at c:/mozilla/mozilla/js/src/jsinterp.c:1330
#17 0x00568da1 in JS_CallFunctionValue (cx=0xc53f0d8, obj=0xc2c2b28,
    fval=204223200, argc=1, argv=0xd6c6aa8, rval=0x22fb20)
    at c:/mozilla/mozilla/js/src/jsapi.c:4157
#18 0x109553b7 in nsJSContext::CallEventHandler (this=0xc53f008,
    aTarget=0xc2c2b28, aHandler=0xc2c32e0, argc=1, argv=0xd6c6aa8,
---Type <return> to continue, or q <return> to quit---
    rval=0x22fb20) at c:/mozilla/mozilla/dom/src/base/nsJSEnvironment.cpp:1423
#19 0x1096b9cc in nsGlobalWindow::RunTimeout (this=0xd5e9748,
    aTimeout=0xd6be610)
    at c:/mozilla/mozilla/dom/src/base/nsGlobalWindow.cpp:6243
#20 0x1096c2ba in nsGlobalWindow::TimerCallback (aTimer=0xd6be6c8,
    aClosure=0xd6be610)
    at c:/mozilla/mozilla/dom/src/base/nsGlobalWindow.cpp:6602
#21 0x0087cc31 in nsTimerImpl::Fire (this=0xd6be6c8)
    at c:/mozilla/mozilla/xpcom/threads/nsTimerImpl.cpp:400
#22 0x0087d686 in nsTimerManager::FireNextIdleTimer (this=0x87dd850)
    at c:/mozilla/mozilla/xpcom/threads/nsTimerImpl.cpp:634
#23 0x069e2f5f in nsAppShell::Run (this=0x3efa70)
    at c:/mozilla/mozilla/widget/src/windows/nsAppShell.cpp:141
#24 0x07512086 in nsAppStartup::Run (this=0x3efa30)
    at c:/mozilla/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:161
#25 0x100079cb in XRE_main (argc=3, argv=0x3e67b0, aAppData=0x403080)
    at c:/mozilla/mozilla/toolkit/xre/nsAppRunner.cpp:2298
#26 0x00401303 in main (argc=3, argv=0x3e67b0)
    at c:/mozilla/mozilla/browser/app/nsBrowserApp.cpp:61

TB for 1.5: TB13472565M.  Trace from there:

nsSelection::ConstrainFrameAndPointToAnchorSubtree  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsSelection.cpp, line 1121]
nsSelection::HandleDrag  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsSelection.cpp, line 2403]
nsAutoScrollTimer::Notify  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsSelection.cpp, line 620]
nsAppStartup::Run  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 151]
main  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp, line 61]

Different trace because it takes much less time to run in an optimized build.

Comment 1

[Eli Friedman]

Attachment: Testcase

Comment 2

[jpl24]

I can get this to consistently crash in the current trunk by following the steps outlined above, waiting for the stop script dialog box to come up, waiting a few seconds and clicking stop. If it doesn't crash immediately it will crash eventually.

Comment 3

[jpl24]

So after some debugging, what happens is that the AutoScrollTimer caches a view when the timer is started, but the view is destroyed before the timer fires. So it tries to get the frame from the deleted view, and crashes. If the framed that owned the selection was destroyed first this would stop the timer, but it's not.

Comment 4

[jpl24]

Looks like there are about 100 talkback reports with nsAutoScrollTimer near the top of the stack. Mostly on the google personal homepage, http://www.google.com/ig.

Comment 5

[jpl24]

I've got a patch that fixes this, I'll attach it when I get it cleaned up.

Comment 6

[jpl24]

Attachment: patch_v1

This patch modifies the AutoScrollTimer to store the widget and frame for an event instead of calculating the view and storing it. The view could be destroyed before the timer fired which led to the crash. The view is very disconnected from the timer, so I couldn't see a good way to stop the timer from the view's destructor.

With the patch I don't crash on this testcase.

Comment 7

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

Frames can be destroyed at any time, so you don't want to keep the frame either.

Comment 8

[jpl24]

I couldn't figure out a way not to cache the frame, so I stopped the timer from the frame's destroy function. If there's a good way not to cache the frame that'd be better though.

Comment 9

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

Ah, I missed that chunk, but you don't really want to do that. Random stuff happening in one place would stop any nsAutoScrollTimer in the document.

-  if (mAutoScrollTimer)
+  if (mAutoScrollTimer) {
     return mAutoScrollTimer->Stop();
+  }

Don't do this.

I think the way to fix this is to change GetNearestCapturingView to return the frame which returned non-null for GetMouseCapturer, or the root frame. Then have nsAutoScrollTimer store a strong reference to that frame's GetContent(). Also keep the mPresContext (and clear it out in DetachFromPresentation). Then when you need the capturing view, if mPresContext is null you just have to bail because the whole document is dead. Otherwise you can call mPresContext->PresShell()->GetPrimaryFrameFor(content) to get the element's current frame, and call GetMouseCapturer on that frame to get the correct view.

The general principle is that frames and views can go away at any time, but content (i.e., DOM elements) will live as long as anyone holds strong references to them, so any non-ephemeral references should refer to content if at all possible.

Thanks for working on this!

Comment 10

[jpl24]

Ah okay, the frame stopping the timer didn't seem right. Thanks for the pointers, I'll try and work out a patch that does what you suggested.

Comment 11

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

Actually, that won't work for the root frame case, because the root frame is not the primary frame for its content. In that case you should probably just store null for the content and later check if the content is null, and if it is, use the prescontext->PresShell()->FrameManager()->GetRootFrame().

You should also warn if the GetPrimaryFrameFor for the content reference you're storing is not the frame you got the content from. This should never fire because at this point we're dealing only with scroll frames and I believe scroll frames are always the primary frames for their content. If you later find that the primary frame's GetMouseCapturer returns null, you can just bail and not crash. This could actually happen, if for example in between the drag starting and the autoscroll timer firing, someone changes the style of the element from "overflow:scroll" to "overflow:visible" so it's not scrollable anymore!

["GetPrimaryFrameFor(frame->GetContent()) != frame" can happen when we have multiple frames for content, either because we create a nested stack of frames for an element (e.g., pseudo table frames to handle malformed HTML table content), or because we create continuation frames when the element has to break across some boundary. We do not break scrollframes and I believe scrollframes are always the top of a nested stack.]

Comment 12

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

YOu could also end up getting null from GetPrimaryFrameFor if the style of the element changes to "display:none", so you need to check that too.

Comment 13

[jpl24]

> ["GetPrimaryFrameFor(frame->GetContent()) != frame" can happen when we have
> multiple frames for content, either because we create a nested stack of frames
> for an element (e.g., pseudo table frames to handle malformed HTML table
> content), or because we create continuation frames when the element has to
> break across some boundary. We do not break scrollframes and I believe
> scrollframes are always the top of a nested stack.]

I tried this, and the code is passing in an HTMLScrollFrame as the frame with a mouse capturer, and GetPrimaryFrameFor(frame->GetContent()) is returning a frame of type Area(html). Is it possible the scroll frame is not at the top of the stack?

Comment 14

[Eli Friedman]

For the root content, the scrollframe is not the primary frame.  It should work for other frames, though.

I think that it would be a better model to have the scrollframe own the autoscroll timer because it's more straightforward than assuming stuff about the frame tree, but it's not really important.

Comment 15

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

That would be slightly simpler, but it would mean that a reframe of the scrolled content would cancel autoscrolling, which is not a big deal, but would be nice to avoid.

So I suggest you do this sort of thing:
  nsIFrame* f = GetNearestCapturingFrame();
  if (f) {
    mContent = f->GetContent();
    NS_ASSERTION(!mContent->GetParent() || // it's root?
                 GetPrimaryFrameFor(mContent) == f);
  } else {
    mContent = GetRootContent(); // should be something for this in nsIDocument
  }
...
  nsIFrame* f = GetNearestCapturingFrame(GetPrimaryFrameFor(mContent));
  nsIView* v = f->GetMouseCapturer();

Comment 16

[jpl24]

Attachment: patch_v2

Current work in progress. I'm still tracking down a bug in the patch where the selection turns off when the timer fires.

Comment 17

[jpl24]

Attachment: patch_v3

This fixes the crash and doesn't regress scrolling in any way I could find.

Comment 18

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

You need to set mContent to null in Stop() and Notify().

Don't save mFrame and mView, even in debug builds, because it's not safe and it will cause assertions to fire in testcases like this one, even though we're doing the right thing. Just remove them, and remove the assertions in Notify() that depend on them.

Other than that, this looks great!

Comment 19

[jpl24]

Attachment: patch_v4

Fixed comments

Comment 20

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

Comment on attachment 225577 [details] [diff] [review]
patch_v4

get someone on IRC to help you check this in. This is great, thanks!

Comment 21

[jpl24]

Thanks for reviewing this! I'll look for somebody after the tree opens.

Comment 22

[jpl24]

Patch checked in on the trunk by mrbkap

Comment 23

[Adam Guthrie]

jpl24, should this be marked FIXED now?

Comment 24

[jpl24]

Yes, it should be.