Last Comment Bug 217562 - UMR in nsJSEventListener::HandleEvent after mContext->CallEventHandler
: UMR in nsJSEventListener::HandleEvent after mContext->CallEventHandler
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: timeless
: Hixie (not reading bugmail)
Mentors:
: 162417 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-08-28 00:30 PDT by timeless
Modified: 2003-12-23 05:55 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
copy script disabled behavior (529 bytes, patch)
2003-08-28 00:37 PDT, timeless
caillon: review-
Details | Diff | Review
copy script disabled behavior only when caps fails (1.33 KB, patch)
2003-08-29 02:54 PDT, timeless
caillon: review+
jst: superreview+
Details | Diff | Review

Description timeless 2003-08-28 00:30:03 PDT
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 2003-08-28 00:37:35 PDT
Created attachment 130513 [details] [diff] [review]
copy script disabled behavior
Comment 2 Christopher Aillon (sabbatical, not receiving bugmail) 2003-08-29 02:36:59 PDT
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) 2003-08-29 02:42:30 PDT
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 2003-08-29 02:54:16 PDT
Created attachment 130586 [details] [diff] [review]
copy script disabled behavior only when caps fails
Comment 5 Christopher Aillon (sabbatical, not receiving bugmail) 2003-08-29 03:00:23 PDT
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, jst@mozilla.com) 2003-08-29 16:45:54 PDT
Comment on attachment 130586 [details] [diff] [review]
copy script disabled behavior only when caps fails

sr=jst
Comment 7 timeless 2003-09-10 19:21:43 PDT
checked in
Comment 8 Harshal Pradhan 2003-12-23 05:55:34 PST
*** Bug 162417 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.