Closed Bug 121318 Opened 22 years ago Closed 22 years ago

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

Categories

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

x86
All
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: rginda, Assigned: rginda)

References

Details

(Whiteboard: [native])

Attachments

(2 files, 3 obsolete files)

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.
Status: NEW → ASSIGNED
Depends on: 121171
Whiteboard: [native]
Target Milestone: --- → mozilla0.9.9
Attachment #66042 - Attachment is obsolete: true
Comment on attachment 66045 [details] [diff] [review]
native changes, minus useless include

sr=jst
Attachment #66045 - Flags: superreview+
Attached patch native patches, round three (obsolete) — Splinter Review
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.
Attachment #66045 - Attachment is obsolete: true
Attached patch UI changesSplinter Review
Disable executionContext while stopped at a breakpoint.  Removed old
console._stopLevel (we don't do nested breakpoints anymore) too.
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=brendan@mozilla.org 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
Attachment #66580 - Flags: superreview+
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=brendan@mozilla.org

/be
Attachment #66045 - Flags: review+
>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=brendan@mozilla.org 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.
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
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.
Attachment #66580 - Attachment is obsolete: true
Comment on attachment 66830 [details] [diff] [review]
native patches, round four

r=jst
Attachment #66830 - Flags: review+
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
Attachment #66830 - Flags: superreview+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Verify fixed on linux 2002043010.  Debugging target is disabled during debugging
while at a breakpoint.
Status: RESOLVED → VERIFIED
Product: Core → Other Applications
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.