Closed Bug 1234880 Opened 8 years ago Closed 8 years ago

ObservableObject should not wrap a non-configurable, non-writable property.


(DevTools :: General, defect)

Not set


(firefox46 fixed)

Firefox 46
Tracking Status
firefox46 --- fixed


(Reporter: arai, Assigned: arai)




(2 files)

ObservableObject's Proxy handler violates the following Proxy invariant, for |prototype| property:
> [[Get]] (P, Receiver)
>   * If P was previously observed as a non-configurable, non-writable own data
>     property of the target with value v, then [[Get]] must return the
>     SameValue.

Here's the detail:

ObservableObject wraps passed object with a Proxy, that wraps properties recursively, regardless of the property's descriptor.
> function ObservableObject(object = {}) {
> ...
>   let handler = new Handler(this);
>   this.object = new Proxy(object, handler);
> ...
> }
> ...
> Handler.prototype = {
> ...
>   get: function(target, key) {
>     let value = target[key];
>     let [wrapped, path] = this.wrap(target, key, value);
>     this._emitter.emit("get", path, value);
>     return wrapped;
>   },

It's used here:
> const store = new ObservableObject({ projects:[] });

the array's map method is called here:
>     let projects = => p.location);

bug 1165052 will implement Array[@@species] and related semantics in Array#map [1][2].  || accesses |projects.constructor[@@species]|, and the Proxy handler returns a wrapped Array constructor.  Then, the wrapped constructor is called, and |constructor.prototype| is accessed [3][4][5][6].

There, |prototype| property is non-configurable and non-writable, so the Proxy handler should not wrap it.

Here's a try run that fails with it (search "proxy must report the same value"):

There could be some solutions:
  A. fix ObservableObject to check property descriptor, and not to wrap it when it's non-configurable and non-writable
  B. fix ObservableObject not to wrap "prototype" property
  C. change devtools/client/webide/test/head.js to avoid calling Array#map on wrapped array

maybe the best solution depends on the purpose of ObservableObject.
paul, may I have your comments on comment #1?
how widely ObservableObject is/will be used in devtools?
will it be common case to call method on the proxy?
Flags: needinfo?(paul)
> how widely ObservableObject is/will be used in devtools?

It's a old thing. As you mentioned in comment #0, it's only used in app-projects.js.

jryans, is it better to fix observable-object or to get rid of it in app-projects?
Flags: needinfo?(paul) → needinfo?(jryans)
The only remaining user of app-projects is WebIDE, and it does not use any of the ObservableObject features.  I am not aware of any planned work that would need ObservableObject either.  So, the best fix is likely to remove the ObservableObject module and stop using it in app-projects.
Flags: needinfo?(jryans)
Thank you paul and jryans, I'll remove it.
Assignee: nobody → arai.unmht
Great!  In case it's unclear, I would imagine app-projects can be changed to expose a regular array of projects as `AppProjects.projects` and all the stuff about the `store` can be removed.  Also, any callers that use `` should change to `AppProjects.projects`.
Thank you for the hint :)

changed "store" property to "projects", with array literal, instead of using global "projects" variable.
All accesses to it is changed to AppProjects.projects or this.projects.
Attachment #8703996 - Flags: review?(jryans)
and removed ObservableObject.js and test for it.

green on try run:
Attachment #8703997 - Flags: review?(jryans)
Comment on attachment 8703996 [details] [diff] [review]
Part 1: Stop using ObservableObject in app-projects.

Review of attachment 8703996 [details] [diff] [review]:

Great, thanks for doing this!  It seems to work well here.
Attachment #8703996 - Flags: review?(jryans) → review+
sorry had to back this out for causing conflicts with the merge down from mozilla-central to mozilla-inbound like :

merging devtools/client/webide/modules/app-projects.js warning: conflicts while merging devtools/client/webide/modules/app-projects.js! (edit, then use 'hg resolve --mark')
Flags: needinfo?(arai.unmht)
Thank you :)
Fixed locally.  will land it again after try run.
Flags: needinfo?(arai.unmht)
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46


STR: Not clear.
Developer specific testing

Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.