Last Comment Bug 775552 - Logic error in bug 755264
: Logic error in bug 755264
Status: RESOLVED FIXED
[qa-]
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla17
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
Depends on:
Blocks: 755264 CVE-2012-3958
  Show dependency treegraph
 
Reported: 2012-07-19 08:11 PDT by :Aryeh Gregor (working until September 2)
Modified: 2012-08-14 08:30 PDT (History)
3 users (show)
ayg: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
Patch (already reviewed by ehsan on bug 772332) (1.21 KB, patch)
2012-07-20 04:11 PDT, :Aryeh Gregor (working until September 2)
ayg: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description :Aryeh Gregor (working until September 2) 2012-07-19 08:11:02 PDT
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 :Ehsan Akhgari 2012-07-19 08:17:01 PDT
(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 (working until September 2) 2012-07-20 04:11:38 PDT
Created attachment 644251 [details] [diff] [review]
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 (working until September 2) 2012-07-22 07:51:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d98e00fb4fd6
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-22 12:44:22 PDT
https://hg.mozilla.org/mozilla-central/rev/d98e00fb4fd6
Comment 5 Alex Keybl [:akeybl] 2012-07-23 10:21:21 PDT
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] 2012-07-23 10:24:11 PDT
Aurora 16*. Living in the past.
Comment 8 :Ehsan Akhgari 2012-07-26 19:31:24 PDT
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.

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