79485 - JSSourceHandler called with wrong line length parameter

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P3)

Description

[Brendan Eich [:brendan]]

See news://news.mozilla.org/9d95kt%24e36%241%40news1.wdf.sap-ag.de while it
lasts.  Better URL welcome.

Patch coming up, also picks some unrelated nits in jsscan.c.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix, better for applying than reviewing

Comment 2

[Brendan Eich [:brendan]]

Attachment: diff -wu of last patch, for reviewing

Comment 3

[Brendan Eich [:brendan]]

I left the listener call still passing ts->userbuf.ptr, but the len actual arg
it passes counts the canonical-newline-terminated characters in ts->linebuf.  So
for long userbuf lines, multiple listener calls are possible.  And for lines
terminated by \r\n in userbuf, len will count only one terminator character.  Is
this wrong?  I could farble len a bit to compute the user line length (if that
line fits in linebuf).

Comments from you debugger guys welcome.

/be

Comment 4

[Brendan Eich [:brendan]]

Wait, I was on crack: userbuf contains len chars, and if the user-input line
ends with \r\n, that's counted by len and included in the buffer prefix seen by
ts->listener.  So I'm ready for r=rginda and sr=jband.

/be

Comment 5

[Robert Ginda]

We're passing userbuf.ptr to the listener via jschar *, instead of const jschar
*.  Other than that, r=rginda.

Comment 6

[John Bandhauer]

OK. I'm stupid. I'm still trying to figure out what was wrong in the first 
place. I don't understand the assertion that "JS_CompileScript calls the 
callback for each line". This code was originally intended to send chunks of 
text to the listener without regard for sending each line separately. I can't 
see that it *does* send line by line. 

I just now wrote a test case that shows the old code did not seem to do exactly 
what was said above. But still did not do the right thing. My test also shows 
brendan's patch not working right...

Here's the test (easy to build if saved as js.c):

/**************************************************************************/
#include "jsapi.h"
#include "jsdbgapi.h"
#include <stdio.h>

void handler(const char *filename, uintN lineno, 
             jschar *str, size_t length, 
             void **listenerTSData, void *closure)
{
    size_t i;

    printf("handler called for [%s,%d] with length: %d and text: \n", 
           filename,lineno, length);
    for(i = 0; i < length; i++)
        putchar((char)str[i]);
    
    putchar('\n');
}             

static JSBool
global_resolve(JSContext *cx, JSObject *obj, jsval id)
{
    JSBool resolved;
    return JS_ResolveStandardClass(cx, obj, id, &resolved);
}

static JSClass global_class = {
    "global", 0,
    JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,
    JS_EnumerateStub, global_resolve,   JS_ConvertStub,   JS_FinalizeStub
};

int main()
{
    const char buffer1[] = "Some\nmultiline text\nthat covers many\nlines.\n";       
    const char buffer2[] = "Some\nmultiline text\nthat covers many\nlines.";       
    JSRuntime *rt;
    JSContext *cx;
    JSObject *glob;
    
    rt = JS_NewRuntime(8L * 1024L * 1024L);
    cx = JS_NewContext(rt, 4048);

    glob = JS_NewObject(cx, &global_class, NULL, NULL);
    JS_SetGlobalObject(cx, glob);

    JS_SetSourceHandler(rt, handler, NULL);

    JS_CompileScript(cx, glob, buffer1, sizeof(buffer1)-1, "buffer1", 1);
    printf("----------------------------------------------------------\n");
    JS_CompileScript(cx, glob, buffer2, sizeof(buffer2)-1, "buffer2", 1);

    JS_DestroyContext(cx);
    JS_DestroyRuntime(rt);
    JS_ShutDown();
    return 0;
}
/**************************************************************************/

The old code outputs:

handler called for [buffer1,1] with length: 44 and text:
Some
multiline text
that covers many
lines.

handler called for [buffer1,2] with length: 39 and text:
multiline text
that covers many
lines.

----------------------------------------------------------
handler called for [buffer2,1] with length: 43 and text:
Some
multiline text
that covers many
lines.
handler called for [buffer2,2] with length: 38 and text:
multiline text
that covers many
lines.

With brendan's patch I see:

handler called for [buffer1,1] with length: 5 and text:
Some

handler called for [buffer1,2] with length: 15 and text:
multiline text

----------------------------------------------------------
handler called for [buffer2,1] with length: 5 and text:
Some

handler called for [buffer2,2] with length: 15 and text:
multiline text

Comment 7

[Brendan Eich [:brendan]]

Flail smarter, not harder.

/be

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 91909 has been marked as a duplicate of this bug. ***

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

See also bug 91909 for additional comments

Comment 10

[Brendan Eich [:brendan]]

This little bug should get fixed soon, or I will renounce it.

/be

Comment 11

[Brendan Eich [:brendan]]

Not holding up 0.9.4 for this, but I'll work up a better patch soon.

/be

Comment 12

[Brendan Eich [:brendan]]

rginda, is this still bugging you?  I've been busy elsewhere, and would like to
push it out even further, or give it away if someone else will fix it right
(I've tried and failed once already).

/be

Comment 13

[Robert Ginda]

I don't actually use the source handler, this bug report came from a newsgroup
posting by Olaf Meincke, <[e-mail]>.

Comment 14

[olaf.meincke]

I applied the proposed bug fix and since haven't encountered any problems with 
the source handler. From my point of view the problem is fixed.

Comment 15

[John Bandhauer]

My posting may not have been very clear. My point was that brendan's fix (while 
it may work better for you) is nonetheless broken and should not be checked in. 
Still, we now have code that is headed in the right direction and a half-assed 
test framework available for use by whoever comes up to bat next.

Comment 16

[Brendan Eich [:brendan]]

Who wants to bat next?  Please swing away, reassign to yourself and cc: me.

/be

Comment 17

[Brendan Eich [:brendan]]

I'm a terrible owner for this bug.  Rob, you wanna take a whack at it?

/be

Comment 18

[Robert Ginda]

taking

Comment 19

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 20

[Robert Ginda]

Pushing out.  Venkman doesn't need this, I don't see how I'm going to find time
to get to it before 1.0.

Comment 21

[André Bargull [:anba]]

No longer an issue, bug 588648 (http://hg.mozilla.org/mozilla-central/rev/2b9d805d77a1) removed that code from GetChar.