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

RESOLVED FIXED in Firefox 46

Status

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

unspecified
Firefox 46

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

3 years ago
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.
(Assignee)

Comment 2

3 years ago
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)

Comment 3

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

Comment 5

3 years ago
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 `AppProjects.store` should change to `AppProjects.projects`.
(Assignee)

Comment 7

3 years ago
Created attachment 8703996 [details] [diff] [review]
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.
Attachment #8703996 - Flags: review?(jryans)
(Assignee)

Comment 8

3 years ago
Created attachment 8703997 [details] [diff] [review]
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
Attachment #8703997 - Flags: review?(jryans)
Attachment #8703997 - Flags: review?(jryans) → review+
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)
(Assignee)

Comment 13

3 years ago
Thank you :)
Fixed locally.  will land it again after try run.
Flags: needinfo?(arai.unmht)

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/601d663a23ac
https://hg.mozilla.org/mozilla-central/rev/5005e2d4228f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 16

3 years ago
[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

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.