217562 - UMR in nsJSEventListener::HandleEvent after mContext->CallEventHandler

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[timeless]

PRBool jsBoolResult;
  PRBool returnResult = (mReturnResult == nsReturnResult_eReverseReturnResult);

  rv = mContext->CallEventHandler(obj, JSVAL_TO_OBJECT(funval), argc, argv,
                                  &jsBoolResult, returnResult);

  if (argv != &arg) {
    ::JS_PopArguments(cx, stackPtr);
  }

  if (NS_FAILED(rv)) {
    return rv;
  }

  if (!jsBoolResult)
    aEvent->PreventDefault();

valgrind writes:
==4131== Conditional jump or move depends on uninitialised value(s)
==4131==    at 0x4717A9C7: nsJSEventListener::HandleEvent(nsIDOMEvent *)
(/mnt/ibm/mozhack/mozilla/dom/src/events/nsJSEventListener.cpp:191)
==4131==    by 0x44CBF450:
nsXBLPrototypeHandler::ExecuteHandler(nsIDOMEventReceiver *, nsIDOMEvent *)
(/mnt/ibm/mozhack/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp:457)
==4131==    by 0x44CBF7E4:
nsXBLPrototypeHandler::BindingAttached(nsIDOMEventReceiver *)
(/mnt/ibm/mozhack/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp:225)
==4131==    by 0x44CAF1E0:
nsXBLPrototypeBinding::BindingAttached(nsIDOMEventReceiver *)
(/mnt/ibm/mozhack/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp:384)
==4131==    by 0x44CAC04D: nsXBLBinding::ExecuteAttachedHandler(void)
(/mnt/ibm/mozhack/mozilla/content/xbl/src/nsXBLBinding.cpp:831)
==4131==    by 0x44CCD483: nsBindingManager::ProcessAttachedQueue(void)
(/mnt/ibm/mozhack/mozilla/content/xbl/src/nsBindingManager.cpp:957)

full valgrind log is available ...
build is xlib,disable-debug,-O2 -g, from today on boffo (tinderboxing source)

Comment 1

[timeless]

Attachment: copy script disabled behavior

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 130513 [details] [diff] [review]
copy script disabled behavior

Um, so if caps allows compilation, your patch will clobber the value set here:

1218	 *aBoolResult = ok
1219			? !JSVAL_IS_BOOLEAN(val) || (aReverseReturnResult ?
!JSVAL_TO_BOOLEAN(val) : JSVAL_TO_BOOLEAN(val))
1220			: JS_TRUE;

That's not cool at all.

I also wonder whether the success check of the caps check can be turned into an
early return if failure...

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 130513 [details] [diff] [review]
copy script disabled behavior

Actually, I think the Pop is still needed, so maybe not for the early return
suggestion.

Comment 4

[timeless]

Attachment: copy script disabled behavior only when caps fails

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 130586 [details] [diff] [review]
copy script disabled behavior only when caps fails

Better.  r=caillon.

The only thing I would consider is swapping the if around to have the failure
case first, and the success case in the else.  The closer the else is to the
if, the easier it is to tell what the else means.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 130586 [details] [diff] [review]
copy script disabled behavior only when caps fails

sr=jst

Comment 7

[timeless]

checked in

Comment 8

[Harshal Pradhan]

*** Bug 162417 has been marked as a duplicate of this bug. ***