772387 - Make Ranges update their endpoints immediately on DOM mutations

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P5)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Bug 767169 and bug 771873 were both caused by code that dealt with ranges, which ran after DOM mutations had occurred but before the ranges were updated.  This is bad.  Bug 771873 wound up being a crash due to double-free, because a spellcheck Selection's list of ranges contained one range whose end was before its start!

I'm not sure how best to implement this, because I don't totally understand how the current system works.  Basically, I think script blockers stop mutation notifications from firing -- is that right?  If so, maybe we should implement the change here as a special type of mutation observer that will always be run synchronously before other observers, even if script is blocked.  Whatever the case may be, nothing should be happening between the actual DOM update and the range updates.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We can try, but see http://blogs.msdn.com/b/oldnewthing/archive/2011/03/10/10138969.aspx

The setup is the following.  Script blockers don't stop mutation notifications from firing.  But they _do_ stop script from running, sorta.

So the only way to make this work as you envision is to clearly define dependencies between observers and then run them in dependency order.  And pray for no loops in the dependency chain.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Yeah. Whoever depends on ranges, and is an nsIMutationObserver itself, should probably do its stuff
using a script runner.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Or, perhaps we need pre-script-runner phase, so that whenever scripts runs,
things are in a stable state.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> We can try, but see
> http://blogs.msdn.com/b/oldnewthing/archive/2011/03/10/10138969.aspx

That doesn't apply to things under our direct control.  We can say range updates happen before everything else.  Then if anything else needs to happen before range updates for whatever reason, which I don't think is likely, we can shuffle the order when it happens.

> So the only way to make this work as you envision is to clearly define
> dependencies between observers and then run them in dependency order.  And
> pray for no loops in the dependency chain.

I propose the following initial dependency among observers: range updates > everything else.  I hope you agree there are no loops there.  We know there are problems with some code that gets un-updated ranges, and have no reason to believe that range updates depend on anything else (right?).  So that scheme should work for now.

(In reply to Olli Pettay [:smaug] from comment #2)
> Yeah. Whoever depends on ranges, and is an nsIMutationObserver itself,
> should probably do its stuff
> using a script runner.

See bug 771873 -- using a script runner there caused a lot of spellcheck test failures.  It would have been fine without the script runner if only the range updates ran first.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Aryeh Gregor from comment #4)
> I propose the following initial dependency among observers: range updates >
> everything else. 
Sounds scary. For now we have had document-itself > everything else.

 
> See bug 771873 -- using a script runner there caused a lot of spellcheck
> test failures.  It would have been fine without the script runner if only
> the range updates ran first.
So, I think the pre-script-runner approach would be simpler and safer here.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

FWIW, I tried out a very simple patch:

diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h
--- a/content/base/public/nsINode.h
+++ b/content/base/public/nsINode.h
@@ -811,16 +807,32 @@ public:
   {
     nsSlots* s = GetSlots();
     if (s) {
       s->mMutationObservers.AppendElementUnlessExists(aMutationObserver);
     }
   }
 
   /**
+   * Same as AddMutationObserver, but adds the mutation observer to the start
+   * of the list instead of the end.  Don't use this unless you're sure you
+   * need it.
+   */
+  void AddPrioritizedMutationObserver(nsIMutationObserver* aMutationObserver)
+  {
+    nsSlots* s = GetSlots();
+    if (s) {
+      NS_ASSERTION(s->mMutationObservers.IndexOf(aMutationObserver) ==
+                   nsTArray<int>::NoIndex,
+                   "Observer already in the list");
+      s->mMutationObservers.InsertElementAt(0, aMutationObserver);
+    }
+  }
+
+  /**
    * Removes a mutation observer.
    */
   void RemoveMutationObserver(nsIMutationObserver* aMutationObserver)
   {
     nsSlots* s = GetExistingSlots();
     if (s) {
       s->mMutationObservers.RemoveElement(aMutationObserver);
     }
diff --git a/content/base/src/nsRange.cpp b/content/base/src/nsRange.cpp
--- a/content/base/src/nsRange.cpp
+++ b/content/base/src/nsRange.cpp
@@ -688,17 +688,17 @@ nsRange::DoSetRange(nsINode* aStartN, PR
                     aRoot->IsNodeOfType(nsINode::eCONTENT))),
                   "Bad root");
 
   if (mRoot != aRoot) {
     if (mRoot) {
       mRoot->RemoveMutationObserver(this);
     }
     if (aRoot) {
-      aRoot->AddMutationObserver(this);
+      aRoot->AddPrioritizedMutationObserver(this);
     }
   }
   bool checkCommonAncestor = (mStartParent != aStartN || mEndParent != aEndN) &&
                              IsInSelection() && !aNotInsertedYet;
   nsINode* oldCommonAncestor = checkCommonAncestor ? GetCommonAncestor() : nsnull;
   mStartParent = aStartN;
   mStartOffset = aStartOffset;
   mEndParent = aEndN;

But it didn't fix bug 771873.  I'm not sure why not.  Notifications happen from the start of the list to the end, right?

(In reply to Olli Pettay [:smaug] from comment #5)
> > See bug 771873 -- using a script runner there caused a lot of spellcheck
> > test failures.  It would have been fine without the script runner if only
> > the range updates ran first.
> So, I think the pre-script-runner approach would be simpler and safer here.

How would that work?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

You'd have a list of runnables (expected to *not* run scripts) which are handled just before script runners.
Basically in nsContentUtils::RemoveScriptBlocker() when sScriptBlockerCount drops to 0
you'd ++sScriptBlockerCount, run the pre-script-runners, --sScriptBlockerCount,
run script runners.
You'd also need nsContentUtils::AddPreScriptRunner to add something to the list.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

How would that fix the problem?  Are you saying that then things like spellcheck should be deferred to pre-script runners?  If so, why do you think that will work in general?  Ranges are used in a lot of code, and I'd expect there are some things that use them but can't be easily deferred.

As it stands, we're almost guaranteed to hit bugs in corner cases, which won't be noticeable because ranges will be silently bogus in most cases where the bug hits.  I'd really like a general solution here, of the form "Ranges are updated before (almost) anything else".

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Aryeh Gregor from comment #8)
> Are you saying that then things like
> spellcheck should be deferred to pre-script runners? 
Yes

> If so, why do you
> think that will work in general?
Because nsIMutationObserver methods would be called before pre-script-runners

>  Ranges are used in a lot of code,
So are nsIFrames etc which depend on nsIMutationObserver notifications

> and I'd
> expect there are some things that use them but can't be easily deferred.
Why not?

> As it stands, we're almost guaranteed to hit bugs in corner cases, which
> won't be noticeable because ranges will be silently bogus in most cases
> where the bug hits.  I'd really like a general solution here, of the form
> "Ranges are updated before (almost) anything else".
I really wish to not special case ranges, because then we might end up special casing other things 
too, like the notifications presshell gets. And I *think* that in this case special
casing might end up slowing things down.

Comment 10

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.