Closed
Bug 766426
Opened 12 years ago
Closed 12 years ago
"ASSERTION: aNode isn't in mRange, or something else weird happened" with mutation events, extractContents
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: jruderman, Assigned: ayg)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files, 2 obsolete files)
###!!! 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•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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?
Assignee | ||
Comment 4•12 years ago
|
||
(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
Assignee | ||
Comment 5•12 years ago
|
||
smaug pointed out the existence of nsMutationGuard to me. That might do what we want -- let me try.
Assignee | ||
Comment 6•12 years ago
|
||
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)
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 637828 [details] [diff] [review]
Patch v1, using nsMutationGuard
Nope, test failures. Need to debug.
Attachment #637828 -
Flags: review?(bugs)
Assignee | ||
Comment 8•12 years ago
|
||
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•12 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-
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #638212 -
Attachment is obsolete: true
Attachment #639359 -
Flags: review?(bugs)
Comment 11•12 years ago
|
||
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+
Assignee | ||
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
Flags: in-testsuite+
Target Milestone: --- → mozilla16
Comment 14•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•