224549 - using addEventListener() for keypress handler prevents JavaScript error propagation

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P2)

Description

[Torsten Scheck]

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.

Comment 1

[Torsten Scheck]

Attachment: test case html file

Comment 2

[Brendan Eich [:brendan]]

Not sure if I can help; maybe bryner or jst can.

/be

Comment 3

[Johnny Stenback (:jst)]

Attachment: Don't drop JS exception when rolling back from nested calls through XPConnect

Comment 4

[Johnny Stenback (:jst)]

Taking.

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Brendan Eich [:brendan]]

I'll let another driver + the patch, once it has r=.

/be

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[Brendan Eich [:brendan]]

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

Comment 10

[Johnny Stenback (:jst)]

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.

Comment 11

[Asa Dotzler [:asa]]

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.

Comment 12

[Johnny Stenback (:jst)]

Fixed.

I filed bug 226462 on eliminating the silly aReverseReturnResult argument...

Comment 13

[Brendan Eich [:brendan]]

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

Comment 14

[Johnny Stenback (:jst)]

Yup, caillon got it backwards, and I didn't even see notice your comments before
I mentioned that :-( But your ^ was indeed righteous :-)

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Whoops yeah, it was late when I wrote that comment.  Sorry for the mixup, but
you got the idea ;-)

Comment 16

[Jesse Ruderman]

This checkin might have caused bug 226617.

Comment 17

[Scott MacGregor]

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. 

Comment 18

[Brendan Eich [:brendan]]

I commented in bug 227013 comment 3 and cc'd jst and myself on that bug.

/be