775552 - Logic error in bug 755264

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Bug 772346 comment #8:

> This code is probably wrong anyway, for the same reason as in bug 767684. 
> It now says
> 
>   for (nsCOMPtr<nsIContent> child = aNode->GetLastChild();
>        child;
>        child = child->GetPreviousSibling()) {
>     nsresult rv = DeleteNonTableElements(child);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
> But DeleteNonTableElements(child) might remove child, so GetPreviousSibling
> will incorrectly return null.  Changing it back to the way it was should
> both fix the use-after-free and make it correctly affect all children:
> 
>   for (PRInt32 i = aNode->GetChildCount() - 1; i >= 0; --i) {
>     nsresult rv = DeleteNonTableElements(aNode->GetChildAt(i));
>     NS_ENSURE_SUCCESS(rv, rv);
>   }

I attached a patch to bug 772332, but this should really have its own bug, particularly since the error has now crept into Aurora.

Ehsan, you asked for a backout, but the patch no longer backs out cleanly.  E.g., it needs changes for bug 763283 part 2, and for bug 772807.  Should we just backport the patch from bug 772332 to Aurora instead?

Comment 1

[(no longer active)]

(In reply to :Aryeh Gregor from comment #0)
> Ehsan, you asked for a backout, but the patch no longer backs out cleanly. 
> E.g., it needs changes for bug 763283 part 2, and for bug 772807.  Should we
> just backport the patch from bug 772332 to Aurora instead?

Sure.

Comment 2

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

Attachment: Patch (already reviewed by ehsan on bug 772332)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 755264
User impact if declined: Unknown -- no test case was produced that actually exhibits a problem.  The incorrect change caused bug 772346 (use-after-free), which was fixed separately, but no specific test-case was given.  However, the same erroneous change was made in a different place by bug 752210, which caused bug 767684.  It's likely that there are some cases where this bug breaks editor actions, where the user tries to delete some content but fewer elements are deleted than expected.
Testing completed (on m-c, etc.): None yet.  The patch reverts to the same logic as was in place before bug 755264, however, so in that sense the code has years of testing.
Risk to taking this patch (and alternatives if risky): Low.  The patch just reverts a logic change to the way it was before, and the change is very simple.  It's hard to see how it could cause problems.
String or UUID changes made by this patch: None.

I'll check the patch into m-i when it opens.  (The patch was already pushed in bug 772332 comment 10, but the push was backed out due to a problem with another patch in the same push, so it didn't make it to 16 before the uplift.)

Comment 3

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

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Alex Keybl [:akeybl]]

Comment on attachment 644251 [details] [diff] [review]
Patch (already reviewed by ehsan on bug 772332)

[Triage Comment]
Unknown user impact, but no need to find out with a FF15 logic regression. Approving for Aurora 15.

Comment 6

[Alex Keybl [:akeybl]]

Aurora 16*. Living in the past.

Comment 7

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/be0f058891bc

Comment 8

[(no longer active)]

Comment on attachment 644251 [details] [diff] [review]
Patch (already reviewed by ehsan on bug 772332)

This needs to go on beta as well since the bug which regressed this landed in Firefox 15.

Comment 9

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-beta/rev/433f6a85c2d4