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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: Jesse Ruderman, Assigned: ayg)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla16
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 634704 [details]
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
(Reporter)

Comment 1

5 years ago
Created attachment 634705 [details]
stack trace+
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?
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?
(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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86_64 → All
smaug pointed out the existence of nsMutationGuard to me.  That might do what we want -- let me try.
Created attachment 637828 [details] [diff] [review]
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
Attachment #637828 - Flags: review?(bugs)
Comment on attachment 637828 [details] [diff] [review]
Patch v1, using nsMutationGuard

Nope, test failures.  Need to debug.
Attachment #637828 - Flags: review?(bugs)
Created attachment 638212 [details] [diff] [review]
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.
Attachment #637828 - Attachment is obsolete: true
Attachment #638212 - Flags: review?(bugs)

Comment 9

5 years ago
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.
Attachment #638212 - Flags: review?(bugs) → review-
Created attachment 639359 [details] [diff] [review]
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
Attachment #638212 - Attachment is obsolete: true
Attachment #639359 - Flags: review?(bugs)
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.
Attachment #639359 - Flags: review?(bugs) → review+
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/510602478d52
Flags: in-testsuite+
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/510602478d52
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Depends on: 771639
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core

Updated

11 months ago
Depends on: 1274806
You need to log in before you can comment on or make changes to this bug.