The default bug view has changed. See this FAQ.

Status

()

Core
Editor
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

({regression})

Trunk
mozilla17
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox15+ fixed, firefox16+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

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?
(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.
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.)
Attachment #644251 - Flags: review+
Attachment #644251 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/d98e00fb4fd6
Flags: in-testsuite?
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/d98e00fb4fd6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 5

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

Comment 6

5 years ago
Aurora 16*. Living in the past.
http://hg.mozilla.org/releases/mozilla-aurora/rev/be0f058891bc
status-firefox16: --- → fixed
tracking-firefox16: --- → +
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.
Attachment #644251 - Flags: approval-mozilla-beta?

Updated

5 years ago
Attachment #644251 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
http://hg.mozilla.org/releases/mozilla-beta/rev/433f6a85c2d4
status-firefox15: --- → fixed
tracking-firefox15: --- → +

Updated

5 years ago
Blocks: 772346

Updated

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