Closed Bug 56704 Opened 24 years ago Closed 24 years ago

Crash selecting text


(Core :: DOM: Selection, defect, P1)






(Reporter: bugzilla, Assigned: buster)




(Keywords: crash, Whiteboard: [rtm++] a=waterson, r=erik [InLimbo-OOH])


(3 files)

Build ID: New *trunk* build; not tested in branch yet

Steps to Reproduce:
(1) Go to
(2) Select text from top to bottom

Result:  You crash when starting to select the bottom portion of the page.

Stack Trace:

nsGenericHTMLContainerElement::ChildAt(int 0, nsIContent * & 0x00000000) line 
nsHTMLUListElement::ChildAt(const nsHTMLUListElement * const 0x0145d038, int 0, 
nsIContent * & 0x00000000) line 65 + 22 bytes
nsContentIterator::GetDeepFirstChild(nsCOMPtr<nsIContent> {...}) line 403 + 49 
nsContentSubtreeIterator::Next(nsContentSubtreeIterator * const 0x013e94c0) 
line 1015 + 21 bytes
nsDOMSelection::selectFrames(nsDOMSelection * const 0x014311f0, nsIPresContext 
* 0x01433ef0, nsIDOMRange * 0x013e9750, int 1) line 3807 + 23 bytes
nsDOMSelection::Extend(nsDOMSelection * const 0x014311f0, nsIDOMNode * 
0x0145d030, int 2) line 5312
nsSelection::TakeFocus(nsSelection * const 0x01431260, nsIContent * 0x0145d038, 
unsigned int 2, unsigned int 2, int 1, int 0) line 1800
nsSelection::HandleClick(nsSelection * const 0x01431260, nsIContent * 
0x0145d038, unsigned int 2, unsigned int 2, int 1, int 0, int 0) line 1651 + 35 
nsSelection::HandleDrag(nsSelection * const 0x01431260, nsIPresContext * 
0x01433ef0, nsIFrame * 0x03064fe4, nsPoint & {...}) line 1707 + 37 bytes
nsFrame::HandleDrag(nsFrame * const 0x03064fe4, nsIPresContext * 0x01433ef0, 
nsGUIEvent * 0x007af630, nsEventStatus * 0x007af520) line 1396
nsFrame::HandleEvent(nsFrame * const 0x03064fe4, nsIPresContext * 0x01433ef0, 
nsGUIEvent * 0x007af630, nsEventStatus * 0x007af520) line 767 + 27 bytes
nsBlockFrame::HandleEvent(nsBlockFrame * const 0x03064ab0, nsIPresContext * 
0x01433ef0, nsGUIEvent * 0x007af630, nsEventStatus * 0x007af520) line 6592 + 24 
PresShell::HandleEventInternal(nsEvent * 0x007af630, nsIView * 0x01440580, 
unsigned int 1, nsEventStatus * 0x007af520) line 4903 + 38 bytes
PresShell::HandleEvent(PresShell * const 0x01431354, nsIView * 0x01440580, 
nsGUIEvent * 0x007af630, nsEventStatus * 0x007af520, int 1, int & 1) line 4823 
+ 25 bytes
nsView::HandleEvent(nsView * const 0x01440580, nsGUIEvent * 0x007af630, 
unsigned int 28, nsEventStatus * 0x007af520, int 1, int & 1) line 379
nsViewManager2::DispatchEvent(nsViewManager2 * const 0x014331f0, nsGUIEvent * 
0x007af630, nsEventStatus * 0x007af520) line 1439
HandleEvent(nsGUIEvent * 0x007af630) line 68
nsWindow::DispatchEvent(nsWindow * const 0x01448594, nsGUIEvent * 0x007af630, 
nsEventStatus & nsEventStatus_eIgnore) line 682 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x007af630) line 703
nsWindow::DispatchMouseEvent(unsigned int 300, nsPoint * 0x00000000) line 3895 
+ 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 300, nsPoint * 0x00000000) line 
nsWindow::ProcessMessage(unsigned int 512, unsigned int 1, long 19398899, long 
* 0x007af9ac) line 2942 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x00000220, unsigned int 512, unsigned int 1, 
long 19398899) line 951 + 27 bytes
KERNEL32! bff7363b()
KERNEL32! bff94407()
Keywords: crash
I see this on a debug Mac branch build from today.  Nominate for rtm.
Keywords: rtm
OS: Windows 98 → All
Hardware: PC → All
Whiteboard: [rtm need info]
Target Milestone: --- → M19
This is a problem with generated container element not having the children it 
says it has. 

any hack to get this to not crash is much too risky compared to 49772 (which 
also wont be checked in) that I do not want to work on this for rtm. This hack 
would probably be in layout somewhere around the building of generated content.

Incidentally this would have happened to be fixed by 49772 which will not be 
taken as RTM.(even though i believe it to be safe)

marking this rtm+ to make sure pdt is in agreement with my thinking on this bug 
and 49772. (perhaps the fix for 49772 would be reconsidered since it would 
happen to fix a crasher here)
Whiteboard: [rtm need info] → [rtm+]
Note: when loading the testcase, I get two assertions:
###!!! ASSERTION: not a container: 'PR_FALSE', file nsFrame.cpp, line 374
###!!! ASSERTION: not a container: 'PR_FALSE', file nsFrame.cpp, line 374
PDT says need info: Although this is not a topcrash, mostfreq, and does not
occur on a top100 site, it is not the highest priority, but if a simple
bulletproofing fix could be fashioned, that would probably be a Good Thing.
Don't care about the display, as long as it doesn't crash.
Whiteboard: [rtm+] → [rtm need info]
see bug number 49772 please. that is as simple as this gets. if that fix will 
not be taken please mark this rtm- thank you.
adding dependency. I believe pdt marks bugs rtm-/ marking this rtm+ so they can 
mark this rtm-.  since 49772 has been marked rtm- i am assuming at this point 
this will be as well.
Depends on: 49772
forgot the +
Keywords: rtm
Whiteboard: [rtm need info] → [rtm+]
Here is the diff which Mike is proposing to fix this bug:

I believe that this fix is already turned on for the trunk.
Keywords: rtm
Need review and super review. Marking [rtm need info]
Whiteboard: [rtm+] → [rtm need info]
well after looking at it again it doesnt look like generated content is involved 
here. reassigning to buster to look at the assertions and the wierd kind of 
containers that are there but yet not there...

too bad the gen content thing didnt at least mask this :(
Assignee: mjudge → buster
I just attatched the minimal test case that reproduces the crash:

<div style="text-align: justify">
  Select from here ...
    <li style="float: left">... to here for crash!</li>

It turns out that the assertions are caused by the <object> tags in the page, but they aren't the source of the crash.
erik: please review the patch I am about to attach.
waterson: please super-review.
The problem is in nsTextFrame::GetPositionSlowly() an out-param of type
nsIContent is getting set even in a failure case.  No ref count is being added
in the failure case.  The caller will later call
getter_AddRefs(the_same_content_node) in a loop after the failure, which will
cause the content node's ref count to get decremented, even though we never
incremented it back in nsTextFrame::GetPositionSlowly().  So, in the case wehre
this happened to be the last ref count, we've yanked the content node out from
under the frame without nulling out the frame's weak reference to it.
Crash-ola.  Worse, in the case where we *don't* happen to crash during
selection, we can crash unpredictably in other areas of the code because the
text content's ref count is one less than it should be.

Sorry if that sounds a little convoluted.  A quick glance at the patch will make
this all much more clear.
PDT: I think this is a fix for a crash that you should accept for RTM.  Although
the test case is a bit contrived, it's easy to imagine more common crashes that
will be unpredictable and hard to reproduce without this fix.  A common
operation, selection, is the trigger for the crash, but not the cause.  The
cause is an inappropriately-filled in out-param of a ref-counted object, when
the method returns an error.  This causes an extraneous decrement of the objects
ref-count by the caller, which leads to unpredictable crashes depending on when
the object is actually destroyed (the extraneous decrement may not be the one
that takes the ref count to 0, so the object may linger past the selection call
that caused the error.)
Priority: P3 → P1
Wouldn't it be safer to initialize the out parameter to null at the start of
nsTextFrame::GetPosition()? There are so many early exits out of GetPosition()
and GetPositionSlowly() that the one place this patch fixes will miss...
Priority: P1 → P3
still looking for a reviewer.  erik?
waterson, please re-approve the new patch, which includes your suggestion to
init the out-param at the start of the method.
The new patch (10/24/00 09:16) looks good. Hopefully, there is no code somewhere
that depends on the old behavior (ref count too low). Have we checked for new
leaks? Perhaps we should let the patch "bake" on the trunk first?

Minor nit: When I want to make sure that an out param is set to NULL, I check it
first, then I set it to NULL, and then I check the in params. E.g.:

  if (!aNewContent) {
  *aNewContent = nsnull;
  if ((!aPresContext) || (!aRendContext)) {
Marking rtm+.
Whiteboard: [rtm need info] → [rtm+] a=waterson, r=erik
This bug is in candidate limbo.  We will reconsider this fix once we have a 
candidate in hand, but we can't take this fix before then.
Whiteboard: [rtm+] a=waterson, r=erik → [rtm+] a=waterson, r=erik [InLimbo-OOH]
We are moving toward the candidate.  Please check this fix into the trunk so we 
can get get some cook time.
fix checked into trunk
Priority: P3 → P1
PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.

Whiteboard: [rtm+] a=waterson, r=erik [InLimbo-OOH] → [rtm++] a=waterson, r=erik [InLimbo-OOH]
fix checked into branch
Closed: 24 years ago
Resolution: --- → FIXED
vrfy fixed in new branch and trunk builds on all platforms.
No longer crashes using 10-31-14 MN6 candidate build on Win98
I don't crash making a selection on a Mac build for ns6 (2000103114).
You need to log in before you can comment on or make changes to this bug.