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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Developer Tools: WebIDE
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vporof, Assigned: jryans)

Tracking

unspecified
Firefox 29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28- fixed, firefox29 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Blocks: 808371
(Assignee)

Updated

4 years ago
Blocks: 912912
(Assignee)

Updated

4 years ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Updated

4 years ago
status-firefox28: --- → affected
status-firefox29: --- → affected
(Assignee)

Updated

4 years ago
tracking-firefox28: --- → ?
(Assignee)

Comment 1

4 years ago
You can also trigger this behavior when editing an existing field.  For example, edit an existing string and delete the quotes.
(Assignee)

Comment 2

4 years ago
Created attachment 8347563 [details] [diff] [review]
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
Attachment #8347563 - Flags: review?(paul)
(Reporter)

Comment 3

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

Comment 4

4 years ago
(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! :)
(Assignee)

Updated

4 years ago
Attachment #8347563 - Flags: review?(paul)
(Assignee)

Comment 5

4 years ago
Created attachment 8350368 [details] [diff] [review]
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
Attachment #8347563 - Attachment is obsolete: true
Attachment #8350368 - Flags: review?(vporof)
(Reporter)

Comment 6

4 years ago
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.
Attachment #8350368 - Flags: review?(vporof)
(Assignee)

Comment 7

4 years ago
(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.
Flags: needinfo?(vporof)
(Reporter)

Comment 8

4 years ago
(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 :)
Flags: needinfo?(vporof)
(Assignee)

Comment 9

4 years ago
Created attachment 8355627 [details] [diff] [review]
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
Attachment #8350368 - Attachment is obsolete: true
Attachment #8355627 - Flags: review?(vporof)
(Reporter)

Comment 10

4 years ago
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.
Attachment #8355627 - Flags: review?(vporof) → review+
(Assignee)

Updated

4 years ago
Blocks: 956760
Created attachment 8356161 [details] [diff] [review]
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.
Attachment #8355627 - Attachment is obsolete: true
Attachment #8356161 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/64c3bdede7dd
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/64c3bdede7dd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
(Assignee)

Updated

4 years ago
status-firefox29: affected → fixed
Target Milestone: Firefox 29 → ---
(Assignee)

Updated

4 years ago
Target Milestone: --- → Firefox 29
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
Attachment #8356161 - Flags: approval-mozilla-aurora?
tracking-firefox28: ? → -
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.
Attachment #8356161 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0607d5d0e07
status-firefox28: affected → fixed
You need to log in before you can comment on or make changes to this bug.