651595 - In Javascript 1.8.5, the function registered via JS_SetSourceHandler is never called

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robin Ehrlich]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; GTB6; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; OfficeLiveConnector.1.3; OfficeLivePatch.0.0)
Build Identifier: JS_SetSourceHandler is not called

The function registered by JS_SetSourceHandler should be called whenever a new script is compiled. It is never called.

Reproducible: Always

Steps to Reproduce:
Using a embedded application, register a JS_SetSourceHandler function. It will not be called as scripts are compiled.



I think the problem is in jsscan.cpp:185

    listener = cx->debugHooks->sourceHandler;
    listenerData = cx->debugHooks->sourceHandlerData;

*** add these lines ***
    if (listener) {
        listener(filename, lineno, userbuf.ptr, length, &listenerTSData, 
                 listenerData);
    }

Comment 1

[David Mandelin [:dmandelin]]

(In reply to comment #0)
> I think the problem is in jsscan.cpp:185
> 
>     listener = cx->debugHooks->sourceHandler;
>     listenerData = cx->debugHooks->sourceHandlerData;
> 
> *** add these lines ***
>     if (listener) {
>         listener(filename, lineno, userbuf.ptr, length, &listenerTSData, 
>                  listenerData);
>     }

That doesn't work, because userbuf.ptr is private, and also because it is const, and the listener function pointer type is non-const in that parameter. The const/non-const part is easily fixed, but I'm not sure what to do about userbuf.ptr being private.

It seems to me maybe we should be calling from JS_CompileUCScriptForPrincipals instead. But JS_SetSourceHandler is undocumented, so I'm not sure exactly what it's supposed to do. Robin, do you know what events are supposed to call this callback?

Comment 2

[Robin Ehrlich]

I am not sure about this not working. I added this exact code and there were no compile errors and the hook function was called at the correct time with the correct data.

The purpose of this callback is for a debugger to be informed of file names/source code that is compiled. The debugger can then maintain this source so that the correct source can be displayed when breakpoints are hit.

Comment 3

[Jim Blandy :jimb]

Bug 637572 suggests an alternative way for debuggers to keep track of the names. I'm inclined to argue that, when requested, we should just save the source code in the JSScript as well. There's no reason to make the debugger do it when we can do it more simply and accurately.

Comment 4

[David Mandelin [:dmandelin]]

(In reply to comment #2)
> I am not sure about this not working. I added this exact code and there were no
> compile errors and the hook function was called at the correct time with the
> correct data.

Did you add that code to hg tip, or to the js185 source release?

> The purpose of this callback is for a debugger to be informed of file
> names/source code that is compiled. The debugger can then maintain this source
> so that the correct source can be displayed when breakpoints are hit.

That makes Jim's proposal sound like the ideal solution.

But I imagine you'd like something a little sooner, so we should fix this for now (unless Jim's proposal is expected to be done by whatever release vehicle you would use this in, but I don't know either of those dates). It seems to me that Compiler::compileScript is where we should call out from here.

Can you tell me what sources you are working from/can use, so we can figure out how to get a fix out to you?

Comment 5

[Robin Ehrlich]

> Did you add that code to hg tip, or to the js185 source release?
The js185 source release. Our company only wants to release to our customers official Javascript released engines. The fix I have made works fine for now so I don't need an immediate release unless a new official js185 release will be made in which case a fix would be desired. The code I added attempted to duplicate the code in the 1.7 release as much as possible.

As a side note, I also submitted bug 651599. In order for us to use js 1.8.5 both of these problems need to be resolved.

Comment 6

[Paul Biggar]

Wes, are we planning a new source release to coincide with Firefox 5 (in 8ish weeks I think)?

This fix looks easy to do, so not a bad plan to fix it anyway.

Comment 7

[Paul Biggar]

Was looking for a reason this was removed. It was removed as part of bug 588648, but the bug comments don't mention why, so I'm assuming an oversight. njn?

Comment 8

[Paul Biggar]

(In reply to comment #2)
> I am not sure about this not working. I added this exact code and there were
> no compile errors and the hook function was called at the correct time with
> the correct data.

Looks like this was changed in bug 638034 (constmess added at least):

changeset:   63053:3035bb782013
user:        Nicholas Nethercote <[e-mail]>
date:        Tue Mar 08 16:10:51 2011 -0800
summary:     Bug 638034 - Make scanning safer.  r=brendan.

Comment 9

[Wesley W. Garland]

Paul -- I have been *considering* doing a new JS 1.8.5 source release.  I'm not sure yet what this release should look like, however.  I want to get the OS/2 build fix, Sparc Jaegermonkey, and any small, self-contained API bugs fixed.

I'm not sure if upgrading all the way to JS.current is the right plan, though -- if there is breaking API this soon after the JS 1.8.5 upheaval, it will cost us in embedder mind-space. Has there been API breakage? How about ABI breakage?  I haven't done a survey yet.

I think the right target for the stand-alone target is a fast, bug-free engine with a stablish API/ABI, whereas the browser's target is the fastest possible bug-free engine, without significant concern for API stability.

Robin, thanks for your bug reports and feedback in dev-tech-jseng.

Comment 10

[Wesley W. Garland]

dmandelin:

> But JS_SetSourceHandler is undocumented, so I'm not sure 
> exactly what it's supposed to do

from http://mxr.mozilla.org/mozilla/source/js/jsd/jsdb/jsdb.c#384 :
/*
 * This facilitates sending source to JSD (the debugger system) in the shell
 * where the source is loaded using the JSFILE hack in jsscan. The function
 * below is used as a callback for the jsdbgapi JS_SetSourceHandler hook.
 * A more normal embedding (e.g. mozilla) loads source itself and can send
 * source directly to JSD without using this hook scheme.
 */

FWIW this will bite me too, and anybody else trying to use a debugger that was based on that code (JS debugger written in JS on top of jsd_high.c which reflects JSD into JS).

Comment 11

[Paul Biggar]

I don't really see the reason to have both listenerData and listenerTSData. They both seem to serve the same purpose. Can we reduce them to just one?

Comment 12

[Paul Biggar]

Attachment: Fix current behaviour

This looks roughly like it fixes it. Robin?

As dmandelin says, this should probably go in Compiler::compileScript. However, this is the only place I can put it now without changing member visibility.

As I see it, we have both listenerTSData and listenerData, one of which is redundant. It seems like we should get rid of listenerTSData; can we mess with the API?

(If not, I'd like to move listenerTSData into JSDebugHooks, but the constness of cx->DebugHooks prevents that. The solution would be to wrap listenerData and listenerTSData in a struct that goes in JSDebugHooks, and I'm undecided as to whether that makes things better or worse).

I'd like to add a comment for JSSourceHandler, but don't know what to say about listenerTSData. Similarly, this needs a test once I understand what it's supposed to do.

Comment 13

[Robin Ehrlich]

> This looks roughly like it fixes it. Robin?
Yes.

> As I see it, we have both listenerTSData and listenerData, one of which is 
> redundant. It seems like we should get rid of listenerTSData; can we mess 
> with the API?

I agree that it seems that the two parameters are redundant, but the code in jsd requires them. I suggest leaving the API unchanged.

Comment 14

[Paul Biggar]

Comment on attachment 534000 [details] [diff] [review]
Fix current behaviour

Of course we can't change the API, I don't know what I was thinking.

(Although I did add a const it, that shouldn't be a problem).

The existing code here is a tad ugly, but since we're in the process of making jsd2, I don't see a reason to do any more than the minimum to make this work.

Comment 15

[David Mandelin [:dmandelin]]

Comment on attachment 534000 [details] [diff] [review]
Fix current behaviour

(In reply to comment #14)
> Comment on attachment 534000 [details] [diff] [review] [review]
> Fix current behaviour
> 
> The existing code here is a tad ugly, but since we're in the process of
> making jsd2, I don't see a reason to do any more than the minimum to make
> this work.

Agreed.

Comment 16

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/b9a23bc89996

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

hg.mozilla.org/mozilla-central/rev/b9a23bc89996