Closed Bug 872774 Opened 6 years ago Closed 6 years ago
Crash with expando on SVGRect
Debug: Assertion failure: ccISupports, at nsContentUtils.h:1288 Nightly: Crash [@ mozilla::dom::SVGRectBinding::_addProperty] bp-a9ffdf10-268f-47d7-b677-f6fa82130515 Likely a regression from something in: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=08be63954b6b Perhaps bug 853386 or bug 866796
Do we have cycle collection for SVGIRect?
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #750172 - Flags: review?(bzbarsky)
Comment on attachment 750172 [details] [diff] [review] Patch r=me, and please nominate for branches as needed. That said, why is SVGIRect itself not cced? Seems to me like that would be preferable, since it's the thing that inherits from nsWrapperCache...
Attachment #750172 - Flags: review?(bzbarsky) → review+
Thanks for the quick fix! > That said, why is SVGIRect itself not cced? Seems to me like that would be preferable, since it's the thing that inherits from nsWrapperCache... The general approach that is taken is that "abstract" classes like nsIFoo never have CC in them, even if they have fields that need to be CCed. Instead, any "concrete" subclasses nsFoo implement CC. Probably not a great approach for a class like this that has multiple inheritors. Anyways, we're never tracing the wrapper cache for these classes, so we can end up touching dead memory, so I'm marking this sec-crit. This is on Aurora and Nightly, so fill out the sec-approval form (under patch details) before landing on inbound, please.
I'll do an audit of the CC goop for bug 866796 tomorrow.
Comment on attachment 750172 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 866796 User impact if declined: crashes, possibly exploitable Testing completed (on m-c, etc.): fixes crash Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #750172 - Flags: approval-mozilla-aurora?
Comment on attachment 750172 [details] [diff] [review] Patch [Security approval request comment] How easily could an exploit be constructed based on the patch? probably not too difficult to somebody who understand it Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no Which older supported branches are affected by this flaw? Aurora If not all supported branches, which bug introduced the flaw? I marked the dependency Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? should be the same How likely is this patch to cause regressions; how much testing does it need? It looks fairly safe to me.
Attachment #750172 - Flags: sec-approval?
Please wait for sec-approval+ before landing on inbound.
Comment on attachment 750172 [details] [diff] [review] Patch sec-approval+ for m-c. Please nominate for Aurora.
Attachment #750172 - Flags: sec-approval? → sec-approval+
(In reply to Jesse Ruderman from comment #10) > Does this patch also fix bug 872790 and/or bug 872812? I can't reproduce bug 872790 with this patch. bug 872812 still exists.
Attachment #750172 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.