Closed Bug 752340 Opened 13 years ago Closed 13 years ago

js::FunctionProxyClass needs a finalizer

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox13 --- unaffected
firefox14 --- unaffected
firefox15 + verified
firefox16 + verified
firefox-esr10 --- unaffected

People

(Reporter: bc, Assigned: mccr8)

References

(
URL
)

Details

(5 keywords, Whiteboard: [sg:critical][advisory-tracking-] regressed around 5/5)

Crash Data

Attachments

(5 files, 1 obsolete file)

summary of results
5.74 KB, text/html
Details
bisection blame changesets
1.52 KB, text/plain
Details
Testcase mochitest
2.52 KB, patch
Details | Diff | Splinter Review
use finalizer for FunctionProxyClass
1.07 KB, patch
jorendorff
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
check that swap preserves finalizerness
1.24 KB, patch
mccr8
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
1. http://de.futbol24.com/national/Algeria/Algerian-Cup/2011-2012/ http://www.futbol24.com/team/Germany/1FC-Koln/ 2. Shutdown 3. Crash | Assert s-s due to 0xda in edx/crash address on Windows. This could be XPConnect or Plugins even but I'm putting in JS for now due to the GC related issues. Mac Nightly [@ XPCWrappedNative::GetUsedOnly ] bp-8a6c1947-5971-49fb-b457-f1cc42120506 bp-76602895-fcb6-4a2f-8b97-763312120506 Linux Nightly [@ XPCWrappedNative::GetUsedOnly ] bp-33368303-f3fd-4146-b816-dd52e2120506 [@ js::IsIncrementalBarrierNeededOnObject ] bp-aa6a4d60-b85e-4626-bfb2-d7e742120506 Windows/Mac/Linux Debug Assertion failure: allocated(), at ../../../dist/include/gc/Heap.h:596 Operating system: Linux 0.0.0 Linux 2.6.32-220.13.1.el6.x86_64 #1 SMP Thu Mar 29 11:46:40 EDT 2012 x86_64 CPU: amd64 family 6 model 37 stepping 1 2 CPUs Crash reason: SIGABRT Crash address: 0x1ff000053a8 Thread 0 (crashed) 0 libpthread-2.12.so + 0xf36b rbx = 0x0000000000000001 r12 = 0x0000000000000000 r13 = 0x00007fe3b62c60bd r14 = 0x0000000000000000 r15 = 0x0000000000000000 rip = 0x0000003f0a20f36b rsp = 0x00007fff6e056188 rbp = 0x00007fff6e0561a0 Found by: given as instruction pointer in context 1 libxul.so!js::gc::ArenaHeader::getThingSize [Heap.h : 596 + 0x21] rip = 0x00007fe3b54236d3 rsp = 0x00007fff6e056190 Found by: stack scanning 2 libxul.so!js::gc::AssertValidColor [Heap.h : 917 + 0xe] rip = 0x00007fe3b72c1049 rsp = 0x00007fff6e0561b0 Found by: stack scanning 3 libxul.so!nsGenericElement::InitCCCallbacks [nsGenericElement.cpp : 4949 + 0x1] rip = 0x00007fe3b59c1336 rsp = 0x00007fff6e0561e0 Found by: stack scanning 4 0x7fff6e0561ff rip = 0x00007fff6e056200 rsp = 0x00007fff6e0561e8 rbp = 0x00007fe3b59c1336 Found by: call frame info 5 libxul.so!js::gc::Cell::isMarked [Heap.h : 947 + 0x10] rip = 0x00007fe3b72c10a0 rsp = 0x00007fff6e0561f0 Found by: stack scanning 6 libxul.so!js::GCThingIsMarkedGray [jsfriendapi.cpp : 464 + 0x10] rip = 0x00007fe3b72c2715 rsp = 0x00007fff6e056210 Found by: stack scanning 7 libxul.so!xpc_IsGrayGCThing [xpcpublic.h : 163 + 0xb] rip = 0x00007fe3b5909b60 rsp = 0x00007fff6e056230 Found by: stack scanning 8 libxul.so!nsWrapperCache::IsBlack [nsWrapperCacheInlines.h : 57 + 0xb] rip = 0x00007fe3b5909b95 rsp = 0x00007fff6e056250 Found by: stack scanning 9 libxul.so!nsGenericElement::CanSkip [nsGenericElement.cpp : 4874 + 0xf] rip = 0x00007fe3b59c100f rsp = 0x00007fff6e056280 Found by: stack scanning also Windows Debug hit what appears to be deleted memory Windows Operating system: Windows NT 5.1.2600 Service Pack 3 CPU: x86 GenuineIntel family 6 model 37 stepping 1 1 CPU Crash reason: EXCEPTION_ACCESS_VIOLATION_READ Crash address: 0xffffffffdadadada Thread 0 (crashed) 0 xul.dll!js::GetObjectClass(JSObject const *) [jsfriendapi.h : 366 + 0x7] eip = 0x01b2a1ca esp = 0x0012cee0 ebp = 0x0012cee0 ebx = 0x00000001 esi = 0x00000080 edi = 0x0022afd8 eax = 0x0e1a4f10 ecx = 0x0e1a4fb0 edx = 0xdadadada efl = 0x00210202 Found by: given as instruction pointer in context 1 xul.dll!DebugCheckWrapperClass(JSObject *) [xpcpublic.h : 101 + 0x8] eip = 0x01b2a41c esp = 0x0012cee8 ebp = 0x0012ceec Found by: call frame info 2 xul.dll!XPCWrappedNative::GetUsedOnly(XPCCallContext &,nsISupports *,XPCWrappedNativeScope *,XPCNativeInterface *,XPCWrappedNative * *) [XPCWrappedNative.cpp : 864 + 0xe] eip = 0x023961a9 esp = 0x0012cef4 ebp = 0x0012cf2c Found by: call frame info 3 xul.dll!nsXPConnect::GetWrappedNativeOfNativeObject(JSContext *,JSObject *,nsISupports *,nsID const &,nsIXPConnectWrappedNative * *) [nsXPConnect.cpp : 1526 + 0x29] eip = 0x02347d8c esp = 0x0012cf34 ebp = 0x0012d018 Found by: call frame info 4 xul.dll!nsJSNPRuntime::OnPluginDestroy(_NPP *) [nsJSNPRuntime.cpp : 2048 + 0x5c] eip = 0x026a3d0d esp = 0x0012d020 ebp = 0x0012d0e8 Found by: call frame info 5 xul.dll!nsNPAPIPluginInstance::Stop() [nsNPAPIPluginInstance.cpp : 249 + 0xb] eip = 0x02683334 esp = 0x0012d0f0 ebp = 0x0012d164 Found by: call frame info 6 xul.dll!nsPluginHost::StopPluginInstance(nsNPAPIPluginInstance *) [nsPluginHost.cpp : 3175 + 0x7] eip = 0x0269a569 esp = 0x0012d16c ebp = 0x0012d1a4 Found by: call frame info
On http://www.futbol24.com/Live/ I saw additional windows crashes with the 0xda pattern as well as one new one for Mac ###!!! ASSERTION: Forgot to check if this is a wrapper?: 'IS_WRAPPER_CLASS(js::GetObjectClass(obj))', file /work/mozilla/builds/nightly/mozilla/js/xpconnect/src/xpcpublic.h, line 101 Assertion failure: slot < (((GetObjectClass(obj))->flags >> 8) & (((uint32_t)1 << (8)) - 1)), at /work/mozilla/builds/nightly/mozilla/js/src/jsfriendapi.h:439 perating system: Mac OS X 10.6.8 10K549 CPU: amd64 family 6 model 23 stepping 10 2 CPUs Crash reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS Crash address: 0x0 Thread 0 (crashed) 0 XUL!js::GetReservedSlot [jsfriendapi.h : 439 + 0x3e] rbx = 0x0000000101842560 r12 = 0x000000010291b270 r13 = 0x00007fff5fbf7450 r14 = 0x00000001288e8f10 r15 = 0x0000000101809730 rip = 0x0000000103a19c3e rsp = 0x00007fff5fbf7110 rbp = 0x00007fff5fbf7120 Found by: given as instruction pointer in context 1 XUL!XPCWrappedNative::GetUsedOnly [XPCWrappedNative.cpp : 864 + 0x21] rbx = 0x0000000101842560 r12 = 0x000000010291b270 r13 = 0x00007fff5fbf7450 r14 = 0x00000001288e8f10 r15 = 0x0000000101809730 rip = 0x000000010297829c rsp = 0x00007fff5fbf7130 rbp = 0x00007fff5fbf71d0 Found by: call frame info 2 XUL!nsXPConnect::GetWrappedNativeOfNativeObject [nsXPConnect.cpp : 1526 + 0x29] rbx = 0x0000000101842560 r12 = 0x000000010291b270 r13 = 0x00007fff5fbf7450 r14 = 0x00000001288e8f10 r15 = 0x0000000101809730 rip = 0x000000010291b497 rsp = 0x00007fff5fbf71e0 rbp = 0x00007fff5fbf7360 Found by: call frame info 3 XUL!nsJSNPRuntime::OnPluginDestroy [nsJSNPRuntime.cpp : 2048 + 0x79] rbx = 0x0000000101842560 r12 = 0x000000010291b270 r13 = 0x00007fff5fbf7450 r14 = 0x00000001288e8f10 r15 = 0x0000000101809730 rip = 0x0000000102ce0f64 rsp = 0x00007fff5fbf7370 rbp = 0x00007fff5fbf74f0 Found by: call frame info 4 XUL!nsNPAPIPluginInstance::Stop [nsNPAPIPluginInstance.cpp : 249 + 0xc] rbx = 0x000000011c243e50 r12 = 0x0000000101fed58e r13 = 0x0000000000000001 r14 = 0x0000000101809730 r15 = 0x0000000101809730 rip = 0x0000000102cc3214 rsp = 0x00007fff5fbf7500 rbp = 0x00007fff5fbf7550 Found by: call frame info
The breakpad exploitable tool rates this additional signature as highly exploitable: http://de.futbol24.com/national/Algeria/Division-2/2011-2012/ Operating system: Windows NT 6.1.7601 Service Pack 1 CPU: x86 GenuineIntel family 6 model 37 stepping 1 2 CPUs Crash reason: EXCEPTION_ACCESS_VIOLATION_EXEC Crash address: 0x2f6b726f Thread 0 (crashed) 0 0x2f6b726f eip = 0x2f6b726f esp = 0x0035ce08 ebp = 0x0035ce20 ebx = 0x00000001 esi = 0x00000080 edi = 0x00000000 eax = 0x0035ce68 ecx = 0x71c6169c edx = 0x2f6b726f efl = 0x00210206 Found by: given as instruction pointer in context 1 mozjs.dll!js::gc::MarkChildren(JSTracer *,JSObject *) [Marking.cpp : 676 + 0xb] eip = 0x727faeff esp = 0x0035ce28 ebp = 0x0035ce2c Found by: previous frame's frame pointer 2 mozjs.dll!js::TraceChildren(JSTracer *,void *,JSGCTraceKind) [Marking.cpp : 1163 + 0xc] eip = 0x727fd38b esp = 0x0035ce34 ebp = 0x0035ce40 Found by: call frame info 3 mozjs.dll!JS_TraceChildren [jsapi.cpp : 2495 + 0x10] eip = 0x72544154 esp = 0x0035ce48 ebp = 0x0035ce54 Found by: call frame info 4 xul.dll!xpc_UnmarkGrayGCThingRecursive(void *,JSGCTraceKind) [nsXPConnect.cpp : 701 + 0x11] eip = 0x7072432d esp = 0x0035ce5c ebp = 0x0035ce90 Found by: call frame info 5 xul.dll!xpc_UnmarkGrayObject(JSObject *) [xpcpublic.h : 182 + 0xa] eip = 0x6ff0a4b4 esp = 0x0035ce98 ebp = 0x0035cea0 Found by: call frame info 6 xul.dll!nsWrapperCache::GetWrapper() [nsWrapperCacheInlines.h : 49 + 0x8] eip = 0x6ff0a47d esp = 0x0035cea8 ebp = 0x0035ceb4 Found by: call frame info 7 xul.dll!XPCWrappedNative::GetUsedOnly(XPCCallContext &,nsISupports *,XPCWrappedNativeScope *,XPCNativeInterface *,XPCWrappedNative * *) [XPCWrappedNative.cpp : 863 + 0x7] eip = 0x70776197 esp = 0x0035cebc ebp = 0x0035cef0 Found by: call frame info
Attached file summary of results —
I've included the list of futbol24 related urls where these crashes have occurred. A number have 0xda in the crashing address.
Severity: normal → critical
Crash Signature: [@ XPCWrappedNative::GetUsedOnly ] [@ js::IsIncrementalBarrierNeededOnObject ] → [@ CallQueryInterface(nsISupports*, nsWrapperCache**) ] [@ IS_WRAPPER_CLASS ] [@ JS::Value::isUndefined ] [@ XPCWrappedNative::GetUsedOnly ] [@ js::GetObjectClass(JSObject const*) ] [@ js::GetReservedSlot(JSObject const* unsigned int) ] [@ js::IsInc…
Why "regression"? do you have a broad (or specific!) range of versions that do and don't crash?
Keywords: regressionwindow-wanted, sec-critical, testcase-wanted
Whiteboard: [sg:critical]
I'm going to send this to DOM. I got it to crash by loading the page, waiting about 5 seconds, and then hitting reload. In all the crashes I saw, the JSObject attached to a DOM node via the wrapper cache has already been garbage collected. It seems like maybe there's some object that should clear the wrapper cache when it's finalized, and somehow that's not happening.
regression since it is a) nightly only and b) it started with 5/5 nightly builds. (Sorry I forgot to mention that).
status-firefox-esr10: --- → unaffected
status-firefox13: --- → unaffected
status-firefox14: --- → unaffected
status-firefox15: --- → affected
tracking-firefox15: --- → +
Keywords: regressionwindow-wanted
Whiteboard: [sg:critical] → [sg:critical] regressed around 5/5
Moving to DOM per comment 5.
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
very possibly the same as bug 752286. See bug 752286 comment 1 According to Scoobidiver, the regression range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db9df42823d&tochange=9ebf3dc839c5 possibly either bug 748701 or bug 751641.
I bisected on Linux 32bit and after dealing with the busted builds, it blamed these changesets. These don't include the bugs Scoobidiver implicated though. :-(
Going back to Scoobidiver's regression range Josh Aas \u2014 Bug 751641: Fix bug in which we add a Java MIME type to all plugins. r=jst Jared Wein — Bug 748701 - Crash in nsObjectLoadingContent::IsPluginEnabledForType. r=joshmoz Robert O'Callahan — Bug 653994. Avoid trying to paint plugin widgets in the case where a plugin fails to subclass our window. r=bsmedberg
Two of the three in comment 10 involve Josh (not to mention the plugin involvement) so I nominate him to find someone to fix this regression.
Assignee: nobody → joshmoz
For the signature XPCWrappedNative::GetUsedOnly in the past week: Windows 7: 68.182%, 45 crashes Windows XP: 25.758%, 17 crashes Windows Unknown: 4.545%, 3 crashes Windows Vista: 1.515%, 1 crash x86: 63.636%, 42 crashes amd64: 36.364%, 24 crashes Firefox 15.0a1: 66.667%, 44 crashes Firefox 16.0a1: 10.606%, 7 crashes Firefox 12.0: 7.576%, 5 crashes Firefox 11.0: 3.030% 2 crashes Firefox 13.0b7: 3.030% 2 crashes Thunderbird 12.0.1: 3.030% 2 crashes Firefox 13.0b6: 3.030% 2 crashes Firefox 12.0b6: 1.515% 1 crash Firefox 11.0b3: 1.515% 1 crash Comments from the past four weeks: * Det såg svårt ut de lilla jag såg o jag förståg inte mycket av spelet.. men hann fixa över 20. NET.API iaf.. ;) * Frequently responds looking up web site before saying can't find it. I reload the page a few times and eventually the page loads OK * outlook,gimp and explorer is open when i close a preview of print the Nightly crashes * Trying to print a page
Benjamin is looking into this. I was able to reproduce the crash with a debug build on Mac OS X.
Assignee: joshmoz → benjamin
Reproed the assertion and/or a crash on Windows debug build: xul.dll!js::GetObjectClass(obj=0x10877c90) Line 323 C++ xul.dll!DebugCheckWrapperClass(obj=0x10877c90) Line 69 C++ > xul.dll!XPCWrappedNative::GetUsedOnly(ccx={...}, Object=0x0e2f0d60, Scope=0x0a51d700, Interface=0x068fe1a0, resultWrapper=0x002cca2c) Line 832 C++ xul.dll!nsXPConnect::GetWrappedNativeOfNativeObject(aJSContext=0x0a581588, aScope=0x07c0c040, aCOMObj=0x0e2f0d60, aIID={...}, _retval=0x002ccb2c) Line 1489 C++ xul.dll!nsObjectLoadingContent::NotifyContentObjectWrapper() Line 2265 C++ xul.dll!nsObjectLoadingContent::InstantiatePluginInstance(aMimeType=0x0ebdada0, aURI=0x0e2eae78) Line 707 C++ xul.dll!nsPluginStreamListenerPeer::OnStartRequest(request=0x0e2eb074, aContext=0x00000000) Line 605 C++ xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x0e2eb074, aContext=0x00000000) Line 969 C++ xul.dll!nsHttpChannel::CallOnStartRequest() Line 767 C++ It's pretty clear that nsWrapperCache::GetWrapper is returning a GCed object. Earlier when SetWrapper was called, the object has the correct js::Class and appears to be alive: + this 0x0e2f0d64 {mIsDoneAddingChildren=true } nsWrapperCache * const + js::GetObjectClass(aWrapper) 0x0a957564 {name=0x08a98b80 "HTMLObjectElement" flags=0x0000012d addProperty=0x5b2ca660 ...} js::Class * aWrapper 0x10877c90 JSObject * At the point of the crash the object has obviously been GCed. Still working on why the normal finalizer wouldn't be clearing the cache properly.
According to bz and IRC discussion, this appears to be fallout from compartment-per-global. The stack which shows the problem so far is: xul.dll!nsWrapperCache::SetWrapper(aWrapper=0x0fe8b060) Line 101 C++ > xul.dll!XPCWrappedNative::ReparentWrapperIfFound(ccx={...}, aOldScope=0x0db85140, aNewScope=0x0a110e70, aNewParent=0x07f31180, aCOMObj=0x08394790, aWrapper=0x0036872c) Line 1698 C++ xul.dll!nsXPConnect::ReparentWrappedNativeIfFound(aJSContext=0x0a8a0e38, aScope=0x10617360, aNewParent=0x07f31180, aCOMObj=0x08394790, _retval=0x0036872c) Line 1520 C++ xul.dll!nsNodeUtils::CloneAndAdopt(aNode=0x08394790, aClone=false, aDeep=true, aNewNodeInfoManager=0x0a0f6690, aCx=0x0a8a0e38, aNewScope=0x07f31180, aNodesWithProperties={...}, aParent=0x00000000, aResult=0x00368830) Line 535 C++ xul.dll!nsNodeUtils::CloneAndAdopt(aNode=0x0d498ce8, aClone=false, aDeep=true, aNewNodeInfoManager=0x0a0f6690, aCx=0x0a8a0e38, aNewScope=0x07f31180, aNodesWithProperties={...}, aParent=0x00000000, aResult=0x003688f0) Line 559 C++ xul.dll!nsNodeUtils::CloneAndAdopt(aNode=0x0d498ce8, aClone=false, aDeep=true, aNewNodeInfoManager=0x0a0f6690, aCx=0x0a8a0e38, aNewScope=0x07f31180, aNodesWithProperties={...}, aResult=0x00000000) Line 272 C++ xul.dll!nsNodeUtils::Adopt(aNode=0x0d498ce8, aNewNodeInfoManager=0x0a0f6690, aCx=0x0a8a0e38, aNewScope=0x07f31180, aNodesWithProperties={...}) Line 174 C++ xul.dll!nsDocument::AdoptNode(aAdoptedNode=0x0d498d30, aResult=0x00368aa0) Line 6156 C++ xul.dll!nsHTMLDocument::AdoptNode(source=0x0d498d30, _retval=0x00368aa0) Line 90 C++ xul.dll!AdoptNodeIntoOwnerDoc(aParent=0x0d603540, aNode=0x0d498ce8) Line 3713 C++ xul.dll!nsINode::ReplaceOrInsertBefore(aReplace=false, aNewChild=0x0d498ce8, aRefChild=0x00000000) Line 4260 C++ xul.dll!nsINode::ReplaceOrInsertBefore(aReplace=false, aNewChild=0x0d498ce8, aRefChild=0x00000000, aReturn=0x00368d64) Line 1438 C++ xul.dll!nsINode::InsertBefore(aNewChild=0x0d498ce8, aRefChild=0x00000000, aReturn=0x00368d64) Line 477 C++ xul.dll!nsINode::AppendChild(aNewChild=0x0d498ce8, aReturn=0x00368d64) Line 487 C++ xul.dll!nsIDOMNode_AppendChild(cx=0x0a688008, argc=0x00000001, vp=0x07730500) Line 5502 C++ In this call to SetWrapper, the "allocKind" of the JSObject is FINALIZE_OBJECT8_BACKGROUND. For normal nsHTMLObjectElement wrappers it is FINALIZE_OBJECT2. So the JS engine is not calling the finalizer (which clears the wrapper cache) because of the incorrect GC kind.
All line numbers are from rev 0aa7fc75cad5, by the way. The previous wrapper object for this node is a js::FunctionProxyClass. GetProxyHandler says that this is ajs::CrossCompartmentWrapper::singleton. I'm going to try and make a more minimal testcase for this and then hand it to bholley.
If we assert in SetWrapper that the object has a finalizer, would that catch where the proxy is getting set as the wrapper? This seems like a reasonable thing to assert anyway, since we need a finalizer to clear the wrapper cache eventually. Also, we should be asserting in the JS engine that you never swap an object with a background finalize kind with an object that doesn't have a background finalize kind. However, that assertion would trip later than the first one in this case.
mccr8 is looking into something similar in bug 752764 and bug 760887. I'll bet this is related.
I added that assertion in a local build and it never tripped: although I don't have access to that machine this week.
Here's the testcase in mochitest form for checkin: it doesn't trigger the crash until you reload/unload the testcase, but it also discovered a bug about the scripting plugin prototype not being installed correctly, which is probably directly related ;-) Giving the bug to somebody else now!
Assignee: benjamin → continuation
Jesse, are there any changes that could be made to the DOM fuzzer so it could catch things like this in the future?
The first bad revision is: changeset: 92954:bed8c4e3dfdf user: Luke Wagner <luke@mozilla.com> date: Thu May 03 09:10:12 2012 +0200 summary: Bug 650353 - Implement Compartment-Per-Global in XPConnect. r=mrbkap
Blocks: cpg
Keywords: testcase-wanted → testcase
Blocks: 752286
It looks like this is actually a JS problem with a one line fix. FunctionProxyClass doesn't have a finalizer, but it probably should. billm helped me dig through this. I have no idea why this is suddenly a problem. Maybe some XPConnect thing became a function where it wasn't before. Bug 769059 has a test case :johns found that is slightly less finicky. I'll upload it here at some point.
In a followup bug, I should add assertions about wrappers having finalizers, and that transplanted things have a finalizer iff the thing they are being finalized to has one.
Component: DOM → JavaScript Engine
QA Contact: general → general
Summary: Plugin related crash on shutdown - Crash [@ XPCWrappedNative::GetUsedOnly ] | Crash [@ js::IsIncrementalBarrierNeededOnObject ] | Assertion failure: allocated() → js::FunctionProxyClass needs a finalizer
Comment on attachment 637342 [details] [diff] [review] use finalizer for FunctionProxyClass Bill suggested you might have an idea of why it is the way it is, jorendorff.
Attachment #637342 - Flags: review?(jorendorff)
This is the kind of assertion that Bill suggested. With this patch applied, but not the fix, all of these various test cases people have come up with fail immediately with this assertion. I'll do some try runs to see if the assertion fails with and without the proxy patch.
Attachment #638030 - Flags: review?(wmccloskey)
Comment on attachment 638030 [details] [diff] [review] check that swap preserves finalizerness Review of attachment 638030 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.cpp @@ +200,5 @@ > #undef OFFSET > > #ifdef DEBUG > +bool > +IsAllocKindFinalized(AllocKind ak) There's already IsBackgroundAllocKind, which is the negation of this, so let's use it. ::: js/src/jsobj.cpp @@ +3566,5 @@ > bool > JSObject::swap(JSContext *cx, JSObject *other) > { > + // Ensure swap doesn't cause a finalizer to not be run. > + MOZ_ASSERT(IsAllocKindFinalized(getAllocKind()) == JS_ASSERT
Attachment #638030 - Flags: review?(wmccloskey) → review+
Updated to address review comments. Carrying forward billm's r+. Thanks for the suggestions. I looked around for AllocKind functions, but I somehow failed to find IsBackgroundAllocKind.
Attachment #638030 - Attachment is obsolete: true
Attachment #638051 - Flags: review+
Comment on attachment 637342 [details] [diff] [review] use finalizer for FunctionProxyClass Review of attachment 637342 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a test; bsmedberg's mochitest would fit the bill. It bothers me a little that ObjectProxyClass has a convert hook and FunctionProxyClass doesn't. But maybe it's impossible to observe.
Attachment #637342 - Flags: review?(jorendorff) → review+
Looking into the test failure in this mochitest, it seems that moving the plugin into a subdocument makes it impossible to access any functions exported by plugin. The plugin remains running and does not reinstantiate (as expected).
I'm going to use johns's test case from bug 769059 as a crash test, as it seems to be a little less flakey about failing. The last test of bsmedberg's mochitest still fails with this patch, but all the multitude of crashes in this bug and the dupes seem to be fixed.
Are you going to file a separate bug about the incorrect plugin behavior, then? It appears that when reparenting happens it isn't taking into account the plugin proto object.
Sure, I can do that. That does sound like another CPG regression. Does it need to be security sensitive?
No
(In reply to Benjamin Smedberg [:bsmedberg] from comment #37) > Are you going to file a separate bug about the incorrect plugin behavior, > then? It appears that when reparenting happens it isn't taking into account > the plugin proto object. Filed as bug 771202.
Comment on attachment 637342 [details] [diff] [review] use finalizer for FunctionProxyClass [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 650353 User impact if declined: crashes, potential exploits Testing completed (on m-c, etc.): it has been on m-c for a few days Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none
Attachment #637342 - Flags: approval-mozilla-aurora?
Comment on attachment 638051 [details] [diff] [review] check that swap preserves finalizerness (see above) These assertion changes will help us catch potential future regressions of this nature (which will be sec-crit) on Aurora (as unlikely as that is) and will only affect debug builds.
Attachment #638051 - Flags: approval-mozilla-aurora?
Blocks: 771202
No longer blocks: 771202
Comment on attachment 637342 [details] [diff] [review] use finalizer for FunctionProxyClass [Triage Comment] Low risk sg:crit fix for cpg in FF15. Approved.
Attachment #637342 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #638051 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: 752286
Regression after branching for Firefox 15 so it doesn't need an advisory.
Whiteboard: [sg:critical] regressed around 5/5 → [sg:critical][advisory-tracking-] regressed around 5/5
Keywords: verifyme
2012-05-05 Firefox 15 Nightly debug: crash 2012-08-17 Firefox 15 Beta debug: no crash 2012-08-17 Firefox 16 Aurora debug: no crash
Status: RESOLVED → VERIFIED
status-firefox15: fixed → verified
status-firefox16: fixed → verified
Keywords: verifyme
QA Contact: anthony.s.hughes
Group: core-security
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: