54825 - Infinite loop in nsTextServicesDocument; doing Find in Page.

Status: VERIFIED FIXED

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

Description

[John Morrison]

Overview Description:

  I hit an infinite loop while doing a Find on Page, using the PR3
  build 2000092908 on win2k. I broke in the debugger for a trunk build
  pulled ~2am 09/30, and got the stack trace below.

Steps to Reproduce:

1) Load the URL
http://support.microsoft.com/support/kb/articles/Q246/3/46.ASP?LN=EN-US&SD=gn
&FR=0

2) Do 'Search->Find on This Page' from the menu, and enter the text "drop"
   in the textfield.

3) a. Click on the 'Find' button       --> first "drop" is found
   b. Click on the 'Find' button again --> second "drop" is found
   c. Click on the 'Find' button again --> mozilla hangs

Actual Results:   hangs
Expected Results: user feedback that no more matches can be found.

Reproducibility: 100% win2k

Build Date & Platform Bug Found:
  2000092908 win2k; and on a current debug trunk mozilla build
  2000092909 linux rh 6.1
  2000092812 mac 9.0


Here is the stack when I broke in the loop.

nsTextServicesDocument::FirstTextNodeInNextBlock(nsIContentIterator *
0x032c2600) line 4059
nsTextServicesDocument::GetFirstTextNodeInNextBlock(nsIContent * * 0x032dddd4)
line 4146 + 20 bytes
nsTextServicesDocument::NextBlock(nsTextServicesDocument * const 0x032dddb8)
line 1710 + 35 bytes
nsFindComponent::Context::DoFind(nsFindComponent::Context * const 0x02e36c48,
int * 0x0012dcc0) line 509
nsFindComponent::FindNext(nsFindComponent * const 0x02e36ae8, nsISupports *
0x02e36c48, int * 0x0012dcc0) line 884
XPTC_InvokeByIndex(nsISupports * 0x02e36ae8, unsigned int 8, unsigned int 2,
nsXPTCVariant * 0x0012dcb0) line 139
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext * 0x0321fdf8,
nsXPCWrappedNative * 0x03220b98, const XPCNativeMemberDescriptor * 0x03220864,
nsXPCWrappedNativeClass::CallMode CALL_METHOD, unsigned int 1, long *
0x032c2980, long * 0x0012de64) line 913 + 43 bytes
WrappedNative_CallMethod(JSContext * 0x0321fdf8, JSObject * 0x03153540, unsigned
int 1, long * 0x032c2980, long * 0x0012de64) line 228 + 34 bytes
js_Invoke(JSContext * 0x0321fdf8, unsigned int 1, unsigned int 0) line 820 + 23
bytes
js_Interpret(JSContext * 0x0321fdf8, long * 0x0012e998) line 2621 + 15 bytes
js_Invoke(JSContext * 0x0321fdf8, unsigned int 1, unsigned int 2) line 837 + 13
bytes
js_InternalInvoke(JSContext * 0x0321fdf8, JSObject * 0x031534b8, long 51722640,
unsigned int 0, unsigned int 1, long * 0x0012eb30, long * 0x0012eac0) line 909 +
20 bytes
JS_CallFunctionValue(JSContext * 0x0321fdf8, JSObject * 0x031534b8, long
51722640, unsigned int 1, long * 0x0012eb30, long * 0x0012eac0) line 3193 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x0321f7e0, void * 0x031534b8,
void * 0x03153990, unsigned int 1, void * 0x0012eb30, int * 0x0012eb2c, int 0)
line 907 + 33 bytes
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x032c0f9c) line 154 + 64 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x0325cce8,
nsIDOMEvent * 0x032c0f9c, nsIDOMEventTarget * 0x032567d8, unsigned int 4,
unsigned int 7) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x032681f8, nsEvent *
0x0012f410, nsIDOMEvent * * 0x0012f32c, nsIDOMEventTarget * 0x032567d8, unsigned
int 7, nsEventStatus * 0x0012f730) line 935 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x032567d0, nsIPresContext *
0x032681f8, nsEvent * 0x0012f410, nsIDOMEvent * * 0x0012f32c, unsigned int 1,
nsEventStatus * 0x0012f730) line 3321
PresShell::HandleEventInternal(nsEvent * 0x0012f410, nsIView * 0x00000000,
unsigned int 1, nsEventStatus * 0x0012f730) line 4255 + 47 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x03268d50, nsEvent *
0x0012f410, nsIFrame * 0x0329c408, nsIContent * 0x032567d0, unsigned int 1,
nsEventStatus * 0x0012f730) line 4236 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const
0x032964a0, nsIPresContext * 0x032681f8, nsMouseEvent * 0x0012f840,
nsEventStatus * 0x0012f730) line 1854 + 61 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x032964a8,
nsIPresContext * 0x032681f8, nsEvent * 0x0012f840, nsIFrame * 0x0329c408,
nsEventStatus * 0x0012f730, nsIView * 0x03268908) line 935 + 28 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f840, nsIView * 0x03268908,
unsigned int 1, nsEventStatus * 0x0012f730) line 4275 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x03268d54, nsIView * 0x03268908,
nsGUIEvent * 0x0012f840, nsEventStatus * 0x0012f730, int 1, int & 1) line 4190 +
25 bytes
nsView::HandleEvent(nsView * const 0x03268908, nsGUIEvent * 0x0012f840, unsigned
int 28, nsEventStatus * 0x0012f730, int 1, int & 1) line 379
nsViewManager2::DispatchEvent(nsViewManager2 * const 0x032686f0, nsGUIEvent *
0x0012f840, nsEventStatus * 0x0012f730) line 1439
HandleEvent(nsGUIEvent * 0x0012f840) line 68
nsWindow::DispatchEvent(nsWindow * const 0x032689a4, nsGUIEvent * 0x0012f840,
nsEventStatus & nsEventStatus_eIgnore) line 681 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f840) line 702
nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 3890 +
21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4100
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 8192086, long *
0x0012fbbc) line 2960 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x00190212, unsigned int 514, unsigned int 0, long
8192086) line 950 + 27 bytes
USER32! 77e13eb0()
USER32! 77e1401a()
USER32! 77e192da()
nsAppShellService::Run(nsAppShellService * const 0x00c1d528) line 408
main1(int 1, char * * 0x00497178, nsISupports * 0x00000000) line 1004 + 32 bytes
main(int 1, char * * 0x00497178) line 1185 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

Comment 1

[John Morrison]

Nominating for RTM

Comment 2

[John Morrison]

Here is a minimal test case. It seems to get hung up on the SCRIPT inside the FORM
(inside a P?).

<HTML>
<BODY>
  <p>drop drop</p>
  <FORM>
    <P>Do find in page for "foo" three times; you will hang on the third try.
      <SCRIPT LANGUAGE="JavaScript">
      <!--//
        var foo;
      //-->
      </SCRIPT>
  </FORM>
</BODY>
</HTML>

Comment 3

[John Morrison]

Er, should be 'Do find in page for "drop" three times ...'

Comment 4

[rubydoo123]

kin, how difficult is this to fix? what is the risk in doing so?

Comment 5

[rubydoo123]

m19

Comment 6

[kinmoz]

Cc: [e-mail] and [e-mail].

This should probably be assigned to one of them.

With John's minimal test case, the infinite loop is happening because it appears 
that the form content node appears twice in the body's child list at position 3 
and 4.

If the form content node is the current node in the content iterator, when the 
TextServices module calls nsIContentIterator::Next(), it eventually calls 
nsContentIterator::NextNode() which calls parent->IndexOf(), where the 
form's parent is actually the body, which returns 3. It then increments the 
index to 4 and calls parent->ChildAt(4) to retrieve the next child from the 
list, which just so happens to be the same form node. The next time the 
TextServices module calls nsIContentIterator::NextNode(), parent->IndexOf() will 
return 3 because it expects the first occurence to be the only occurence of a 
node in the child list so the whole cycle starts over again.

I think there may be some sort of race condition going on here ... if I put 
break points to try and debug what is actually causing 2 occurences of the same 
element to be put in the same child list, it never happens ... but if I remove 
all my break points and run, I can get the bug to happen.

Comment 7

[kinmoz]

I just verified that the same thing is happening with the URL

http://support.microsoft.com/support/kb/articles/Q246/3/46.ASP?LN=EN-US&SD=gn&FR
=0

except that I'm seeing a different node (a table cell) that appears twice in 
it's parent's list.

Reassigning to [e-mail]. Clearing status and Target Milestone. Leaving 
nomination for rtm since this looks like it's real important to fix.

Comment 8

[kinmoz]

This should be a P1.

Comment 9

[Nisheeth Ranjan]

Agreed that this is a P1.  Marking [rtm+ need info]

Comment 10

[Johnny Stenback (:jst)]

Attachment: Proposed fix

Comment 11

[Johnny Stenback (:jst)]

The patch I just attached does fix the problem but I doubt the patch fixes the
real problem that is causing this. The problem is that the document contains a
form that is not properly closed (like most forms on the web) and the cleanup
code in the sink that deals with this ends up adding the form to its parent even
if the form is already a child of the parent.

I think the patch I attached would be a good candidate for rtm, but again, I
don't think it fixes the real problem. I'll investigate further...

Comment 12

[kinmoz]

Johnny, I'm a little confused by your previous comments. What do you mean the
form is not properly closed? John's simplified example contains a <form> tag
that has a matching </form> tag.

By the way, bug #49041 (hang while selecting text) is also related to this same
duplication of a form node in it's parent child list problem.

The common thing between this bug and bug #49041 is the use of JavaScript and a
form element.

Comment 13

[Johnny Stenback (:jst)]

Ok, so the problem really that the form itself is not properly closed, but some
content inside the form is not properly closed (in the simple case it's the <P>)
and that's what triggers HTMLContentSink::DemoteContainer() which triggers the
bug. I tried reproducing the problem in bug 49041 with my changes and so far it
looks like that problem is fixed by this change too.

I'll attach a slightly more efficient version (as suggested by Vidur) of the
current patch in a while...

Comment 14

[Johnny Stenback (:jst)]

Attachment: Slightly faster fix

Comment 15

[Johnny Stenback (:jst)]

Attachment: One more time, faster, better and smaller fix

Comment 16

[selmer (gone)]

rtm++

Comment 17

[Johnny Stenback (:jst)]

Fix checked in on both branch and trunk.

Comment 18

[sujay]

verified in 10/13 build.