Workaround for override mistake #146

kumavis · 2019-08-26T04:40:50Z

Fixes #12

introduces repairDataProperties originally from tc39/proposal-ses and adapts it to meet current SES.

A few notes for review:

need to adjust/update copyright info? file has been modified
currently only handling dynamic presence of Promise (there's a test for it), need to expand?
want a more minimal toBeRepaired set? can do, #12 has suggestions
defineProperty function was based off a comment I found in realms-shim/src/common, but doesn't seem to be true in node v10
mutability aspect of this feature is not tested
compare to SalesForce Aura repairDataProperties
in this comment @erights seems to propose doing this inside of harden

review requested @erights @michaelfig @jfparadis @bmeck

kumavis · 2019-08-26T05:30:46Z

src/bundle/mutable.js

+  } = Object;
+  const { ownKeys } = Reflect;
+
+  // Object.defineProperty is allowed to fail silently,


This was based off a comment I found in realms-shim/src/common, but doesn't seem to be true in at least node v10 REPL

Another obscure corner of JavaScript. The only case where this might happen is on browsers, where there's a distinction between the reified so-called "WindowProxy" object and the unreified so-called "Window" object. The issue is that a browser frame, when it is navigated from url1 to url2, gets the completely new root realm associated with url2, including global variable bindings, but keeps the same global "window" object (which is actually a WindowProxy) because that is the same object the parent can hold onto to designate this frame no matter what it is navigated to.

The way this is implemented is that every root realm that occupied the frame has its own hidden so-called "Window" object, whose properties are aliased to that root realm's global variables. The so-called "WindowProxy" object has a current hidden "Window" object, that it forwards all operations to. Done naively, this leads to a contradiction:

Say wp is such a WindowProxy object. While it is at url1, and therefore forwarding to hidden window hw1, someone does an

Object.defineProperty(wp, 'foo', { value: 88, writable: false, configurable: false });

Let's say that this call reports success, however defineProperty is supposed to report success. Then, presumably, hw1 actually has a non-writable, non-configurable foo data property with value 88. By the object invariants, these conditions represent a commitment that this property on this object must always be a non-writable, non-configurable foo data property with value 88. Fine so far.

Now some navigates the frame to url2, associated with hidden window hw2 that has no foo property, and whose root realm therefore also has no foo global variable. What should

Object.getOwnPropertyDescriptor(wp, 'foo')

return? What it actually does across all browsers, and as mandated by the html spec, is to forward that getOwnPropertyDescriptor request to its current hidden window object, hw2, which returns undefined. However, were wp to do so, after the previous defineProperty had indicated success, it would violate the previous commitment.

Therefore, the previous call to defineProperty must not report success. Heroic work by Mozilla on Firefox at first had that defineProperty call throw an error, which was the only way that defineProperty could report that the operation failed. This broke some old code in some old version of some library that did not actually care whether the defineProperty succeeded, failed, or did anything at all. But if the call to defineProperty threw, the library broke.

Therefore, for this one horrible special case, we allow defineProperty to report failure by returning false rather than throwing. SES does not care about breaking that old version of some obscure library. SES does care about the threat to integrity if code proceeds normally under the assumption that some operation succeeded when it didn't. Thus, SES should restore the less kludgy behavior of defineProperty, that could only report failure by throwing.

For now should I continue to use Object.defineProperties as workaround? or something else?

Attn @ljharb Do you know the status of the possibility that the failure masking behavior of Object.defineProperty be extended to Object.defineProperties? In your opinion, should we switch to another technique to unmask the failure masking, to stay safe in the face of this possible change?

Based on what I'm reading in this thread, the only reason Object.defineProperty was forced to not throw was for legacy web compat - if Object.defineProperties doesn't already have that problem, then I can't see why we'd ever want to make it not throw when it fails?

Good. I agree with the conclusion. I was asking because I recall some tc39 talk of extending the failure masking behavior to Object.defineProperties for consistency. But I haven't heard much. If you haven't run across this, it is likely inactive. Whew. Thanks.

src/bundle/mutable.js

kumavis · 2019-08-26T10:41:17Z

Is the toBeRepaired set inclusion a trade-off of perf vs extensibility? I assume so. I'll likely be pushing for wider inclusion as I find more things in use in the wild.

erights

Glad to see this!!

Mostly, I'd like to wait a reasonable amount of time to see what answers we get to the questions below. But if necessary, I'd rather see this PR proceed as is, and then get updated in light of those answers later.

erights · 2019-08-26T17:13:46Z

src/bundle/mutable.js

+  } = Object;
+  const { ownKeys } = Reflect;
+
+  // Object.defineProperty is allowed to fail silently,


Another obscure corner of JavaScript. The only case where this might happen is on browsers, where there's a distinction between the reified so-called "WindowProxy" object and the unreified so-called "Window" object. The issue is that a browser frame, when it is navigated from url1 to url2, gets the completely new root realm associated with url2, including global variable bindings, but keeps the same global "window" object (which is actually a WindowProxy) because that is the same object the parent can hold onto to designate this frame no matter what it is navigated to.

The way this is implemented is that every root realm that occupied the frame has its own hidden so-called "Window" object, whose properties are aliased to that root realm's global variables. The so-called "WindowProxy" object has a current hidden "Window" object, that it forwards all operations to. Done naively, this leads to a contradiction:

Say wp is such a WindowProxy object. While it is at url1, and therefore forwarding to hidden window hw1, someone does an

Object.defineProperty(wp, 'foo', { value: 88, writable: false, configurable: false });

Let's say that this call reports success, however defineProperty is supposed to report success. Then, presumably, hw1 actually has a non-writable, non-configurable foo data property with value 88. By the object invariants, these conditions represent a commitment that this property on this object must always be a non-writable, non-configurable foo data property with value 88. Fine so far.

Now some navigates the frame to url2, associated with hidden window hw2 that has no foo property, and whose root realm therefore also has no foo global variable. What should

Object.getOwnPropertyDescriptor(wp, 'foo')

return? What it actually does across all browsers, and as mandated by the html spec, is to forward that getOwnPropertyDescriptor request to its current hidden window object, hw2, which returns undefined. However, were wp to do so, after the previous defineProperty had indicated success, it would violate the previous commitment.

Therefore, the previous call to defineProperty must not report success. Heroic work by Mozilla on Firefox at first had that defineProperty call throw an error, which was the only way that defineProperty could report that the operation failed. This broke some old code in some old version of some library that did not actually care whether the defineProperty succeeded, failed, or did anything at all. But if the call to defineProperty threw, the library broke.

Therefore, for this one horrible special case, we allow defineProperty to report failure by returning false rather than throwing. SES does not care about breaking that old version of some obscure library. SES does care about the threat to integrity if code proceeds normally under the assumption that some operation succeeded when it didn't. Thus, SES should restore the less kludgy behavior of defineProperty, that could only report failure by throwing.

src/bundle/mutable.js

erights · 2019-08-26T17:23:30Z

src/bundle/mutable.js

+
+      // Re-attach the data property on the object so
+      // it can be found by the deep-freeze traversal process.
+      getter.value = value;


IIRC, this was one of the things that Salesforce changed, though I do not remember to what.

its present in forcedotcom/aura, don't have access to their latest work for comparison.

also present in node's version

getter.value = value; is fine.

src/bundle/mutable.js

erights · 2019-08-26T17:54:10Z

Is the toBeRepaired set inclusion a trade-off of perf vs extensibility?

Yes. Salesforce found they only needed to repair 5 primordial prototypes for the problem to practically disappear. OTOH Node did all the primordial prototypes. I am not aware of anyone running into problems with non-prototypes.

I assume so. I'll likely be pushing for wider inclusion as I find more things in use in the wild.

Good. After the core set are done, we should indeed wait till we see actual problems before extending the repair set further.

…edOverride

kumavis · 2019-08-27T03:15:58Z

Addressed most primary concerns and added some basic tests. While this can be improved, I think it is ready for merge and can be improved in subsequent PRs.

kumavis · 2019-08-27T03:41:31Z

I didn't significantly reduce the toBeRepaired list, but I restrained my desire to make it more complete 😅

kumavis · 2019-08-27T04:47:59Z

Ok, took a swing at reducing the repairDataProperties list. This is enough to get metamask working with sesify/lavamoat.

erights

LGTM!

kumavis · 2019-08-27T09:55:05Z

this will allow me to remove some entries from https://github.com/Agoric/SES/issues/136

michaelfig

LGTM as well.

kumavis and others added 5 commits August 26, 2019 11:11

add mutable.js from tc39/proposal-ses e5271cc

18fcbaa

repairDataProperties on intrinsics before harden

aa68ac8

create repairDataProperties in SES via makeRepairDataProperties

014fda1

lint fix

3108ad8

Merge branch 'master' into override-mistake-workaround

6630e69

kumavis commented Aug 26, 2019

View reviewed changes

src/bundle/mutable.js Outdated Show resolved Hide resolved

repairDataProperties - add TypedArray.prototype

25d1369

kumavis force-pushed the override-mistake-workaround branch from f5b35f6 to 25d1369 Compare August 26, 2019 08:28

kumavis commented Aug 26, 2019

View reviewed changes

src/bundle/mutable.js Outdated Show resolved Hide resolved

kumavis commented Aug 26, 2019

View reviewed changes

src/bundle/mutable.js Outdated Show resolved Hide resolved

Merge branch 'master' into override-mistake-workaround

b8687f9

erights requested changes Aug 26, 2019

View reviewed changes

kumavis added 3 commits August 27, 2019 10:44

rename mutable to makeRepairDataProperties + beMutable to enableDeriv…

2fa711d

…edOverride

adjust toBeRepaired set for repairDataProperties

31ea209

add basic tests for repairDataProperties

39a7372

minimize toBeRepaired list for repairDataProperties

c962e27

erights approved these changes Aug 27, 2019

View reviewed changes

michaelfig approved these changes Aug 27, 2019

View reviewed changes

michaelfig merged commit a1a52e1 into Agoric:master Aug 27, 2019

kumavis deleted the override-mistake-workaround branch August 27, 2019 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround for override mistake #146

Workaround for override mistake #146

kumavis commented Aug 26, 2019 •

edited

Loading

kumavis Aug 26, 2019 •

edited

Loading

erights Aug 26, 2019

kumavis Aug 27, 2019 •

edited

Loading

erights Aug 27, 2019

ljharb Aug 27, 2019

erights Aug 27, 2019

kumavis commented Aug 26, 2019

erights left a comment

erights Aug 26, 2019

erights Aug 26, 2019

kumavis Aug 27, 2019 •

edited

Loading

erights Aug 27, 2019

erights commented Aug 26, 2019

kumavis commented Aug 27, 2019

kumavis commented Aug 27, 2019

kumavis commented Aug 27, 2019

erights left a comment

kumavis commented Aug 27, 2019

michaelfig left a comment

Workaround for override mistake #146

Workaround for override mistake #146

Conversation

kumavis commented Aug 26, 2019 • edited Loading

kumavis Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

erights Aug 26, 2019

Choose a reason for hiding this comment

kumavis Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

erights Aug 27, 2019

Choose a reason for hiding this comment

ljharb Aug 27, 2019

Choose a reason for hiding this comment

erights Aug 27, 2019

Choose a reason for hiding this comment

kumavis commented Aug 26, 2019

erights left a comment

Choose a reason for hiding this comment

erights Aug 26, 2019

Choose a reason for hiding this comment

erights Aug 26, 2019

Choose a reason for hiding this comment

kumavis Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

erights Aug 27, 2019

Choose a reason for hiding this comment

erights commented Aug 26, 2019

kumavis commented Aug 27, 2019

kumavis commented Aug 27, 2019

kumavis commented Aug 27, 2019

erights left a comment

Choose a reason for hiding this comment

kumavis commented Aug 27, 2019

michaelfig left a comment

Choose a reason for hiding this comment

kumavis commented Aug 26, 2019 •

edited

Loading

kumavis Aug 26, 2019 •

edited

Loading

kumavis Aug 27, 2019 •

edited

Loading

kumavis Aug 27, 2019 •

edited

Loading