211493 - nsXBLProtoImplMethod::CompileMember doesn't check rv from aContext->CompileFunction. Crash [@ JS_CloneFunctionObject]

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[timeless]

Broken code in my source file nsXBLProtoImplMethod.cpp rev 1.5:

    JSObject* methodObject = nsnull;
    aContext->CompileFunction(aClassObject,
                              cname,
                              paramCount,
                              (const char**)args,
                              body, 
                              functionUri.get(),
                              mUncompiledMethod->mBodyText.GetLineNumber(),
                              PR_FALSE,
                              (void **) &methodObject);

    // Destroy our uncompiled method and delete our arg list.

Stack of crash:
JS_CloneFunctionObject(JSContext * 0x1444be70, JSObject * 0x1f0a4f50, JSObject * 0x047cbb10) line 2867 + 5 bytes
nsXBLProtoImplMethod::InstallMember(nsIScriptContext * 0x144490b0, nsIContent * 0x2562fe20, void * 0x118b8848, void * 0x118b8858) line 180 + 21 bytes
nsXBLProtoImpl::InstallImplementation(nsXBLPrototypeBinding * 0x0ce04fb8, nsIContent * 0x2562fe20) line 86 + 32 bytes
nsXBLPrototypeBinding::InstallImplementation(nsIContent * 0x2562fe20) line 444 + 19 bytes
nsXBLBinding::InstallImplementation(nsXBLBinding * const 0x23132a60) line 1035
nsXBLService::LoadBindings(nsXBLService * const 0x004f5f60, nsIContent * 0x2562fe20, const nsAString & {...}, int 0, nsIXBLBinding * * 0x0012ebec, int * 0x0012ebd8) line 644
nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell * 0x25637360, nsIPresContext * 0x25633a10, nsFrameConstructorState & {...}, nsIContent * 0x2562fe20, nsIFrame * 0x0cbc3790, nsIAtom * 0x010103d0, int 9, nsStyleContext * 0x0cbc3a28, nsFrameItems & {...}, int 0) line 7235 + 50 bytes

nsXBLProtoImplMethod.mJSMethodObject = {}
+  map    0x00000000
+  slots  0x00000000

Comment 1

[timeless]

Attachment: -uwp11 check CompileFunction rv

I believe JS_CloneFunctionObject and JS_DefineUCProperty can fail due to OOM,
so this handles that.
The old code leaked args[] if body.IsEmpty().
If the compilation fails this returns OOM, but the object should still be valid
if there is more memory later and someone tries to compile it again.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127027 [details] [diff] [review]
-uwp11 check CompileFunction rv

If you look, the caller of CompileMember does not check rv, so it would just
treat the member as compiled.  Now mUncompiledMethod and mJSMethodObject are in
a union, and if things think that the method is compiled they will try to
access mJSMethodObject (since that's assumed to be safe after the CompileMember
call) and get garbage (because what's really there is mUncompiledMethod, which
you never cleared).  This is all in the error case, of course.

So imo you should still be deleting mUncompiledMethod and setting
mJSMethodObject to null on failure...

Also, there is some similar code in nsXBLProtoImplProperty (in two places) that
may need similar changes...

Comment 3

[timeless]

Attachment: -uwp11 draft

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127060 [details] [diff] [review]
-uwp11 draft

sr=me assuming that the error handling in nsXBLProtoImplProperty::CompileMember
is fine...

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 127060 [details] [diff] [review]
-uwp11 draft

>+  nsresult rv = NS_OK;
>+  if (!mClassObject) {
>+    rv = CompilePrototypeMembers(aBinding); // This is the first time we've ever installed this binding on an element.
>                                  // We need to go ahead and compile all methods and properties on a class
>                                  // in our prototype binding.
>+    if (NS_FAILED(rv))
>+      return rv;

Other places in this file use NS_ENSURE_SUCCESS(rv, rv).  Please be consistent
with that (applies to a few other parts of the patch as well).

r=bryner with that change. (the error handling in
nsXBLProtoImplProperty::CompileMember looks ok)

Comment 6

[timeless]

checked in