503975 - "Change Value" context menu item is always disabled in Local Variables window of Venkman (JavaScript Debugger)

Status: RESOLVED INCOMPLETE

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

Description

[Lo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5 (.NET CLR 3.5.30729)
Build Identifier: 0.9.87.4

Observed beginning with version 0.9.87.1

Reproducible: Always

Steps to Reproduce:
1. Set a breakpoint in a loaded script.
2. When the breakpoint is hit, the local variables window is populated. Right-click on any of the variables for the context menu.
3. Observe that the Change Value context menu item is always grayed out no matter what variable you click on.
Actual Results:  
The 1st item in the context menu, "Change Value...", is disabled.

Expected Results:  
The "Change Value..." option should be enabled.

Comment 1

[Lo]

Attachment: Proposed patch, requesting r/sr attention

Comment 2

[:Gijs (back August 25; he/him)]

Comment on attachment 388344 [details] [diff] [review]
Proposed patch, requesting r/sr attention

This code was removed in a security bug. I am not comfortable with blanket-reinstating it without being very, very sure that doing so is safe. I should add that if you doubleclick a value, the dialog comes up normally, so it doesn't seem.

In particular, a safer fix may be to use the same test as is used for the qualified name menu entry (in venkman-views.js), ie has('expression'). I think that is, regardless of everything else, a more proper fix (as we need a complete expression anyway). That said, looks like cmdChangeValue could do with some fixing... Meh, that is not for this bug.

Comment 3

[:Gijs (back August 25; he/him)]

Egh. Last sentence of the first paragraph should end "so it doesn't seem like the check is proper anyway".

Comment 4

[James Ross]

The actual line being added by the patch is safe in and of itself (it's a wrapped jsdValue), however, cmdChangeValue is unsafe from start to finish. :(

Comment 5

[:Gijs (back August 25; he/him)]

(In reply to comment #4)
> The actual line being added by the patch is safe in and of itself (it's a
> wrapped jsdValue), however, cmdChangeValue is unsafe from start to finish. :(

Heh. There's a bug for all these cases (which I presume includes this particular one, although I should check). As for the actual line - I wasn't sure, and for some reason although I can look at the relevant bug, I can't actually see the attachment (testcase)? (I used to be able to!) Blah. :-\

Comment 6

[Lo]

Attachment: Updated patch to test for "has('expression')" instead.

Comment 7

[:Gijs (back August 25; he/him)]

(In reply to comment #6)
> Created an attachment (id=389708) [details]
> Updated patch to test for "has('expression')" instead.

Why do we still need the .parentRecord assignment? Or was leaving that in the patch unintentional?

Comment 8

[Lo]

> (In reply to comment #6)
> > Created an attachment (id=389708) [details] [details]
> > Updated patch to test for "has('expression')" instead.
> 
> Why do we still need the .parentRecord assignment? Or was leaving that in the
> patch unintentional?

Yes, intentional. It is specified as one of the params used by cmdChangeValue(), which won't work without it.

Comment 9

[:Gijs (back August 25; he/him)]

Comment on attachment 389708 [details] [diff] [review]
Updated patch to test for "has('expression')" instead.

OK, so, basically, I still don't want to take this patch. The reason is that cmdChangeValue, as James said, is a catastrophe in terms of security. I'm fixing it in bug 396142, but it involves rewriting it from scratch, also removing the need for parentValue to be around. So I'm going to r- this, sorry! :-(

Comment 10

[BMO Automation]

Component is obsolete so resolving bugs as INCOMPLETE