newOffset used uninitialized in nsRange::InsertNode

RESOLVED FIXED in Firefox 14



DOM: Core & HTML
5 years ago
5 years ago


(Reporter: Ms2ger, Assigned: ayg)


(Blocks: 1 bug, {regression})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox13 unaffected, firefox14 fixed, firefox15 fixed, firefox16 fixed)


(Whiteboard: [qa-])


(1 attachment)



5 years ago
Aryeh, could you fix?
Component: Editor → DOM: Core & HTML
QA Contact: editor → general
Assignee: nobody → ayg
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.
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.

Attachment #634310 - Flags: review?(bugs)
(in-testsuite+ because of bug 765177)
Flags: in-testsuite+
Try run without other patches that cause unrelated failures:
Attachment #634310 - Flags: review?(bugs) → review+
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
Attachment #634310 - Flags: approval-mozilla-beta?
Attachment #634310 - Flags: approval-mozilla-aurora?
Target Milestone: --- → mozilla16
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 8

5 years ago
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.
Attachment #634310 - Flags: approval-mozilla-beta?
Attachment #634310 - Flags: approval-mozilla-beta+
Attachment #634310 - Flags: approval-mozilla-aurora?
Attachment #634310 - Flags: approval-mozilla-aurora+

Comment 9

5 years ago
status-firefox13: --- → unaffected
status-firefox14: --- → fixed
status-firefox15: --- → fixed
status-firefox16: --- → fixed


5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.