JSSourceHandler called with wrong line length parameter

RESOLVED WORKSFORME

Status

()

Core
JavaScript Engine
P3
normal
RESOLVED WORKSFORME
17 years ago
3 years ago

People

(Reporter: brendan, Assigned: Robert Ginda)

Tracking

({js1.5})

Trunk
mozilla1.2alpha
js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
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
(Reporter)

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla0.9.1
Priority: -- → P1
Target Milestone: --- → mozilla0.9.1
(Reporter)

Comment 1

17 years ago
Created attachment 33547 [details] [diff] [review]
proposed fix, better for applying than reviewing
(Reporter)

Comment 2

17 years ago
Created attachment 33548 [details] [diff] [review]
diff -wu of last patch, for reviewing
(Reporter)

Comment 3

17 years ago
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
(Reporter)

Comment 4

17 years ago
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
Keywords: approval, patch, review
(Assignee)

Comment 5

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

Comment 6

17 years ago
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
(Reporter)

Comment 7

17 years ago
Flail smarter, not harder.

/be
Keywords: mozilla0.9.1 → mozilla0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Updated

17 years ago
Target Milestone: mozilla0.9.2 → mozilla0.9.3
*** Bug 91909 has been marked as a duplicate of this bug. ***
See also bug 91909 for additional comments
(Reporter)

Comment 10

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

/be
Keywords: mozilla0.9.2, patch, review → mozilla0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Reporter)

Comment 11

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

/be
Keywords: mozilla0.9.4 → mozilla0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Reporter)

Comment 12

17 years ago
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
Keywords: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Assignee)

Comment 13

17 years ago
I don't actually use the source handler, this bug report came from a newsgroup
posting by Olaf Meincke, <olaf.meincke@sap.com>.

Comment 14

17 years ago
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

17 years ago
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.
(Reporter)

Updated

17 years ago
Priority: P1 → P3
(Reporter)

Comment 16

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

/be
Keywords: mozilla0.9.6 → mozilla1.0
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Reporter)

Comment 17

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

/be
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

Comment 18

17 years ago
taking
Assignee: brendan → rginda
Status: ASSIGNED → NEW
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.8 → mozilla1.0

Comment 19

17 years ago
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 adt@netscape.com.  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.
Target Milestone: mozilla1.0 → mozilla1.2
(Assignee)

Updated

17 years ago
Target Milestone: mozilla1.2 → mozilla1.0
(Assignee)

Comment 20

17 years ago
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.
Target Milestone: mozilla1.0 → mozilla1.2

Updated

16 years ago
Blocks: 149801

Updated

12 years ago
QA Contact: pschwartau → general
No longer an issue, bug 588648 (http://hg.mozilla.org/mozilla-central/rev/2b9d805d77a1) removed that code from GetChar.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.