Closed Bug 767169 Opened 12 years ago Closed 12 years ago

"Assertion failure: NS_SUCCEEDED(nsRange::CompareNodeToRange(lastCandidate, mRange, &nodeBefore, &nodeAfter))"

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: jruderman, Assigned: ayg)

References

(Depends on 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(6 files, 1 obsolete file)

testcase (asserts fatally when loaded)
474 bytes, text/html
Details
stack trace
13.45 KB, text/plain
Details
diff -w for patch part 1, v1
4.93 KB, patch
Details | Diff | Splinter Review
Patch part 2, v1 -- Handle lastCandidate being null in nsContentSubtreeIterator::Init
2.86 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Patch part 1, v2 -- Partial cleanup of Selection::selectFrames
6.42 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Use script runner for nsHTMLEditor::ResetRootElementAndEventTarget
3.59 KB, patch
ehsan.akhgari
: review+
bzbarsky
: review+
Details | Diff | Splinter Review 
Assertion failure: (((__builtin_expect(!!(!(( nsRange::CompareNodeToRange(lastCandidate, mRange, &nodeBefore, &nodeAfter)) & 0x80000000)), 1)))), at content/base/src/nsContentIterator.cpp:1295

nsRange::CompareNodeToRange returned NS_ERROR_UNEXPECTED, tripping the assertion:

MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
  nsRange::CompareNodeToRange(lastCandidate, mRange, &nodeBefore, &nodeAfter)));
Attached file stack trace — 

    
    

    
  
(Contrary to the comment in the test-case, this isn't actually a quirks vs. standards issue.  The same bug occurs if you have <!doctype html> but remove it before doing anything else in boom() -- it's just that there can't be a doctype node.)
Assignee: nobody → ayg
Status: NEW → ASSIGNED

    
    

    
  
One problem here is that we're passing a range to nsContentSubtreeIterator::Init whose end offset is greater than the length of its end node -- this is being called from nsNodeUtils::ContentRemoved, and the ranges haven't been adjusted yet.

I suspect this is very broken.  If a node has been removed and the end offset of the selection hasn't yet been adjusted, a bogus node will be selected here.  Selection::selectFrames will then do whatever it does to it.

Ehsan, do you have any insight into what the right behavior here is?


This might not be the only way to trigger the bug, though.  The issue is that lastCandidate is null in nsContentSubtreeIterator::Init.  This is because node is the document, so GetPrevSibling(node) is null, and that's what we set lastCandidate to.  So I'll write a patch to explicitly handle the !lastCandidate case by "MakeEmpty(); return NS_OK;", as is done for firstCandidate.

    
    

    
  


  
Possibly unrelated to the bug.  I say "partial" because I'm quite suspicious of the error-handling here in the case where various nodes aren't content -- I preserved existing behavior, but maybe that isn't what we actually want.  If non-content nodes weren't an error condition, we could make the function return void.  (Only one caller pays attention to the return code anyway.)

        Attachment #637155 -
        Flags: review?(ehsan)

    
    

    
  


  
The patch isn't very long, but it does do some outdenting.

    
  

        Attachment #637155 -
        Flags: review?(ehsan) → review+

    
    

    
  
(In reply to :Aryeh Gregor from comment #3)
> One problem here is that we're passing a range to
> nsContentSubtreeIterator::Init whose end offset is greater than the length
> of its end node -- this is being called from nsNodeUtils::ContentRemoved,
> and the ranges haven't been adjusted yet.
> 
> I suspect this is very broken.  If a node has been removed and the end
> offset of the selection hasn't yet been adjusted, a bogus node will be
> selected here.  Selection::selectFrames will then do whatever it does to it.
> 
> Ehsan, do you have any insight into what the right behavior here is?

Could you perhaps use a script blocker to defer this to until after the range has been updated?

    
    

    
  


  
This doesn't answer the question of "why the heck are we being passed a bogus range?", but even if the range is legit, I don't see any obvious reason why lastCandidate couldn't be null here, so we need the check anyway.  If we decide that nsContentSubtreeIterator shouldn't be legitimately passed invalid ranges, I can remove the newly-added comment and add an assertion to that effect to the start of Init().  The check I'm adding is paralleled by an existing check for firstCandidate:

  if (!firstCandidate) {
    // then firstCandidate is next node after node
    firstCandidate = GetNextSibling(node);

    if (!firstCandidate) {
      MakeEmpty();
      return NS_OK;
    }
  }

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> Could you perhaps use a script blocker to defer this to until after the
> range has been updated?

How would that help?  That stops JS scripts from running, right?  But the issue here is native code -- there are no mutation listeners or anything in the stack.  I don't know what the native code does, so I don't know if we want to defer it (or how to).  But maybe I don't understand what you're saying, or don't understand what script blockers do.

Try: https://tbpl.mozilla.org/?tree=Try&rev=7ecf5632e7d7

        Attachment #637430 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 637430 [details] [diff] [review]
Patch part 2, v1 -- Handle lastCandidate being null in nsContentSubtreeIterator::Init

There are test failures, so I'm deferring the review request until I figure them out.

        Attachment #637430 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 637430 [details] [diff] [review]
Patch part 2, v1 -- Handle lastCandidate being null in nsContentSubtreeIterator::Init

Okay, so the problem was actually with part 1.  I'm trying out a fix for that now:


diff --git a/layout/generic/nsSelection.cpp b/layout/generic/nsSelection.cpp
--- a/layout/generic/nsSelection.cpp
+++ b/layout/generic/nsSelection.cpp
@@ -80,17 +80,16 @@ static NS_DEFINE_CID(kFrameTraversalCID,
 #include "nsDOMError.h"
 #include "mozilla/dom/Element.h"

 using namespace mozilla;

 //#define DEBUG_TABLE 1

 static NS_DEFINE_IID(kCContentIteratorCID, NS_CONTENTITERATOR_CID);
-static NS_DEFINE_IID(kCSubtreeIteratorCID, NS_SUBTREEITERATOR_CID);

 //PROTOTYPES
 class nsFrameSelection;
 class nsAutoScrollTimer;

 static bool IsValidSelectionPoint(nsFrameSelection *aFrameSel, nsINode *aNode);

 static nsIAtom *GetTag(nsINode *aNode);
@@ -4025,17 +4024,17 @@ Selection::selectFrames(nsPresContext* a
       } else {
         endOffset = content->Length();
       }
       textFrame->SetSelectedRange(startOffset, endOffset, aFlags, mType);
     }
   }

   iter->First();
-  nsCOMPtr<nsIContentIterator> inneriter = NS_NewContentSubtreeIterator();
+  nsCOMPtr<nsIContentIterator> inneriter = NS_NewContentIterator();
   for (iter->First(); !iter->IsDone(); iter->Next()) {
     content = do_QueryInterface(iter->GetCurrentNode());
     SelectAllFramesForContent(inneriter, content, aFlags);
   }

   // We must now do the last one if it is not the same as the first
   if (aRange->GetEndParent() != aRange->GetStartParent()) {
     nsresult res;

But the second part should still be reviewable while I get the first part fixed up.

        Attachment #637430 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 637430 [details] [diff] [review]
Patch part 2, v1 -- Handle lastCandidate being null in nsContentSubtreeIterator::Init

r=me

        Attachment #637430 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
I thought we set a script blocker when ranges are being updated...  Boris, don't we do that?

    
    

    
  
I don't know, offhand.

    
    

    
  
Can you give this a shot, Aryeh?  (FWIW, I'm talking about nsContentUtils::AddScriptRunner and friends)

    
    

    
  


  
In my cleanup, I accidentally made a regular iterator into a subtree iterator, which understandably broke stuff.  Interdiff is in comment 9; green try run at <https://tbpl.mozilla.org/?tree=Try&rev=f4e252126e3c>.

(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> I thought we set a script blocker when ranges are being updated...  Boris,
> don't we do that?

(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Can you give this a shot, Aryeh?  (FWIW, I'm talking about
> nsContentUtils::AddScriptRunner and friends)

I'd be happy to look at this, but can you explain more clearly what I should do?  There's no user script running here on the stack above removeChild(); the assert is caused by nsHTMLEditor::ResetRootElementAndEventTarget resetting the selection.  Do you want me to wrap one of the function calls in nsContentUtils::AddScriptRunner so it doesn't run until the range is adjusted?  If so, which one?  I don't know which parts of this are safe to defer, because I don't know what else can happen before scripts can safely run.

        Attachment #637155 -
        Attachment is obsolete: true

        Attachment #637816 -
        Flags: review?(ehsan)

    
  

        Attachment #637816 -
        Flags: review?(ehsan) → review+

    
    

    
  
OK, here's how script blockers work.  When some piece of code wants to do something during which running scripts is not safe, it calls AddScriptBlocker (usually through allocating an nsAutoScriptBlocker RAII object on the stack), and when it's done it calls RemoveScriptBlocker.

Now, when you have native code that should not run during the period where a script blocker is active, you use AddScriptRunner to tell the script blocking mechanism about your code.  AddScriptRunner will run your code immediately if there is currently no active script blockers.  If there is, it will hold on to the runnable object and will run it once the last RemoveScriptBlocker is called (script blockers can be nested, and they're implemented using a counter.)

The reason I suggested this is that if we block scripts before the range is updated (which I suspect we do), then this will give you the ability to run your code after the ranges are updated.  It's sort of an abuse of "script" blockers, but it wouldn't be the first in Gecko.  :-)

    
    

    
  
That brings me back to:

(In reply to :Aryeh Gregor from comment #14)
> Do you want me to wrap one of the function calls
> in nsContentUtils::AddScriptRunner so it doesn't run until the range is
> adjusted?  If so, which one?  I don't know which parts of this are safe to
> defer, because I don't know what else can happen before scripts can safely
> run.

I don't want to find out that I deferred the execution of something in a way that causes other stuff to break because I didn't understand what I was doing.

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/61070cc08413
https://hg.mozilla.org/integration/mozilla-inbound/rev/3663b7886fdd
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla16

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
I think you should try to call ResetRootElementAndEventTarget itself through a script runner.  I don't think it will have bad effects, but we should of course test it on try.

    
    

    
  


  
Requesting review from Ehsan for editor/, Boris for content/.  The added MOZ_ASSERT in nsContentIterator.cpp causes the test from part 2 to crash without the editor/ change -- it should help catch future cases where we accidentally pass bogus ranges.

Try: https://tbpl.mozilla.org/?tree=Try&rev=cc21722b8de2

        Attachment #638356 -
        Flags: review?(ehsan)

        Attachment #638356 -
        Flags: review?(bzbarsky)

    
  

        Attachment #638356 -
        Flags: review?(ehsan) → review+

    
  
Depends on: 770580

    
    

    
  
Oops -- I pushed the last patch here without remembering that I asked for review from bz too:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f49e52ee86f3

I backed it out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/25bdbb0faa4a

    
    

    
  
Comment on attachment 638356 [details] [diff] [review]
Patch part 3, v1 -- Use script runner for nsHTMLEditor::ResetRootElementAndEventTarget

r=me

        Attachment #638356 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c8715136091b
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Depends on: 772387

    
  
Depends on: 784905

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: