766426 - "ASSERTION: aNode isn't in mRange, or something else weird happened" with mutation events, extractContents

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: aNode isn't in mRange, or something else weird happened: 'NS_SUCCEEDED(res) && !nodeBefore && !nodeAfter', file content/base/src/nsContentIterator.cpp, line 1431

Comment 1

[Jesse Ruderman]

Attachment: stack trace+

Comment 2

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

So I poked at this for a while.  I'm not *totally* sure, but I think what's happening here is pretty simple.  We have three calls of nsRange::CutContents that are nested thanks to mutation events.  (Thanks, mutation events!)  So basically, one call sets up an nsContentSubtreeIterator, then the next one gets called and messes with the DOM.  We return to the previous one, and call Prev() on it -- when mCurNode is now detached.  Obviously this causes Bad Stuff(TM).

I think what's broken here is mutation events.  The assertion here is quite correct, and it's perfectly valid for it to trigger given the messed-up situation created by mutation events.  But we have no sane fix for this kind of thing as long as user scripts can sneak in and make arbitrary DOM changes in the middle of execution.  On the other hand, I don't want to leave a known-incorrect NS_ASSERTION in the codebase.

Boris, what do you think?  Do you see any sane way to handle this kind of thing before the glorious day when we can remove mutation events for good?  If not, what should we do with the assert?

Comment 3

[Boris Zbarsky [:bzbarsky]]

So what's the upshot of the assert?  Something actually bad, or just that the range code has no sane way to behave and bails?

Is it possible to re-verify that stuff is in the range after calling mutators?

Comment 4

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

(In reply to Boris Zbarsky (:bz) from comment #3)
> So what's the upshot of the assert?  Something actually bad, or just that
> the range code has no sane way to behave and bails?

There's nothing sane to do, given the current implementation.  Our position in the range was lost, so we can't continue iterating..

> Is it possible to re-verify that stuff is in the range after calling
> mutators?

So thinking about it, I see three ways to proceed:

1) Don't try to handle it -- mutation events are pathological.  Just return an error.  When we get rid of mutation events, reinstate the assertion.  Behavior is undefined if mutation event handlers mutate the DOM, so we're justified in throwing an exception back to JS.

2) Make the caller handle it.  Have extractContents() run through the whole iterator and save a static list of nodes to clone/remove, and then iterate through the list separately.  This could use more memory for ranges with lots of nodes.

3) Handle it in the iterator.  Don't store any actual nodes.  Clone the input range and save it, and save a separate range whose start and end are fixed at (mCurNode, 0).  Use the first range in place of mFirst/mLast, and the second range in place of mCurNode.  Range mutation (gravity) rules mean that DOM mutations won't cause anything to become invalid -- the second range will always be collapsed somewhere within the first range, and insertions and deletions will be handled in the most sensible way possible.  This might take a lot more processing, though, because we'd effectively have to recompute mFirst/mLast on every call to Prev/Next.


I think (1) is the way to go.  If people modify the DOM from a mutation event, they deserve whatever they get.  Unfortunately, removing the assertion does mean that we won't easily be able to detect bugs in our own code.  I've filed bug 769208 to back out this bug once we get rid of mutation events.

Comment 5

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

smaug pointed out the existence of nsMutationGuard to me.  That might do what we want -- let me try.

Comment 6

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

Attachment: Patch v1, using nsMutationGuard

Thanks for the nsMutationGuard tip!  This returns NS_ERROR_UNEXPECTED if there are unwanted mutations, without having to change behavior in the absence of mutation events.  It fixes this test-case, and I think I got every possible mutation point in CutContents(), so it hopefully won't be possible to mess it up through any other means.

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

Comment 7

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

Comment on attachment 637828 [details] [diff] [review]
Patch v1, using nsMutationGuard

Nope, test failures.  Need to debug.

Comment 8

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

Attachment: Patch v2, test failures fixed

Green try: https://tbpl.mozilla.org/?tree=Try&rev=5b6297bb3b0b

I'm not totally sure all the Mutated() arguments are right, but I'm sure Jesse's diligent fuzzing will tell us if they aren't.

Comment 9

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

Comment on attachment 638212 [details] [diff] [review]
Patch v2, test failures fixed

Unfortunately I think we need to do something more complex.
If unexpected mutations are happened, check whether we can continue
CutContents.

Comment 10

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

Attachment: Patch v3, only returns an error if the current node is actually not in the range

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

Comment 11

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

Comment on attachment 639359 [details] [diff] [review]
Patch v3, only returns an error if the current node is actually not in the range

>+// Helper function for CutContents, making sure that the current node wasn't
>+// removed by mutation events (bug 766426)
>+static bool
>+ValidateCurrentNode(nsRange* aRange, RangeSubtreeIterator& aIter)
>+{
>+  bool before, after;
>+  nsCOMPtr<nsIDOMNode> domNode = aIter.GetCurrentNode();
>+  nsCOMPtr<nsINode> node = do_QueryInterface(domNode);
>+  MOZ_ASSERT(node);
>+
>+  nsresult res = nsRange::CompareNodeToRange(node, aRange, &before, &after);
>+
>+  return NS_SUCCEEDED(res) && !before && !after;
>+}

We may need to tweak this. Add more restrictions, but ok for now.

Comment 12

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

Yes -- I think we also need to check that mFirst is still in the range.  Otherwise you could get the iterator to run backwards up to the document node, and it will probably hit something else that's outside the range.  But this fixes the specific case.  If we also check that mFirst is in the range, I think we'll be fine, because then all intervening nodes will have to be in the range too, so things should work as expected.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/510602478d52

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/510602478d52