Last Comment Bug 765799 - newOffset used uninitialized in nsRange::InsertNode
: newOffset used uninitialized in nsRange::InsertNode
: regression
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)
Depends on:
Blocks: buildwarning 433662 765177
  Show dependency treegraph
Reported: 2012-06-18 09:54 PDT by :Ms2ger
Modified: 2012-08-14 08:16 PDT (History)
5 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (2.19 KB, patch)
2012-06-19 00:13 PDT, :Aryeh Gregor (away until August 15)
bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description :Ms2ger 2012-06-18 09:54:50 PDT
Aryeh, could you fix?
Comment 1 :Aryeh Gregor (away until August 15) 2012-06-18 23:41:06 PDT
So the problem here is that bug 433662 comment 29 asked that we not compute newOffset unless necessary, because IndexOf is not so fast.  But I didn't account for the weird corner case in which inserting the node could collapse the range.  This should happen only when the range is from (parent, n) to (parent, n + 1), and when the node you're inserting is the nth child of parent.  Then it's removed (collapsing the range), and then reinserted (but the range is still collapsed).

I'll try to rewrite this to not use IndexOf at all.
Comment 2 :Aryeh Gregor (away until August 15) 2012-06-19 00:13:14 PDT
Created attachment 634310 [details] [diff] [review]
Patch v1

You asked in bug 433662 comment 29 that I compute newOffset only if needed, but it turns out it's not easy to figure out in advance whether it's needed.  The insertBefore could cause the range to collapse if it wasn't already collapsed, because the node we're inserting might be the only thing in the range.  There are a bunch of ways this could happen, and checking for them in advance would be error-prone, take a lot of code, and probably be more expensive in most cases than just doing IndexOf().  Also, we can't do RemoveChild before checking Collapsed(), because InsertBefore might throw before removing the child.  So I don't see any way to do this correctly without computing IndexOf() in advance every time.

Comment 3 :Aryeh Gregor (away until August 15) 2012-06-19 00:13:56 PDT
(in-testsuite+ because of bug 765177)
Comment 4 :Aryeh Gregor (away until August 15) 2012-06-19 01:21:20 PDT
Try run without other patches that cause unrelated failures:
Comment 5 :Aryeh Gregor (away until August 15) 2012-06-19 06:08:07 PDT
Comment on attachment 634310 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 433662 (Firefox 14)

User impact if declined: Small, but it is a regression.  An uninitialized variable is used when an author script calls the Range.insertNode method, and passes as its argument a node that's already at or contains the start of the range.  Then the range can become collapsed, which causes SetEnd() to be called with the uninitialized variable.  This will most likely cause an exception to be incorrectly thrown due to out-of-range index, and the range to remain collapsed.

Testing completed (on m-c, etc.): None yet, because I'm requesting approval concurrently with committing to m-i.

Risk to taking this patch (and alternatives if risky): The patch is very low-risk.  All it does is remove the conditional if (Collapsed()) { ... } and run the contents of the block unconditionally, so that the variable in question is always initialized.  In the common case where the bug doesn't occur, the variable won't be used, so there's no behavior change.  In the rare case where the bug does occur, it will be prevented.  There's extensive W3C testing of Range.insertNode <>, and this patch causes Firefox to pass all tests.  The block of code that's being made unconditional was intended to be unconditional to start with, but was made (incorrectly) conditional to avoid unnecessary computation.

String or UUID changes made by this patch: None
Comment 6 :Aryeh Gregor (away until August 15) 2012-06-19 06:18:20 PDT
Comment 7 Ed Morley [:emorley] 2012-06-20 02:23:12 PDT
Comment 8 Alex Keybl [:akeybl] 2012-06-20 15:37:53 PDT
Comment on attachment 634310 [details] [diff] [review]
Patch v1

[Triage Comment]
Little to no risk (as compared to current behavior) - let's land on all branches.

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