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

VERIFIED FIXED in mozilla0.9.9

Status

VERIFIED FIXED
17 years ago
2 months ago

People

(Reporter: rginda, Assigned: rginda)

Tracking

Trunk
mozilla0.9.9
x86
All

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [native])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
Created attachment 66042 [details] [diff] [review]
Native changes, add |scriptsEnabled| property to jsdIStackFrame
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Depends on: 121171
Whiteboard: [native]
Target Milestone: --- → mozilla0.9.9
(Assignee)

Comment 2

17 years ago
Created attachment 66045 [details] [diff] [review]
native changes, minus useless include
Attachment #66042 - Attachment is obsolete: true
Comment on attachment 66045 [details] [diff] [review]
native changes, minus useless include

sr=jst
Attachment #66045 - Flags: superreview+
(Assignee)

Comment 4

17 years ago
Created attachment 66580 [details] [diff] [review]
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.
Attachment #66045 - Attachment is obsolete: true
(Assignee)

Comment 5

17 years ago
Created attachment 66581 [details] [diff] [review]
UI changes

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+
(Assignee)

Comment 8

17 years ago
>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
(Assignee)

Comment 10

17 years ago
Created attachment 66830 [details] [diff] [review]
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.
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+
(Assignee)

Comment 13

17 years ago
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 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

Updated

2 months ago
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.