164394 - use of uninitialised variable in jsd_step.c:_callHook

Status: RESOLVED INCOMPLETE

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Bradley Baetz (:bbaetz)]

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.

Comment 1

[Robert Ginda]

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.

Comment 2

[Bradley Baetz (:bbaetz)]

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...

Comment 3

[Robert Ginda]

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.

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: 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?

Comment 5

[Robert Ginda]

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.

Comment 6

[BMO Automation]

Component is obsolete so resolving bugs as INCOMPLETE