The default bug view has changed. See this FAQ.

UMR in nsJSEventListener::HandleEvent after mContext->CallEventHandler

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.33 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
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)
(Assignee)

Comment 1

14 years ago
Created attachment 130513 [details] [diff] [review]
copy script disabled behavior
(Assignee)

Updated

14 years ago
Attachment #130513 - Flags: superreview?(jst)
Attachment #130513 - Flags: review?(caillon)
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...
Attachment #130513 - Flags: review?(caillon) → review-
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.
(Assignee)

Comment 4

14 years ago
Created attachment 130586 [details] [diff] [review]
copy script disabled behavior only when caps fails
Attachment #130513 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #130586 - Flags: superreview?(jst)
Attachment #130586 - Flags: review?(caillon)
(Assignee)

Updated

14 years ago
Attachment #130513 - Flags: superreview?(jst)
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.
Attachment #130586 - Flags: review?(caillon) → review+
Comment on attachment 130586 [details] [diff] [review]
copy script disabled behavior only when caps fails

sr=jst
Attachment #130586 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 7

14 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 8

14 years ago
*** Bug 162417 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.