76472 - Assertion due to no ChildAt in SHistory

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

FreeBSD 4.1 Mozilla 20010417xx

WHen returning from this page (it's an error page I hit when trying to go to the
reported url for bug 65643) to the first testcase from bug 65643, I got this
assertion.  Those two pages were the only ones visited since I started Mozilla,
though I had tried retyping the URL from bug 65643 to make sure I'd gone to the
right place.  I hit back, and got this assertion.

As this code from nsSHistory.cpp shows, there's an unhandled case here.

    prevContainer->GetChildCount(&pcnt);
    nextContainer->GetChildCount(&ncnt);
    dsTreeNode->GetChildCount(&dsCount);

    //XXX What to do if the children count don't match
    
    for (PRInt32 i=0; i<ncnt; i++){
	  nsCOMPtr<nsISHEntry> pChild, nChild;
      nsCOMPtr<nsIDocShellTreeItem> dsTreeItemChild;
	  
      prevContainer->GetChildAt(i, getter_AddRefs(pChild));
	  nextContainer->GetChildAt(i, getter_AddRefs(nChild));
	  dsTreeNode->GetChildAt(i, getter_AddRefs(dsTreeItemChild));


In this case, ncnt & pcnt are both 2, but dsCount is 0.  We hit the assertion
when doing dsTreeNode->GetChildAt() (of course).  I'm guessing (maybe
incorrectly!) that it should limit the for() to the min of the 3 counts - or
maybe punt and return FALSE?

GDB trace to be attached.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: gdb backtrace

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Note: The sequence was the testcase, the URL (at www.technotes.net), and then
that www.technotes.net url again, then Back.

Comment 3

[Radha on family leave (not reading bugmail)]

Is this a recent regression?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

I believe so, but I'm far from certain.  I don't have any older versions to test
with.

Comment 5

[Claudius Gayle]

Is there anything to see here in an opt build for this assertion? Errors to the console, anything?
I have dozens of daily builds to check with if you give me something to look for, and
precise steps to repro.

Comment 6

[Radha on family leave (not reading bugmail)]

I visited the sites listed here few times and  did back and forward and don't 
see the assertion. I think I got the sequence wrong. Can you clearly give me the  
steps to reproduce. Things that I tried in yesterday's build, include

1) visit the url attached to bug 65643 (a www.technote.net url, but redirects me 
to wwwlostgarlic.com/404a.html)
2) visit the first testcase attached to bug 65643 (a bugzilla attachement url)
3) Do back and forward among them

1) visit the url attached to bug 65643 (a www.technote.net url, but redirects me 
to wwwlostgarlic.com/404a.html)
2) Visit www.technotes.net 
3) Do back and forward among them

1) visit the url attached to bug 65643 (a www.technote.net url, but redirects me 
to wwwlostgarlic.com/404a.html)
2) visit http://wwwlost.garlic.com
3) Do back and forward.

Anyways I think putting a test on dsCount before calling 
dsTreeNore->GetChildAt() should fix the problem

Comment 7

[Radha on family leave (not reading bugmail)]

Attachment: patch to docshell

Comment 8

[Radha on family leave (not reading bugmail)]

That is actually a patch to nsSHistory.cpp. The actual patch is a one-liner. The
rest are for tab mis-alignment, that were already present. The patch attached to
bug 68847 somewhat prevents the situation  in which this bug can bug(in reload
cases)  The patch attached here will prevent the assertion.

Comment 9

[Adam Lock]

r=adamlock

Comment 10

[rpotts (gone)]

Perhaps a more inclusive fix would be:

if ((ncnt != pcnt) || (ncnt!= dsCount)) {
  return PR_FALSE;
}

Is there ever a case where we would want to return a result if the counts did 
not match?

-- rick

Comment 11

[Judson Valeski]

r=valeski

Comment 12

[Radha on family leave (not reading bugmail)]

Rick, your suggestion could work, but I don't want to add more rigid conditions 
until I test it out thoroughly. That part of code is the crux of subframe 
navigation. Right now ncnt drives the loop, which I think is right.  I think 
adding similar test conditions for valid ncnt and pcnt could avoid assertions in  
nsSHEntry.cpp

Comment 13

[Radha on family leave (not reading bugmail)]

I think ncnt and pcnt can be different when traversing between pages created by 
document.write() or some JS creating a iframe dynamically. 

Comment 14

[Radha on family leave (not reading bugmail)]

Attachment: patch to nsSHistory.cpp and nsSHEntry.cpp

Comment 15

[rpotts (gone)]

Radha,
I think that this patch looks better - since it seems to cover more cases...
sr=rpotts

One thing, though...  It looks like GetChildAt(...) needs to initialize aResult
to NULL, to prevent uninitialized memory from being returned if the index is out
of range...

In the old version of GetChildAt(...), if the index was out of range, the method
would return a NULL SHEntry and NS_OK...

In this version, if the index is out of range, NS_ERROR_FAILURE will be
returned, but the SHEntry is not cleared.

Since, the callers of GetChildAt(...) do not check the return code, bad things
could happen if the method fails :-)

-- rick

 nsSHEntry::GetChildAt(PRInt32 aIndex, nsISHEntry ** aResult)
 {
 	NS_ENSURE_ARG_POINTER(aResult);
-    if (PRUint32(aIndex) >= PRUint32(mChildren.Count())) {
-      *aResult = nsnull;
-
}
-    else {
-      *aResult = (nsISHEntry*) mChildren.ElementAt(aIndex);
-      NS_IF_ADDREF(*aResult);
-
}
-    return NS_OK;

  *aResult = nsnull;  <- null out to preserve original behavior

+  NS_ENSURE_ARG_RANGE(aIndex, 0, mChildren.Count() - 1);
+
+  *aResult = (nsISHEntry*) mChildren.ElementAt(aIndex);
+  NS_IF_ADDREF(*aResult);
+  return NS_OK;
 }

Comment 16

[Radha on family leave (not reading bugmail)]

Rick,
I noticed that too. Shall post something soon.
Radha

Comment 17

[Radha on family leave (not reading bugmail)]

Rick,
I noticed that too. Shall post something soon.
Radha

Comment 18

[Radha on family leave (not reading bugmail)]

fix  checked in. 

Comment 19

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]