358660 - [FIX]Error console evaluator no longer works

Status: RESOLVED FIXED

(Toolkit Graveyard :: Error Console, defect, P2)

Description

[:Gavin Sharp [email: [e-mail]]]

STR:
1) Open Error Console
2) Type 0 into field
3) Click Evaluate

Expected: "0" displayed in the console.
Actual: nothing displayed in the console.

Evaluating alert(0) alerts, so the script is being run, but the result isn't being shown.

This sounds like a regression from bug 351633: the load event is probably fired before the script is evaluated, so the console's displayResult() is likely called before there is anything to display.

This applies to both error console implementations (toolkit and XPFE), not surprisingly.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So this regressed between 2006-10-20-01 and 2006-10-21-02.  So bug 351633 is off the hook.

The range is:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-10-20+01&maxdate=2006-10-21+02&cvsroot=%2Fcvsroot

and I bet the problem is that selectNode or toString are doing something weird here, and that this is a regression from bug 357445.

Comment 2

[[e-mail]]

I can verify that switching to selectNodeContents works around the bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the problem is that bug 357445 fixed selectNode for the case when the node is the documentElement to select the right thing.  But the toString code still doesn't handle this case, so this code in the console JS:

  130   var resultRange = Evaluator.document.createRange();
  131   resultRange.selectNode(Evaluator.document.documentElement);
  132   var result = resultRange.toString();

started returning the empty string.  Sadly, nsContentIterator seems to not handle this case well.... :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 5

[Boris Zbarsky [:bzbarsky]]

So that does fix this bug...  I _think_ the ickyness in nsContentIterator is correct.  Also, in the interests of sanity I did not attempt to clean up the rest of nsRange::ToString...

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 244015 [details] [diff] [review]
Fix

>Index: content/base/src/nsContentIterator.cpp

>@@ -415,122 +407,156 @@ nsContentIterator::Init(nsIDOMRange* aRa
...
>   if (!cChild) // no children, must be a text node
>   {
>+    // XXXbz no children might also just mean no children.  So I'm not
>+    // sure what that comment above is talking about.

I'd say remove the above comment. It's better to have no comment than a lying one.

>+    else {
>+      // post-order
>+      if (startNode->IsNodeOfType(nsINode::eCONTENT)) {
>+        mFirst = NS_STATIC_CAST(nsIContent*, startNode);
>+      } else {
>+        // What else can we do?
>+        mFirst = GetNextSibling(startNode, nsnull);
>       }

The GetNextSibling call seems useless, neither documents or attributes ever has any siblings.

Isn't GetFirstDeepChild(startNode, nsnull) what you want?


>   // Find last node in range.
> 
>-  PRBool endIsData = endCon->IsNodeOfType(nsINode::eDATA_NODE);
>+  PRBool endIsData = endNode->IsNodeOfType(nsINode::eDATA_NODE);
> 
>-  if (endIsData || !ContentHasChildren(endCon) || endIndx == 0)
>+  if (endIsData || !NodeHasChildren(endNode) || endIndx == 0)
>   {
>-    if (mPre)
>-      mLast = endCon;
>+    if (mPre) {
>+      if (endNode->IsNodeOfType(nsINode::eCONTENT)) {
>+        mLast = NS_STATIC_CAST(nsIContent*, endNode);
>+      } else {
>+        // Not much else to do here...
>+        mLast = GetDeepFirstChild(GetPrevSibling(endNode, nsnull), nsnull);

Again, this doesn't seem right since it'll always be null. Don't you want something like:

mLast = endNode->GetChildAt(endNode->GetChildCount() - 1);

(which may or may not still be nsnull).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> The GetNextSibling call seems useless

Hmm.... yeah.  I guess it does always return null.  If/when we convert this whole file to nsINode, we should just set mFirst to startNode, I'd say....

> Isn't GetFirstDeepChild(startNode, nsnull) what you want?

We're in the !cChild branch, so we know startNode has no kids.  I suppose I should just explicitly set mFirst to null here.

> Again, this doesn't seem right since it'll always be null.

Ineed.  :(  I'm not actually sure what mLast should be in this case...  Are there any docs for what this code is trying to do?  :( I'm sadly not in a position to try to figure it out by reading the code for a good long while.  :(

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Setting to null explicitly in both places seems like what we want to do since in neither case there are enough children.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I wanted this in tomorrows nightlies, so I took the liberty of checking this in. Hope that was ok?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This broke the tree pretty badly.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, that was other cruft in my tree. It's been backed out.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yep, fixed.  Thanks for landing this, sicking!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in test to mochitest.