use of uninitialised variable in jsd_step.c:_callHook

ASSIGNED
Assigned to

Status

Other Applications
Venkman JS Debugger
ASSIGNED
16 years ago
10 years ago

People

(Reporter: bbaetz, Assigned: Robert Ginda)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
hookresult (in jsd_step.c:_callHook) is never initialised to a default value,
and if JS_GetFrameScript returns NULL, then this uninited value will be returned.

valgrind is picking this up in js_invoke, since the uninitialised return value
is used as a condition for the jsd_FunctionCallHook value, which ends up as
hookData var in line 826 to be uninitialised.

I have no idea whether or not the default val should be JS_TRUE or JS_FALSE, though.

I also noticed that the JSD_IS_PROFILE_ENABLED branch in _callHook seems to
ignore the return value from jsd_CallCallHook, while the non-profile branch
doesn't. I don't know if this is intentional or not.
(Assignee)

Comment 1

16 years ago
The default should be JS_FALSE.

JS_FALSE means that we don't want to be called again when we |return| from this
stack frame.  If we have profiling enabled, we always want to be called for
|return| (regardless of what our hook says), so that we can mark the call duration.
Status: NEW → ASSIGNED
(Reporter)

Comment 2

16 years ago
If profiling isn't enabled, and we don't have a hook, why is hookresult set to
JS_TRUE?

Also, if profiling is enabled, but pdata is null, do you still want JS_TRUE to
be returned? I'm guessing no, unless a script can somehow add profile data to
itsself during this call (in which case you'll be missing the start info anyway,
so...
(Assignee)

Comment 3

16 years ago
We return JS_TRUE in that case, because a hook might be created *during* the
call.  If we say "don't call me on function return", then that newly added hook
has no chance to see the return.  This is a specific problem with venkman, where
"step out" registers a call hook, and expects to be called for the return of the
function currently being debugged.
(Reporter)

Comment 4

16 years ago
Created attachment 96934 [details] [diff] [review]
try this

OK, try this. Profiling a reload of www.google.com appears to work both before
and after this patch.

Is the comment I added correct, then?
(Assignee)

Comment 5

16 years ago
This doesn't address comment 13 at all.  We CAN'T return false by default.  If
we return false on call, and the debugger stops IN THE CALL, and the user
selects "Step Out", we need to hear about the function return.

If we returned false on the call, we'd never hear the return.  We should only
return false if the call hook says to.
Product: Core → Other Applications

Updated

10 years ago
QA Contact: caillon → venkman
You need to log in before you can comment on or make changes to this bug.