Crash with nsAutoScrollTimer [@ nsSelection::ConstrainFrameAndPointToAnchorSubtree]

RESOLVED FIXED in mozilla1.9alpha1

Status

()

--
critical
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: sharparrow1, Assigned: jlurz24)

Tracking

({crash})

Trunk
mozilla1.9alpha1
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 207320 [details]
Testcase

Updated

13 years ago
Severity: normal → critical
OS: Windows XP → All
Hardware: PC → All
Summary: Crash with nsAutoScrollTimer → Crash with nsAutoScrollTimer [@ nsSelection::ConstrainFrameAndPointToAnchorSubtree]
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

12 years ago
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.
(Assignee)

Comment 4

12 years ago
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.
(Assignee)

Comment 5

12 years ago
I've got a patch that fixes this, I'll attach it when I get it cleaned up.
(Assignee)

Comment 6

12 years ago
Created attachment 221701 [details] [diff] [review]
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.
Attachment #221701 - Flags: review?(roc)
Frames can be destroyed at any time, so you don't want to keep the frame either.
(Assignee)

Comment 8

12 years ago
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.
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!
(Assignee)

Comment 10

12 years ago
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.
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.]
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.
(Assignee)

Comment 13

12 years ago
> ["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?
(Reporter)

Comment 14

12 years ago
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.
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();
(Assignee)

Comment 16

12 years ago
Created attachment 224390 [details] [diff] [review]
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.
Attachment #221701 - Attachment is obsolete: true
Attachment #221701 - Flags: review?(roc)
(Assignee)

Comment 17

12 years ago
Created attachment 225370 [details] [diff] [review]
patch_v3

This fixes the crash and doesn't regress scrolling in any way I could find.
Attachment #224390 - Attachment is obsolete: true
Attachment #225370 - Flags: review?(roc)
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!
(Assignee)

Comment 19

12 years ago
Created attachment 225577 [details] [diff] [review]
patch_v4

Fixed comments
Attachment #225370 - Attachment is obsolete: true
Attachment #225577 - Flags: review?(roc)
Attachment #225370 - Flags: review?(roc)
Comment on attachment 225577 [details] [diff] [review]
patch_v4

get someone on IRC to help you check this in. This is great, thanks!
Attachment #225577 - Flags: superreview+
Attachment #225577 - Flags: review?(roc)
Attachment #225577 - Flags: review+
(Assignee)

Comment 21

12 years ago
Thanks for reviewing this! I'll look for somebody after the tree opens.
(Assignee)

Comment 22

12 years ago
Patch checked in on the trunk by mrbkap

Comment 23

12 years ago
jpl24, should this be marked FIXED now?
Assignee: selection → jlurz24
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 24

12 years ago
Yes, it should be.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsSelection::ConstrainFrameAndPointToAnchorSubtree]
You need to log in before you can comment on or make changes to this bug.