All users were logged out of Bugzilla on October 13th, 2018

[FIX]Error console evaluator no longer works

RESOLVED FIXED in mozilla1.9alpha1

Status

P2
normal
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: Gavin, Assigned: bzbarsky)

Tracking

({regression})

Trunk
mozilla1.9alpha1
regression
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 1

12 years ago
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.
Blocks: 357445
No longer blocks: 351633
Flags: blocking1.9?

Comment 2

12 years ago
I can verify that switching to selectNodeContents works around the bug.
(Assignee)

Comment 3

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

Comment 4

12 years ago
Created attachment 244015 [details] [diff] [review]
Fix
Assignee: js-console → bzbarsky
Status: NEW → ASSIGNED
Attachment #244015 - Flags: superreview?(bugmail)
Attachment #244015 - Flags: review?(bugmail)
(Assignee)

Comment 5

12 years ago
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...
Priority: -- → P2
Summary: Error console evalutor no longer works → [FIX]Error console evalutor no longer works
Target Milestone: --- → mozilla1.9alpha
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).
(Assignee)

Comment 7

12 years ago
> 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.  :(
Setting to null explicitly in both places seems like what we want to do since in neither case there are enough children.
(Assignee)

Comment 9

12 years ago
Created attachment 244380 [details] [diff] [review]
Updated to comments
Attachment #244015 - Attachment is obsolete: true
Attachment #244380 - Flags: superreview?(bugmail)
Attachment #244380 - Flags: review?(bugmail)
Attachment #244015 - Flags: superreview?(bugmail)
Attachment #244015 - Flags: review?(bugmail)
Attachment #244380 - Flags: superreview?(bugmail)
Attachment #244380 - Flags: superreview+
Attachment #244380 - Flags: review?(bugmail)
Attachment #244380 - Flags: review+
I wanted this in tomorrows nightlies, so I took the liberty of checking this in. Hope that was ok?
This broke the tree pretty badly.
Sorry, that was other cruft in my tree. It's been backed out.
(Assignee)

Comment 13

12 years ago
Yep, fixed.  Thanks for landing this, sicking!
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Summary: [FIX]Error console evalutor no longer works → [FIX]Error console evaluator no longer works
(Assignee)

Updated

12 years ago
Flags: blocking1.9?
(Assignee)

Comment 14

12 years ago
Checked in test to mochitest.
Flags: in-testsuite+
Product: Core → SeaMonkey

Updated

10 years ago
Component: Error Console → Error Console
Product: SeaMonkey → Toolkit
QA Contact: error-console → error.console
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.