56704 - Crash selecting text

Status: VERIFIED FIXED

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

Description

[Blake Ross]

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

Steps to Reproduce:
(1) Go to http://www.qazilla.org/crash.html
(2) Select text from top to bottom

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

Stack Trace:

ffffffff()
nsGenericHTMLContainerElement::ChildAt(int 0, nsIContent * & 0x00000000) line 
3333
nsHTMLUListElement::ChildAt(const nsHTMLUListElement * const 0x0145d038, int 0, 
nsIContent * & 0x00000000) line 65 + 22 bytes
nsContentIterator::GetDeepFirstChild(nsCOMPtr<nsIContent> {...}) line 403 + 49 
bytes
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 
bytes
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 
bytes
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 
4105
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()
007a8a32()

Comment 1

[Kathleen :Brade]

I see this on a debug Mac branch build from today.  Nominate for rtm.

Comment 2

[mjudge]

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)

Comment 3

[Simon Fraser [no longer active]]

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

Comment 4

[Peter Trudelle]

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.

Comment 5

[mjudge]

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.

Comment 6

[mjudge]

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.

Comment 7

[mjudge]

forgot the +

Comment 8

[Kathleen :Brade]

Here is the diff which Mike is proposing to fix this bug:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16222

I believe that this fix is already turned on for the trunk.

Comment 9

[Phil Peterson]

Need review and super review. Marking [rtm need info]

Comment 10

[mjudge]

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 :(

Comment 11

[kinmoz]

Attachment: Minimal test case that causes crash.

Comment 12

[kinmoz]

I just attatched the minimal test case that reproduces the crash:

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

It turns out that the assertions are caused by the <object> tags in the 
http://www.qazilla.org/crash.html page, but they aren't the source of the crash.

Comment 13

[buster]

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.

Comment 14

[buster]

Attachment: proposed patch, zero's the out param in the failure case so no addref is needed

Comment 15

[buster]

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.)

Comment 16

[Chris Waterson]

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...

Comment 17

[buster]

Attachment: more complete diff, incorporates waterson's suggestions and robert's comments

Comment 18

[buster]

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.

Comment 19

[Chris Waterson]

a=waterson

Comment 20

[Erik van der Poel]

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) {
    return NS_ERROR_NULL_POINTER;
  }
  *aNewContent = nsnull;
  if ((!aPresContext) || (!aRendContext)) {
    return NS_ERROR_NULL_POINTER;
  }

Comment 21

[karnaze (gone)]

Marking rtm+.

Comment 22

[selmer (gone)]

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.

Comment 23

[selmer (gone)]

We are moving toward the candidate.  Please check this fix into the trunk so we 
can get get some cook time.

Comment 24

[buster]

fix checked into trunk

Comment 25

[selmer (gone)]

PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.

Comment 26

[buster]

fix checked into branch

Comment 27

[Blake Ross]

vrfy fixed in new branch and trunk builds on all platforms.

Comment 28

[Peter Trudelle]

No longer crashes using 10-31-14 MN6 candidate build on Win98

Comment 29

[Kathleen :Brade]

I don't crash making a selection on a Mac build for ns6 (2000103114).