765799 - newOffset used uninitialized in nsRange::InsertNode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Aryeh, could you fix?

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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 (:ayg) (no longer with Mozilla)]

Attachment: 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.

Try: https://tbpl.mozilla.org/?tree=Try&rev=cd4b680961bf

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(in-testsuite+ because of bug 765177)

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Try run without other patches that cause unrelated failures: https://tbpl.mozilla.org/?tree=Try&rev=40e761d3f9c5

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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 <http://w3c-test.org/webapps/DOMCore/tests/approved/Range-insertNode.html>, 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 (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e82037b16e21

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e82037b16e21

Comment 8

[Alex Keybl [:akeybl]]

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.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/655c41dbd234
https://hg.mozilla.org/releases/mozilla-beta/rev/caf4a31896ee