Status

()

Core
Selection
P1
critical
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Blake Ross, Assigned: buster)

Tracking

({crash})

Trunk
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

18 years ago
Keywords: crash

Comment 1

18 years ago
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

Comment 2

18 years ago
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)
Status: NEW → ASSIGNED
Whiteboard: [rtm need info] → [rtm+]

Comment 3

18 years ago
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

18 years ago
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]

Comment 5

18 years ago
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

18 years ago
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

Comment 7

18 years ago
forgot the +
Keywords: rtm
Whiteboard: [rtm need info] → [rtm+]

Comment 8

18 years ago
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.
Keywords: rtm

Comment 9

18 years ago
Need review and super review. Marking [rtm need info]
Whiteboard: [rtm+] → [rtm need info]

Comment 10

18 years ago
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
Status: ASSIGNED → NEW

Comment 11

18 years ago
Created attachment 17657 [details]
Minimal test case that causes crash.

Comment 12

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

Comment 13

18 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 14

18 years ago
Created attachment 17854 [details] [diff] [review]
proposed patch, zero's the out param in the failure case so no addref is needed
(Assignee)

Comment 15

18 years ago
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

Comment 16

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

Comment 17

18 years ago
Created attachment 17872 [details] [diff] [review]
more complete diff, incorporates waterson's suggestions and robert's comments
(Assignee)

Comment 18

18 years ago
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

18 years ago
a=waterson

Comment 20

18 years ago
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

18 years ago
Marking rtm+.
Whiteboard: [rtm need info] → [rtm+] a=waterson, r=erik

Comment 22

18 years ago
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.

Updated

18 years ago
Whiteboard: [rtm+] a=waterson, r=erik → [rtm+] a=waterson, r=erik [InLimbo-OOH]

Comment 23

18 years ago
We are moving toward the candidate.  Please check this fix into the trunk so we 
can get get some cook time.
(Assignee)

Comment 24

18 years ago
fix checked into trunk
Priority: P3 → P1

Comment 25

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

Comment 26

18 years ago
fix checked into branch
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Reporter)

Comment 27

18 years ago
vrfy fixed in new branch and trunk builds on all platforms.
Status: RESOLVED → VERIFIED

Comment 28

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

Comment 29

18 years ago
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.