123371 - '\n' after a JS function call name before its argument list's left paren asserts

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[kinmoz]

So I ran across this nugget when trying to run an attatchment to a bug I was
looking at ...

If there's a newline character between a JS function call name and it's parens:


    <input type="button" value="Click Me" onClick="myJSFunc
    ();">


We get the following assertion in the console, when the JS is called:


    Assertion failure: _line != 0, at x:\mozilla\js\src\jsemit.c:1822


and then an "Abnormal Program Termination" dialog. I'll attatch a test case.

Here's the stack trace to the assertion:


NTDLL! 77fa018c()
js_EmitTree(JSContext * 0x034120d8, JSCodeGenerator * 0x0012e688, JSParseNode *
0x03b137e0) line 1822 + 126 bytes
js_EmitTree(JSContext * 0x034120d8, JSCodeGenerator * 0x0012e688, JSParseNode *
0x03b13810) line 3587 + 20 bytes
js_EmitTree(JSContext * 0x034120d8, JSCodeGenerator * 0x0012e688, JSParseNode *
0x03a74a90) line 3158 + 20 bytes
Statements(JSContext * 0x034120d8, JSTokenStream * 0x03b13468, JSTreeContext *
0x0012e688) line 926 + 61 bytes
FunctionBody(JSContext * 0x034120d8, JSTokenStream * 0x03b13468, JSFunction *
0x03b16cb8, JSTreeContext * 0x0012e688) line 555 + 17 bytes
js_CompileFunctionBody(JSContext * 0x034120d8, JSTokenStream * 0x03b13468,
JSFunction * 0x03b16cb8) line 602 + 24 bytes
JS_CompileUCFunctionForPrincipals(JSContext * 0x034120d8, JSObject * 0x03a10688,
JSPrincipals * 0x03a516b8, const char * 0x0012e854, unsigned int 1, const char *
* 0x023d9768 char const * *  gEventArgv, const unsigned short * 0x0012e918,
unsigned int 11, const char * 0x00000000, unsigned int 0) line 3168 + 17 bytes
nsJSContext::CompileEventHandler(nsJSContext * const 0x033e78e0, void *
0x03a10688, nsIAtom * 0x01584b58, const nsAString & {...}, int 0, void * *
0x0012e9b0) line 909 + 91 bytes
nsEventListenerManager::CompileEventHandlerInternal(nsIScriptContext *
0x033e78e0, nsISupports * 0x036c7568, nsIAtom * 0x01584b58, nsListenerStruct *
0x03a77af8, unsigned int 4) line 1100 + 46 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03a77af8,
nsIDOMEvent * 0x03a51140, nsIDOMEventTarget * 0x0385f2a0, unsigned int 4,
unsigned int 7) line 1153 + 49 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x036c78b8,
nsIPresContext * 0x03b15d78, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x0012f124,
nsIDOMEventTarget * 0x0385f2a0, unsigned int 7, nsEventStatus * 0x0012f828) line
1373 + 36 bytes
nsGenericElement::HandleDOMEvent(nsGenericElement * const 0x036c7568,
nsIPresContext * 0x03b15d78, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x0012f124,
unsigned int 1, nsEventStatus * 0x0012f828) line 1652
nsHTMLInputElement::HandleDOMEvent(nsHTMLInputElement * const 0x036c7568,
nsIPresContext * 0x03b15d78, nsEvent * 0x0012f48c, nsIDOMEvent * * 0x00000000,
unsigned int 1, nsEventStatus * 0x0012f828) line 1190 + 29 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f48c, nsIView * 0x00000000,
unsigned int 1, nsEventStatus * 0x0012f828) line 6000 + 44 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x0373ed40, nsEvent *
0x0012f48c, nsIFrame * 0x03aa9094, nsIContent * 0x036c7568, unsigned int 1,
nsEventStatus * 0x0012f828) line 5969 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const
0x03a39108, nsIPresContext * 0x03b15d78, nsMouseEvent * 0x0012f92c,
nsEventStatus * 0x0012f828) line 2463 + 63 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x03a39110,
nsIPresContext * 0x03b15d78, nsEvent * 0x0012f92c, nsIFrame * 0x03aa9094,
nsEventStatus * 0x0012f828, nsIView * 0x03b58148) line 1544 + 28 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f92c, nsIView * 0x03b58148,
unsigned int 1, nsEventStatus * 0x0012f828) line 6020 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x0373ed44, nsIView * 0x03b58148,
nsGUIEvent * 0x0012f92c, nsEventStatus * 0x0012f828, int 0, int & 1) line 5923 +
25 bytes
nsView::HandleEvent(nsView * const 0x03b58148, nsGUIEvent * 0x0012f92c, unsigned
int 0, nsEventStatus * 0x0012f828, int 0, int & 1) line 390
nsView::HandleEvent(nsView * const 0x03b57d98, nsGUIEvent * 0x0012f92c, unsigned
int 0, nsEventStatus * 0x0012f828, int 0, int & 1) line 347
nsView::HandleEvent(nsView * const 0x036b8b00, nsGUIEvent * 0x0012f92c, unsigned
int 0, nsEventStatus * 0x0012f828, int 1, int & 1) line 347
nsViewManager::DispatchEvent(nsViewManager * const 0x03a84dc0, nsGUIEvent *
0x0012f92c, nsEventStatus * 0x0012f828) line 1900
HandleEvent(nsGUIEvent * 0x0012f92c) line 83
nsWindow::DispatchEvent(nsWindow * const 0x03b57e64, nsGUIEvent * 0x0012f92c,
nsEventStatus & nsEventStatus_eIgnore) line 850 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f92c) line 871
nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4537 +
21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 4789
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 9175278, long *
0x0012fd20) line 3429 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x00130388, unsigned int 514, unsigned int 0, long
9175278) line 1115 + 27 bytes
USER32! 77e12e98()
USER32! 77e130e0()
USER32! 77e15824()
nsAppShellService::Run(nsAppShellService * const 0x010c7978) line 308
main1(int 1, char * * 0x003c7380, nsISupports * 0x00000000) line 1285 + 32 bytes
main(int 1, char * * 0x003c7380) line 1625 + 37 bytes
mainCRTStartup() line 338 + 17 bytes

Comment 1

[kinmoz]

Attachment: JS Test Case (Makes Moz Exit)

Comment 2

[Phil Schwartau]

This does not seem to be a problem in the standalone JS engine.
For example, the following script prints out "HELLO" just fine:

function f
(s)
{
print(s);
}

f('HELLO');



In addition, the following javascript: URL works fine in the browser:

      javascript: eval('function f\n(s) {alert(s);}'); f('HELLO');

Using Mozilla binary 20020125xx on WinNT.

Comment 3

[Phil Schwartau]

Gee, the testcase provided above also seems to WORKFORME using
Mozilla trunk binaries 20020125xx on WinNT, Linux, and Mac9.1.

I will try with later binaries. kin, what build are you using?

Comment 4

[kinmoz]

Your example is putting a newline between the name of the function and the paren
in the function declaration ... the bug I'm seeing is when there's a newline
between the name of the function and the paren when *calling* the function.

What about putting the return in when you *call* the function:

    javascript: eval('function f(s) {alert(s);}'); f\n('HELLO');

I get an error in the console:


Error loading URL javascript: eval('function f\n(s) {alert(s);}'); f\n('HELLO');
 : 805303f5

but no exit ... try the test case, it causes an exit.

I'm using my TRUNK debug build from lastnight.

Comment 5

[timeless]

this is from that special little helper function.

As best as i can tell, c=L-d-2, and d>2. brendan, rginda, fur and perhaps mccabe touched the helper, so ideally they can provide more info. (I know, not enough cc's, yada, my browsers are all tripping right now so i'll look at this in the evening and try to get better cc's after i read cvsblame w/ a sane browser)

Comment 6

[Phil Schwartau]

It is illegal syntax to use a bare |\n|; it should be inside quotes
as a string. Therefore the example in Comment #4 should always error:

     javascript: eval('function f(s) {alert(s);}'); f\n('HELLO');


Using my build from 2002-01-25, I get the expected error in the JS Console:

Error: illegal character
Source Code: eval('function f(s) {alert(s);}'); f\n('HELLO');


It should be this:

     javascript: eval('function f(s) {alert(s);} f\n("HELLO");');


Again, this works fine in my Jan. 25 builds; I will try today's builds.
However, I do have an up-to-date JavaScript shell, and I also have
no trouble if there is a '\n' in the middle of the call. This 
script successfully prints out 'HELLO' :


function f(s)
{
  print(s);
}

f
('HELLO');

Comment 7

[Phil Schwartau]

Using trunk binary 2002020411 on WinNT, I am not seeing any problems.

The HTML testcase given above passes, and the javascript: URLs above,
when correctly posed, also pass. No errors occur in JS Console. 

Note: this holds true whether or not I have "Show JS strict warnings"
selected in Edit > Preferences > Debug.

Comment 8

[Brendan Eich [:brendan]]

Hrm.

/be

Comment 9

[Phil Schwartau]

Moving over to my Linux box, comparing:

1. debug/optimized JavaScript shell built today
2. Mozilla trunk binary from today
3. debug build of Mozilla built today (8PM PST)


With the debug Mozilla build, the javascript: URL from Comment #2 is no
trouble, but the one from Comment #6 causes a crash after the assertion
kin has reported above. The HTML testcase crashes as well.

The debug/optimized JS shell, and the Mozilla binary, by contrast,
do not crash on any of this. This is what I experienced on WinNT.

Comment 10

[timeless]

fwiw one of the interesting fields is -1 (UL).  I took the macro and moved it 
into the function that calls it and then changed the macro use to use the 
function form.

Comment 11

[Mike Shaver (:shaver emeritus)]

When it crashes, what's the buffer being passed to
JS_CompileUCFunctionForPrincipals?  We might need a one-off microapp to test
this with.

Comment 12

[Brendan Eich [:brendan]]

Attachment: proposed fix

The bug was a failure to propagate the call (TOK_LP) list-node's first child's
pn_pos.begin state to the parent list-node; the same problem existed for new
list-nodes.  I also simplified ArgumentList to have a boolean return value,
rather than returning null on failure and its final argument on success, which
caused needless and suboptimal dataflow in callers.

Looking for r= and sr= quickly.

/be

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 67981 [details] [diff] [review]
proposed fix

sr=shaver, and I like the plan for beefed-up assertions in jsemit.c

Comment 14

[Brendan Eich [:brendan]]

Attachment: better assertion in UPDATE_LINENO_NOTES

We caught the bug fixed by the "proposed fix" attachment only because (due to
an independent bug) HTML event handlers are compiled starting with line 0, and
the old assertion in UPDATE_LINENO_NOTES, the one conditioned by delta_ != 0
and delta_ overflowing a single srcnote delta's domain, insists that line_ is
non-zero.  This *happened* to be the case, but it was coincidence.  The
jsemit.c patch here eliminates that assertion, adding a stronger one that
subsumes it and that catches any would-be-negative srcnote delta conditions.

/be

Comment 15

[Brendan Eich [:brendan]]

"This *happened* to be the case" should have a "not" before "to be" in my last
comment.

/be

Comment 16

[Robert Ginda]

Comment on attachment 67981 [details] [diff] [review]
proposed fix

r=rginda

Comment 17

[Robert Ginda]

Comment on attachment 67990 [details] [diff] [review]
better assertion in UPDATE_LINENO_NOTES

r=rginda

Comment 18

[Brendan Eich [:brendan]]

Attachment: oops, that jsemit.c patch was bad -- please review this one

I forgot how the clever old code used unsigned arithmetic to handle cases where
the line number does jump backwards when generating code (e.g., for loops where
the update part is emitted after the loop body, even though its line number is
less than or equal to any line number in the loop body).  In the interest of
clarity, and to avoid extremely long for loops from wrapping back to a small
positive line-number delta (in which case, the latent bug would bite), I made
delta_ an int32 that UPDATE_LINENO_NOTES tests < 0 explicitly.

Hope this is all clear -- ask me questions if not.

/be

Comment 19

[Brendan Eich [:brendan]]

I should hasten to add that only on Win16, which we no longer support, might
uintN be 16 bits, and there I expect a > 32768 line for loop would probably fail
to compile for lots of reasons.  The uintN difference being stored in an int32
is a little too pedantic -- int would do, or intN for symmetry.  Note that the
high-bit-tagged, 3-byte maximum source note offset limit means we can't have
2^23 or more lines in any script.

So perhaps jsemit.c's patch can be even shorter:

@@ -1768,24 +1768,23 @@
 /* A macro for inlining at the top of js_EmitTree (whence it came). */
 #define UPDATE_LINENO_NOTES(cx, cg, pn)                                       \
    JS_BEGIN_MACRO                                                            \-
       uintN _line = (pn)->pn_pos.begin.lineno;                              \-
       uintN _delta = _line - (cg)->currentLine;                             \-
       (cg)->currentLine = _line;                                            \-
       if (_delta) {                                                         \+
       uintN line_ = (pn)->pn_pos.begin.lineno;                              \+
       uintN delta_ = line_ - (cg)->currentLine;                             \+
       if (delta_ != 0) {                                                    \ 
           /*                                                                \ 
            * Encode any change in the current source line number by using   \ 
            * either several SRC_NEWLINE notes or one SRC_SETLINE note,      \ 
            * whichever consumes less space.                                 \ 
            */                                                               \-
           if (_delta >= (uintN)(2 + ((_line > SN_3BYTE_OFFSET_MASK)<<1))) { \-
               JS_ASSERT(_line != 0);                                        \-
               if (js_NewSrcNote2(cx, cg, SRC_SETLINE, (ptrdiff_t)_line) < 0)\+
           (cg)->currentLine = line_;                                        \+
           if (delta_ >= (int32)(2 + ((line_ > SN_3BYTE_OFFSET_MASK)<<1))) { \+
               if (js_NewSrcNote2(cx, cg, SRC_SETLINE, (ptrdiff_t)line_) < 0)\ 
                   return JS_FALSE;                                          \ 
           } else {                                                          \ 
               do {                                                          \ 
                   if (js_NewSrcNote(cx, cg, SRC_NEWLINE) < 0)               \ 
                       return JS_FALSE;                                      \-
               } while (--_delta != 0);                                      \+
               } while (--delta_ != 0);                                      \ 
           }                                                                 \ 
       }                                                                     \ 
   JS_END_MACRO

If you ignore the line_ => _line and similar changes to avoid invading the
standard C namespace, the only change here is to avoid storing cg->currentLine
if it doesn't need to change.

/be

Comment 20

[Brendan Eich [:brendan]]

And of course, I removed the JS_ASSERT(_line != 0);, which was always bogus (one
might start a for loop on line 0 in a broken embedding such as Mozilla's event
handler compilation code :-/.

I got antsy and checked in -- see bonsai for the better comment I wrote in
UPDATE_LINENO_NOTES.  Thanks,

/be

Comment 21

[Mike Shaver (:shaver emeritus)]

Comment on attachment 68095 [details] [diff] [review]
oops, that jsemit.c patch was bad -- please review this one

sr=shaver.  Did you run the test suite?

Comment 22

[Brendan Eich [:brendan]]

Testsuite all good.  But see bonsai for my late-night blunders, caught by
compilers with warnings.

/be

Comment 23

[Phil Schwartau]

Marking Verified FIXED. Debug builds of the browser on WinNT, Linux,
built on 2002-02-08, now function properly on

1. The HTML testcase above
2. The javascript: URL in Comment #2
3. The javascript: URL in Comment #6

Comment 24

[Bob Clary [:bc] (inactive)]

Checking in regress-123371.js;
/cvsroot/mozilla/js/tests/js1_5/Function/regress-123371.js,v  <--  regress-123371.js
initial revision: 1.1