Closed Bug 752340 Opened 12 years ago Closed 12 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: