Last Comment Bug 766426 - "ASSERTION: aNode isn't in mRange, or something else weird happened" with mutation events, extractContents
: "ASSERTION: aNode isn't in mRange, or something else weird happened" with mut...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 771639 1274806
Blocks: 325861 336383
  Show dependency treegraph
 
Reported: 2012-06-19 19:06 PDT by Jesse Ruderman
Modified: 2016-06-25 13:42 PDT (History)
4 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (577 bytes, text/html)
2012-06-19 19:06 PDT, Jesse Ruderman
no flags Details
stack trace+ (16.96 KB, text/plain)
2012-06-19 19:07 PDT, Jesse Ruderman
no flags Details
Patch v1, using nsMutationGuard (4.54 KB, patch)
2012-06-29 03:30 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Splinter Review
Patch v2, test failures fixed (4.64 KB, patch)
2012-07-01 12:19 PDT, :Aryeh Gregor (away until August 15)
bugs: review-
Details | Diff | Splinter Review
Patch v3, only returns an error if the current node is actually not in the range (5.87 KB, patch)
2012-07-05 08:44 PDT, :Aryeh Gregor (away until August 15)
bugs: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-19 19:06:56 PDT
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
Comment 1 Jesse Ruderman 2012-06-19 19:07:12 PDT
Created attachment 634705 [details]
stack trace+
Comment 2 :Aryeh Gregor (away until August 15) 2012-06-22 05:00:16 PDT
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 [:bz] 2012-06-22 09:18:06 PDT
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 (away until August 15) 2012-06-28 04:15:41 PDT
(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 (away until August 15) 2012-06-28 04:23:17 PDT
smaug pointed out the existence of nsMutationGuard to me.  That might do what we want -- let me try.
Comment 6 :Aryeh Gregor (away until August 15) 2012-06-29 03:30:27 PDT
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
Comment 7 :Aryeh Gregor (away until August 15) 2012-06-29 04:39:50 PDT
Comment on attachment 637828 [details] [diff] [review]
Patch v1, using nsMutationGuard

Nope, test failures.  Need to debug.
Comment 8 :Aryeh Gregor (away until August 15) 2012-07-01 12:19:05 PDT
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.
Comment 9 Olli Pettay [:smaug] 2012-07-03 13:35:42 PDT
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 (away until August 15) 2012-07-05 08:44:54 PDT
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
Comment 11 Olli Pettay [:smaug] 2012-07-05 10:04:46 PDT
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 (away until August 15) 2012-07-05 23:52:14 PDT
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 (away until August 15) 2012-07-06 01:02:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/510602478d52

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