Bug 222538 – REGRESSION: Object.defineProperties triggering a setter

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark S. Miller]

Created attachment 421790 [details]
Screenshot of debugger state caught in buggy behavior

See https://github.com/Agoric/SES-shim/pull/592

The SES Demo console, after previously having worked on Safari Technology Preview, suddenly started failing with an infinite recursion. This instrumented variation of the PR in which this started failing catches when `Object.defineProperties` causes a setter to be called. The `Object.defineProperties` is attempting to convert `Array.prototype.toString` from a data property to an accessor property, after `Object.prototype.toString` has been similarly converted. Violating the semantics of `defineProperties`, it causes the setter of the `Object.prototype.toString` accessor property to be called. There follows some screenshots examining this state from within the debugger.

This is on Safari Technology Preview
Release 121 (Safari 14.2, WebKit 16612.1.4.3)

[Mark S. Miller]

Created attachment 421791 [details]
Screenshot of debugger state caught in buggy behavior

[Mark S. Miller]

Created attachment 421792 [details]
Screenshot of debugger state caught in buggy behavior

[Mark S. Miller]

Created attachment 421793 [details]
Screenshot of debugger state caught in buggy behavior

[Mark S. Miller]

Created attachment 421794 [details]
Screenshot of debugger state caught in buggy behavior

[Mark S. Miller]

Created attachment 421795 [details]
Screenshot of debugger state caught in buggy behavior

[Radar WebKit Bug Importer]

<rdar://problem/74888939>

[Yusuke Suzuki]

(In reply to Mark S. Miller from comment #0)
> Created attachment 421790 [details]
> Screenshot of debugger state caught in buggy behavior
> 
> See https://github.com/Agoric/SES-shim/pull/592
> 
> The SES Demo console, after previously having worked on Safari Technology
> Preview, suddenly started failing with an infinite recursion. This
> instrumented variation of the PR in which this started failing catches when
> `Object.defineProperties` causes a setter to be called. The
> `Object.defineProperties` is attempting to convert
> `Array.prototype.toString` from a data property to an accessor property,
> after `Object.prototype.toString` has been similarly converted. Violating
> the semantics of `defineProperties`, it causes the setter of the
> `Object.prototype.toString` accessor property to be called. There follows
> some screenshots examining this state from within the debugger.
> 
> This is on Safari Technology Preview
> Release 121 (Safari 14.2, WebKit 16612.1.4.3)

Thanks for the report! Is there a instruction reproducing this problem?

[Mark S. Miller]

> Thanks for the report! Is there a instruction reproducing this problem?

Yes. I set aside that PR only for the purpose of demonstrating and reproducing this bug. 

Do a git clone of the `https://github.com/Agoric/SES-shim/tree/safari-defProps-bug` branch. (The branch corresponding to that PR)

cd to packages/ses

run any local web browser. I used httpdhere

Visit http://localhost:8080/demos/console/

Set the breakpoint shown

Refresh the page.

[Mark S. Miller]

I omitted a step in my instructions to reproduce. After

> cd to packages/ses

do a `yarn build` in that directory. 

You need yarn v1, yes v1, installed. Sorry about that.

[Mark S. Miller]

Sigh. Sorry about this. Always hard to remember habits. Before that cd'ing or yarn build, in the root directory of the monorepo, do a `yarn install && yarn build`. Not sure all of that is needed but it is what I've been doing.

[Yusuke Suzuki]

Created attachment 421886 [details]
Patch

[Keith Miller]

Comment on attachment 421886 [details]
Patch

r=me.

[EWS]

Committed r273717: <https://commits.webkit.org/r273717>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421886 [details].

[Yusuke Suzuki]

@Mark Thanks! I think the issue is now fixed.

[Mark S. Miller]

Awesomely speedy response! I am impressed. Thanks.

[Mark S. Miller]

I have some evidence that this bug happened again under Safari Version 14.1 (16611.1.21.161.3) *in the debugger* with weaker evidence that it did not happen for the same code not under the debugger. Until I know more I'm not reopening. But posting this anyway it case it rings a bell.

[Mark S. Miller]

Yes, the bug is still there in the latest Safari. I now believe that it is intermittent or at least sensitive to conditions I don't understand. But it seems to happen both inside the debugger and normally, so my previous speculation is wrong. 

I will upload a set of screenshots that make it clear is still happening.

[Mark S. Miller]

Created attachment 428041 [details]
The call to defineProperties

1 of 3

To reproduce yourself, visit https://ses-demo.netlify.app/demos/console/ and open the debugger console. You might already see the error message in the console from the page initializing itself. If not, try refreshing. If not, shut down and relaunch the browser. I really have no better formula.

[Mark S. Miller]

Created attachment 428042 [details]
The call to the setter

At line 5325 is a call to a function named defineProperty. This is actually the defineProperty defined in the first screenshot, which calls defineProperties.

[Mark S. Miller]

Created attachment 428046 [details]
Shows about box and full url

[Mark S. Miller]

Since Safari has this bug, I plan to revise our sources to workaround this bug. When I do so, the website at that url will update. So first I'll indicate the current version of out sources.

[Mark S. Miller]

Created attachment 428049 [details]
Snapshot of built package currently use by page

[Mark S. Miller]

Created attachment 428050 [details]
snapshot of html page

[Mark S. Miller]

Created attachment 428051 [details]
snapshot of demo source used by that page

[Mark S. Miller]

https://github.com/endojs/endo/blob/0b289021eee1256c05ceb4d83318165cb6288844/packages/ses/src/enable-property-overrides.js#L87

https://github.com/endojs/endo/blob/0b289021eee1256c05ceb4d83318165cb6288844/packages/ses/src/commons.js#L50

Github permalinks into the actual sources that lockdown.umd.js is built from. These specific links that the infinite recursion ping pongs between.

[Mark S. Miller]

I just now noticed my old reproduction advice at https://bugs.webkit.org/show_bug.cgi?id=222538#c8

I have not tried it, but nothing relevant has changed. I suspect that will also work.

[Mark S. Miller]

Ignore my last statement. The reproduction instructions from #c8 may not work. Relevant things are different. Use the reproduction instructions I gave you today.

[Yusuke Suzuki]

(In reply to Mark S. Miller from comment #27)
> Ignore my last statement. The reproduction instructions from #c8 may not
> work. Relevant things are different. Use the reproduction instructions I
> gave you today.

Could you test it on Safari Technology Preview?

[Yusuke Suzuki]

> I have some evidence that this bug happened again under Safari Version 14.1 (16611.1.21.161.3) *in the debugger* with weaker evidence that it did not happen for the same code not under the debugger. Until I know more I'm not reopening. But posting this anyway it case it rings a bell.

Safari 14.1 does not include this change. So closing this issue.
Please reopen when you reproduced it with Safari Technology Preview :)
Thanks!