947710 - Trying to add a property in the manifest editor can break the editor

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect, P1)

Description

[Victor Porof [:vporof][:vp]]

STR:

1. Go to the Manifest Editor in the App Manager
2. Try to add a property named `test` with the value `lol`.

The parent property's name is shifted to the left, its children are hidden and it can't be modified anymore.

https://bugzilla.mozilla.org/show_bug.cgi?id=808371#c46

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

You can also trigger this behavior when editing an existing field.  For example, edit an existing string and delete the quotes.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Catch errors when editing manifest

Wrap edits involving user-supplied values in try blocks to catch errors so we don't break the editor.

Try: https://tbpl.mozilla.org/?tree=Try&rev=290d5c4290c2

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 8347563 [details] [diff] [review]
Catch errors when editing manifest

Review of attachment 8347563 [details] [diff] [review]:
-----------------------------------------------------------------

Quick drive-by, hope you don't mind :)

::: browser/devtools/app-manager/content/manifest-editor.js
@@ +63,5 @@
>  
>    _onEval: function(evalString) {
>      let manifest = this.manifest;
> +    try {
> +      eval("manifest" + evalString);

Are we sure we want to allow arbitrary code evaluation here? Did this go through secreview or am I being too paranoid? :) If it didn't go through secreview, can you please ask for one?

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Victor Porof [:vp] from comment #3)
> Are we sure we want to allow arbitrary code evaluation here?

Actually, the eval's been bothering me for a while...  I'll take a look at how hard it would be to do without eval.  Thanks for bringing this up! :)

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Stop evaling and catch errors in Manifest Editor

No more evals! :)

I ended up passing more args to VView.eval, as that seemed a bit cleaner once I looked at it more closely.  If I had instead added VView.evaluationMacro, then I wouldn't want to set VView.eval at all, but I still want evaluation enabled, and that currently is checked for by testing item.eval...  Felt like it would get a little crazy to support both possible paths.

Try: https://tbpl.mozilla.org/?tree=Try&rev=7b7536480c4b

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8350368 [details] [diff] [review]
Stop evaling and catch errors in Manifest Editor

Review of attachment 8350368 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/app-manager/content/manifest-editor.js
@@ +61,5 @@
>      return this.update();
>    },
>  
> +  _onEval: function(evalMacroResult, variable, value) {
> +    let parent = this._descend(variable.ownerView.symbolicPath);

Can you please explain a bit why you need a symbolicPath? I don't think adding it is necessary. You can get the name of properties up to the parent variable by doing something like this:

// assume you have a property nested 2 levels deep
let name1 = nestedProperty.name;
let name2 = nestedProperty.ownerView.name;
let name3 = nestedProperty.ownerView.ownerView.name;
let symbolicPath = [name3, name2, name1];

So basically:

function getPath(item, store = []) {
  // scopes, variables and properties are all instances of Scope
  if (item instanceof Scope) {
    store.push(item.name);
    return getPath(item.ownerView, store);
  }
  return store;
}

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2721,5 @@
>        onSave: aString => {
>          if (!this._variablesView.preventDisableOnChange) {
>            this._disable();
>          }
> +        this.ownerView.eval(this.evaluationMacro(this, aString), this, aString);

I'm ok with augmenting eval, but you'll have to take care of two things:

1. The `delete` and `new` methods all pass the variable or property instance as the first argument. Eval shouldn't be different now that we're making this change.

2. Sometimes, the eval property on the variables view may have been set to a function that took more than one argument. This change here can subtly break things, or not. We should be careful.

For example, in the debugger, the eval function is set to DebuggerController.StackFrames.evaluate, which takes two arguments (in debugger-controller.js).

You should change things so that evaluate keeps taking only a single param, to avoid unexpected behavior.

In debugger-view.js, in _initializeVariablesView, instead of 
> eval: DebuggerController.StackFrames.evaluate,
it should be something like
> eval: (_, str) => DebuggerController.StackFrames.evaluate(str),

To be sure, please also check the webconsole and scratchpad for this subtle thing.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Victor Porof [:vp] from comment #6)
> ::: browser/devtools/app-manager/content/manifest-editor.js
> @@ +61,5 @@
> >      return this.update();
> >    },
> >  
> > +  _onEval: function(evalMacroResult, variable, value) {
> > +    let parent = this._descend(variable.ownerView.symbolicPath);
> 
> Can you please explain a bit why you need a symbolicPath? I don't think
> adding it is necessary.

I added symbolicPath for parity with symbolicName.  symbolicPath is a better fit for anyone that wants to apply changes without using eval vs. symbolicName which is meant for use with eval or something similar.

If symbolicPath should not be there, it feels like symbolicName doesn't belong either, since they are both expressing the same information, but just formatted differently.

How about I change symbolicPath to a getter that computes on-demand instead of for every variable / property at init time?  That way it still lives in VariableView.jsm (which feels best to me since symbolicName is there), but there's no perf impact if it's not used.

Comment 8

[Victor Porof [:vporof][:vp]]

(In reply to J. Ryan Stinnett [:jryans] from comment #7)
> (In reply to Victor Porof [:vp] from comment #6)

A getter sounds good. Can you do that with the getPath function I described in the previous comment, or is there something wrong with that algorithm? If possible, I'd be happier if we didn't parse strings, as that kind of enforces symbolicName (and absoluteName for that matter) to have a specific format. Recursively creating an array of variable/property names sounds like a better idea to me, but I'm happy to be proven wrong :)

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Stop evaling and catch errors in Manifest Editor v2

(In reply to Victor Porof [:vp] from comment #8)
> A getter sounds good. Can you do that with the getPath function I described
> in the previous comment, or is there something wrong with that algorithm? If
> possible, I'd be happier if we didn't parse strings, as that kind of
> enforces symbolicName (and absoluteName for that matter) to have a specific
> format. Recursively creating an array of variable/property names sounds like
> a better idea to me, but I'm happy to be proven wrong :)

Yes, I am using a recursive algorithm similar to the one you posted.  Definitely no string parsing, that sounds yucky!

Try: https://tbpl.mozilla.org/?tree=Try&rev=4bb3df72d5d4

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8355627 [details] [diff] [review]
Stop evaling and catch errors in Manifest Editor v2

Review of attachment 8355627 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet!

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2297,4 @@
>     * For example, a symbolic name may look like "arguments['0']['foo']['bar']".
>     * @return string
>     */
>    get symbolicName() this._symbolicName,

Nit: similarly, via a getter, you could make symbolicName also be a product of symbolicPath, which would nicely create the expected string. This is going to make things cleaner, and spare a few cycles when variable and property instances are created.

There won't be any performance concerns with switching to a getter, because the symbolicName is only needed when creating evaluation macros (so only when eval is called).

Definitely shouldn't block landing this patch, but please file a followup. It looks like it can be a good first bug!

@@ +2320,5 @@
> +   */
> +  _buildSymbolicPath: function(path = []) {
> +    if (this.name) {
> +      path.unshift(this.name);
> +      if (this.ownerView && this.ownerView._buildSymbolicPath) {

Simply doing `if (this.ownerView instanceof Variable)` is better than checking for traits IMHO. This will work for both Variable and Property instances, since Property inherits from Variable.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Stop evaling and catch errors in Manifest Editor v3

Carrying over Victor's r+ from attachment 8355627 [details] [diff] [review].

(In reply to Victor Porof [:vp] from comment #10)
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +2297,4 @@
> >     * For example, a symbolic name may look like "arguments['0']['foo']['bar']".
> >     * @return string
> >     */
> >    get symbolicName() this._symbolicName,
> 
> Nit: similarly, via a getter, you could make symbolicName also be a product
> of symbolicPath, which would nicely create the expected string. This is
> going to make things cleaner, and spare a few cycles when variable and
> property instances are created.
> 
> There won't be any performance concerns with switching to a getter, because
> the symbolicName is only needed when creating evaluation macros (so only
> when eval is called).
> 
> Definitely shouldn't block landing this patch, but please file a followup.
> It looks like it can be a good first bug!

Okay, filed bug 956760.

> @@ +2320,5 @@
> > +   */
> > +  _buildSymbolicPath: function(path = []) {
> > +    if (this.name) {
> > +      path.unshift(this.name);
> > +      if (this.ownerView && this.ownerView._buildSymbolicPath) {
> 
> Simply doing `if (this.ownerView instanceof Variable)` is better than
> checking for traits IMHO. This will work for both Variable and Property
> instances, since Property inherits from Variable.

Changed to instanceof as suggested.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/64c3bdede7dd

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/64c3bdede7dd

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8356161 [details] [diff] [review]
Stop evaling and catch errors in Manifest Editor v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Present since landing manifest editor
User impact if declined: The manifest editor UI can break if user input contains any syntax errors
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8356161 [details] [diff] [review]
Stop evaling and catch errors in Manifest Editor v3

No need to track since this has been present since the feature landed but we can take the uplift to Aurora to ship the fix sooner.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f0607d5d0e07