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

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

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)

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.

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.

forgot the +

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.

Need review and super review. Marking [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 :(

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.

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

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

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.

a=waterson

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; }

Marking rtm+.

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.

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

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

fix checked into branch

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