121318 - Venkman should disable the target window when stopped at a breakpoint.

Status: VERIFIED FIXED

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

Description

[Robert Ginda]

When Bug 121171 is fixed, it will be possible to disable an nsIScriptContext. 
The debugger service needs to provide a way for clients to access that
functionality.

We don't expose any notion of a js context to the caller, I'd make one, but I
think the lifetime issues would be hard to work out.  In the interim, I've added
a scriptsEnabled property to jsdIStackFrame, which the Venkman UI will clear
when a breakpoint is hit.

Native patch coming up, XUL to follow RSN.

Comment 1

[Robert Ginda]

Attachment: Native changes, add |scriptsEnabled| property to jsdIStackFrame

Comment 2

[Robert Ginda]

Attachment: native changes, minus useless include

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 66045 [details] [diff] [review]
native changes, minus useless include

sr=jst

Comment 4

[Robert Ginda]

Attachment: native patches, round three

New patch implements jsdIContext to represent js contexts.

If the context is a DOM Window we're going to have to locate the parent and
disable *it* too.  I'm going to need to be able enumerate the available
contexts for something like that.  This opens up a few other neat features too.

Comment 5

[Robert Ginda]

Attachment: UI changes

Disable executionContext while stopped at a breakpoint.  Removed old
console._stopLevel (we don't do nested breakpoints anymore) too.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 66580 [details] [diff] [review]
native patches, round three

>+jsdIContext *
>+jsdContext::FromPtr (JSDContext *aJSDCx, JSContext *aJSCx)
>+{
>+    if (!aJSDCx || !aJSCx ||
>+        !(JS_GetOptions(aJSCx) & JSOPTION_PRIVATE_IS_NSISUPPORTS))
>+    {
>+        return nsnull;
>+    }
>+    
>+    nsCOMPtr<jsdIContext> jsdicx;
>+    jsdIEphemeral *eph = jsds_FindEphemeral (&gLiveContexts,
>+                                             NS_STATIC_CAST(void *, aJSCx));

Why not use an nsCOMPtr and have jsds_FindEphemeral return an already_AddRefed?
 Or at least getter_AddRefs?

This manual ref-counting is so 1998 :-).  It may work for you; it won't scale
over others and years.

>@@ -1502,7 +1691,7 @@
> NS_IMETHODIMP
> jsdValue::GetJsFunctionName(char **_rval)
> {
>-    ASSERT_VALID_VALUE;
>+    ASSERT_VALID_EPHEMERAL;
>     *_rval =
>         ToNewCString(nsDependentCString(JSD_GetValueFunctionName(mCx, mValue)));
>     return NS_OK;

return NS_ERROR_OUT_OF_MEMORY if ToNewCString fails.  More such later, not
included below.

>@@ -1519,7 +1708,7 @@
> NS_IMETHODIMP
> jsdValue::GetDoubleValue(double *_rval)
> {
>-    ASSERT_VALID_VALUE;
>+    ASSERT_VALID_EPHEMERAL;
>     double *dp = JSD_GetValueDouble (mCx, mValue);
>     if (dp)
>         *_rval = *dp;

Same here -- isn't a null *_rval after an NS_OK return a crash?

rs=[e-mail] on the rest, fix the above where necessary.  What's the
average number of ephemerals on the list(s)?  Maximum number?  Just wondering
whether O(n^2) growth of linear search will bite.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 66045 [details] [diff] [review]
native changes, minus useless include

The use of NS_ERROR_NO_INTERFACE is a little "cute" -- that's really a code
specific to QI.  But I guess you are saying that a JSContext whose data is not
an nsISupports* has failed a QI.

r=[e-mail]

/be

Comment 8

[Robert Ginda]

>Why not use an nsCOMPtr and have jsds_FindEphemeral return an already_AddRefed?
> Or at least getter_AddRefs?

If I change 
+    jsdIEphemeral *eph = jsds_FindEphemeral (&gLiveContexts,
+                                             NS_STATIC_CAST(void *, aJSCx));
to
+    nsCOMPtr<jsdIEphemeral> eph =
+         getter_Addrefs(jsds_FindEphemeral (&gLiveContexts,
+                                            NS_STATIC_CAST(void *, aJSCx)));
I assert at nsComPtr.h:501, which thinks a QueryInterface is in order.
I'm not sure why |query_result| is different from the return value of
jsds_FindEphemeral, but I suspect it has something to do with

+    mLiveListEntry.value = this;

which is the pointer eventually returned by jsds_FindEphemeral.  Why would
this be bad?

>This manual ref-counting is so 1998 :-).  It may work for you; it won't scale
>over others and years.

Agreed.

>return NS_ERROR_OUT_OF_MEMORY if ToNewCString fails.  More such later, not
>included below.

ok, I'll fix them when I deal with bug 121925.

 NS_IMETHODIMP
 jsdValue::GetDoubleValue(double *_rval)
 {
-    ASSERT_VALID_VALUE;
+    ASSERT_VALID_EPHEMERAL;
     double *dp = JSD_GetValueDouble (mCx, mValue);
     if (dp)
         *_rval = *dp;

>Same here -- isn't a null *_rval after an NS_OK return a crash?

null *_rval after NS_OK means "I'm returning null".  I broke in gdb and
set *_rval to null before returning to be sure.  This code shouldn't
return null though, I'll make it an NS_ERROR_FAILURE.

> rs=[e-mail] on the rest, fix the above where necessary.  What's the
> average number of ephemerals on the list(s)?  Maximum number?  Just wondering
> whether O(n^2) growth of linear search will bite.

As it stands, only a single jsdContext wrapper is alive at any given time.
With just one browser window open, plus venkman and a small xul app, there are
58 contexts.  Opening a second navigator window brings the total number of
contexts to 88.
  8 [object Window]
 27 [object nsXULPrototypeScript compilation scope]
 53 [object nsXBLPrototypeScript compilation scope]

I just hacked jsdValue::FromPtr to make use of jsds_FindEphemeral.  This used to
create a new jsdValue without checking for an existing wrapper, and could
result in almost 2000 jsdValues after giving the object inspector a workout.
With the jsds_FindEphemeral hack, I hit a max of about 230 jsdValues.  Of
course, a user could inspect an array with thousands of unique values and
probably bring the debugger to it's knees.

jsdProperties rarely see 50 live objects, and jsdScripts don't use the
ephemeral list routines.

I suppose I should switch from circular lists to hash tables when I deal with
bug 121505.

Comment 9

[Brendan Eich [:brendan]]

You multiply inherit from jsdIEphemeral e.g. jsDScript inherits from jsdIScript
and jsdIEphemeral.  But jsdIScript inherits from jsdIEphemeral.  Either get rid
of the MI, or QI this to jsdIEphemeral before storing in mLiveListEntry.value.

/be

Comment 10

[Robert Ginda]

Attachment: native patches, round four

I don't see any MI, but it turns out that forgot to include jsdIEphemeral in my

+NS_IMPL_THREADSAFE_ISUPPORTS1(jsdContext, jsdIContext);
Changing this to
+NS_IMPL_THREADSAFE_ISUPPORTS2(jsdContext, jsdIContext, jsdIEhpemeral);
Clears the assertion.  jsdContext wasn't a jsdIEphemeral in the initial draft
of this code.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 66830 [details] [diff] [review]
native patches, round four

r=jst

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 66830 [details] [diff] [review]
native patches, round four

Argh, I misread grep output in my haste -- glad you found the problem anyway.

sr=me again.

/be

Comment 13

[Robert Ginda]

checked in.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Verify fixed on linux 2002043010.  Debugging target is disabled during debugging
while at a breakpoint.