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

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Tooru Fujisawa [:arai]]

ObservableObject's Proxy handler violates the following Proxy invariant, for |prototype| property:

http://www.ecma-international.org/ecma-262/6.0/#sec-invariants-of-the-essential-internal-methods
> [[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.

http://hg.mozilla.org/mozilla-central/file/35b211eaad1f/devtools/client/shared/observable-object.js#l35
> 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:

http://hg.mozilla.org/mozilla-central/file/35b211eaad1f/devtools/client/webide/modules/app-projects.js#l153
> const store = new ObservableObject({ projects:[] });


the array's map method is called here:

http://hg.mozilla.org/mozilla-central/file/35b211eaad1f/devtools/client/webide/test/head.js#l97
>     let projects = AppProjects.store.object.projects.map(p => p.location);


bug 1165052 will implement Array[@@species] and related semantics in Array#map [1][2].  |projects.map(...)| 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"):
https://treeherder.mozilla.org/logviewer.html#?job_id=14846062&repo=try


[1] http://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.map
[2] http://www.ecma-international.org/ecma-262/6.0/#sec-arrayspeciescreate
[3] http://www.ecma-international.org/ecma-262/6.0/#sec-construct
[4] http://www.ecma-international.org/ecma-262/6.0/#sec-ecmascript-function-objects-construct-argumentslist-newtarget
[5] http://www.ecma-international.org/ecma-262/6.0/#sec-ordinarycreatefromconstructor
[6] http://www.ecma-international.org/ecma-262/6.0/#sec-getprototypefromconstructor

Comment 1

[Tooru Fujisawa [:arai]]

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.

Comment 2

[Tooru Fujisawa [:arai]]

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?

Comment 3

[Paul Rouget [: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?

Comment 4

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

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.

Comment 5

[Tooru Fujisawa [:arai]]

Thank you paul and jryans, I'll remove it.

Comment 6

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

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 `AppProjects.store` should change to `AppProjects.projects`.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Stop using ObservableObject in app-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.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove ObservableObject.

and removed ObservableObject.js and test for it.

green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab61a7984c70

Comment 9

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

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.

Comment 10

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bc2537d59799255ab14029d51316881c7a298a
Bug 1234880 - Part 1: Stop using ObservableObject in app-projects. r=jryans

https://hg.mozilla.org/integration/mozilla-inbound/rev/0876b808c67701f77e6fd5abf031500bc7f479fd
Bug 1234880 - Part 2: Remove ObservableObject. r=jryans

Comment 11

[Carsten Book [:Tomcat]]

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')

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ddde197552
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee61a8fff8b9

Comment 13

[Tooru Fujisawa [:arai]]

Thank you :)
Fixed locally.  will land it again after try run.

Comment 14

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/601d663a23ac039aa3f0feacf7b69f4e10d5f0fc
Bug 1234880 - Part 1: Stop using ObservableObject in app-projects. r=jryans

https://hg.mozilla.org/integration/mozilla-inbound/rev/5005e2d4228fe41b36ac2c14b6d35a9115214d98
Bug 1234880 - Part 2: Remove ObservableObject. r=jryans

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/601d663a23ac
https://hg.mozilla.org/mozilla-central/rev/5005e2d4228f

Comment 16

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
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