using addEventListener() for keypress handler prevents JavaScript error propagation

RESOLVED FIXED in mozilla1.6beta

Status

()

Core
DOM: Events
P2
major
RESOLVED FIXED
15 years ago
7 years ago

People

(Reporter: Torsten Scheck, Assigned: jst)

Tracking

Trunk
mozilla1.6beta
Points:
---
Bug Flags:
blocking1.6b +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20030925 Firebird/0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20030925 Firebird/0.7

I use addEventListener() to create a keypress handler which catches 'Enter' and
focuses the next field. For the whole script related to the onblur-event no
JavaScript messages reach the JavaScript console. This is a big problem as I
execute much validation and supplementation code in this phase.

A test case will be uploaded.

Here is a code example: 

function keypresshandler(e) {
  // if Enter-Key is pressed, focus field2
  if (e.keyCode == 13) document.getElementById("field2").focus();
}
document.addEventListener('keypress', keypresshandler, true);

var node = document.getElementById("field1");
node.onblur = function () { 
  alert("before");
  // following undefined function call
  //    creates an entry in the JS-Console, if field1 was left with 'Tab'
  //    remains unnoticed, if field1 was left with 'Enter'
  xyz();
  // script execution stops here; no second message appears
  alert("after");
}


Reproducible: Always

Steps to Reproduce:
1.
2.
3.
(Reporter)

Comment 1

15 years ago
Created attachment 134697 [details]
test case html file
Not sure if I can help; maybe bryner or jst can.

/be
(Assignee)

Comment 3

15 years ago
Created attachment 136028 [details] [diff] [review]
Don't drop JS exception when rolling back from nested calls through XPConnect
(Assignee)

Updated

15 years ago
Attachment #136028 - Flags: superreview?(brendan)
Attachment #136028 - Flags: review?(caillon)
(Assignee)

Updated

15 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → mozilla1.6beta
(Assignee)

Comment 4

15 years ago
Taking.
Assignee: events → jst
Status: ASSIGNED → NEW
Comment on attachment 136028 [details] [diff] [review]
Don't drop JS exception when rolling back from nested calls through XPConnect

Cool, glad this got fixed.  If Chris agrees, I think we should get the patch
into 1.6b.

/be
Attachment #136028 - Flags: superreview?(brendan) → superreview+
I'll let another driver + the patch, once it has r=.

/be
Flags: blocking1.6b+
Comment on attachment 136028 [details] [diff] [review]
Don't drop JS exception when rolling back from nested calls through XPConnect

>+    if (!ok) {
>+      // Tell XPConnect that something went wrong.
>+      SetXPCExpceptionWasThrown();
>+    }
>+
>     *aBoolResult = ok
>                    ? !JSVAL_IS_BOOLEAN(val) || (aReverseReturnResult ? !JSVAL_TO_BOOLEAN(val) : JSVAL_TO_BOOLEAN(val))
>                    : JS_TRUE;

Nit: how about combining these to:

  if (ok) {
    *aBoolResult = JS_TRUE;
  } else {
    SetXPCExpceptionWasThrown();
    *aBoolResult = !JSVAL_IS_BOOLEAN(val) || (aReverseReturnResult ?
					      !JSVAL_TO_BOOLEAN(val) :
					      JSVAL_TO_BOOLEAN(val));
  }


Other than that, I'd say this patch is cool.  We may want to
SetExceptionWasThrown from within caps in a few more places too.  It doesn't do
it everywhere either I don't think.  But yeah, this patch should do the job for
this bug.  r=caillon.
Attachment #136028 - Flags: review?(caillon) → review+
caillon's suggestion for testing ok once got me thinking on the horrors of
aReverseReturnValue:

  if (ok) {
    *aBoolResult = JS_TRUE;
  } else {
    SetXPCExpceptionWasThrown();
    *aBoolResult = !JSVAL_IS_BOOLEAN(val) ||
                   (aReverseReturnResult ^ JSVAL_TO_BOOLEAN(val));
  }

This works for boolean values because (a ? !b : b) == (a ^ b).

/be
One more nit:

    SetXPCExpceptionWasThrown();
    *aBoolResult = !JSVAL_IS_BOOLEAN(val) ||
                   (aReverseReturnResult ^ JSVAL_TO_BOOLEAN(val));

Calls a static function, then uses locals, possibly requiring stores to save
registers in their stack homes.  Transpose these two statements for better code
generation, in general.

/be
(Assignee)

Comment 10

15 years ago
Comment on attachment 136028 [details] [diff] [review]
Don't drop JS exception when rolling back from nested calls through XPConnect

I'll leave that as-is for now (and I think you got the logic wrong there) and
I'll file a new bug on eliminating aReverseReturnResult completely.

Oh, and I fixed the typo in SetXPCExpceptionWasThrown() too :-)

Requesting approval.
Attachment #136028 - Flags: approval1.6b?

Comment 11

15 years ago
Comment on attachment 136028 [details] [diff] [review]
Don't drop JS exception when rolling back from nested calls through XPConnect

a=asa (on behalf of drivers) for checkin to 1.6 beta.
Attachment #136028 - Flags: approval1.6b? → approval1.6b+
(Assignee)

Comment 12

15 years ago
Fixed.

I filed bug 226462 on eliminating the silly aReverseReturnResult argument...
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
jst: you mean caillon's logic?  My XOR reduction is correct (and righteous! ;-),
but I was going off his comment, which I now see did reverse the ok test.

/be
(Assignee)

Comment 14

15 years ago
Yup, caillon got it backwards, and I didn't even see notice your comments before
I mentioned that :-( But your ^ was indeed righteous :-)
Whoops yeah, it was late when I wrote that comment.  Sorry for the mixup, but
you got the idea ;-)

Comment 16

15 years ago
This checkin might have caused bug 226617.

Comment 17

15 years ago
Hi Johnny, this checkin caused a regression in mailnews. See Bug #227013. 

In short after this change the following no longer works:

  window.openDialog("chrome://messenger/content/FilterEditor.xul",
"FilterEditor", "chrome,modal,titlebar,resizable,centerscreen", args);

  dump('post refresh call\n');
  if ("refresh" in args && args.refresh)
    refresh();

Without this patch, I see the dump statement get executed. With this patch, the
script never resumes after the return from window.openDialog.

I have not quite figured out the relationship as to why this is so. 
I commented in bug 227013 comment 3 and cc'd jst and myself on that bug.

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