Bug 234282 – Remove showModalDialog-specific logic from JSDOMWindow::getOwnPropertySlot()

• Component: DOM
• Status: RESOLVED FIXED

[Mark S. Miller]

At https://jsfiddle.net/kryLa94b/ we have the code

console.log(Object.getOwnPropertyNames(globalThis).includes('showModalDialog'));
console.log(Object.getOwnPropertyDescriptor(globalThis, 'showModalDialog'));

The first line tests whether `'showModalDialog'` is a property of the global object. The second line gets its own property descriptor. 

If there is no such own property, as seen for example on Brave on MacOS, the answers are `false` and `undefined`, which is consistent. 

If there is such an own property, as seen for example on Safari 15.1 on MacOS 12.0.1, the answers are

```
true
{
  configurable: true,
  enumerable: true,
  value: function showModalDialog() {
    [native code]
  },
  writable: true
}
```
which is consistent.

However, on iOS 15.1, the answers are `true` and `undefined`. These are inconsistent answers. Is there such an own property or not.

See also https://github.com/endojs/endo/issues/947 where Ash Connell originally reported this bug.

[Alexey Shvayka]

Thanks for detailed report, Mark.

The issue occurs due to some complicated logic for "showModalDialog" in Window's [[GetOwnProperty]], which is not aligned with other internal methods.

showModalDialog() was removed from Chrome in 2015 and from Firefox in 2017, so I guess we are safe to remove it for Web content only.

[Radar WebKit Bug Importer]

<rdar://problem/86749078>

[Chris Dumez]

(In reply to Alexey Shvayka from comment #1)
> Thanks for detailed report, Mark.
> 
> The issue occurs due to some complicated logic for "showModalDialog" in
> Window's [[GetOwnProperty]], which is not aligned with other internal
> methods.
> 
> showModalDialog() was removed from Chrome in 2015 and from Firefox in 2017,
> so I guess we are safe to remove it for Web content only.

Correction, we'll be safe to remove showModalDialog() ONCE we ship the <dialog> element.

[Alexey Shvayka]

(In reply to Chris Dumez from comment #3)
> (In reply to Alexey Shvayka from comment #1)
> > Thanks for detailed report, Mark.
> > 
> > The issue occurs due to some complicated logic for "showModalDialog" in
> > Window's [[GetOwnProperty]], which is not aligned with other internal
> > methods.
> > 
> > showModalDialog() was removed from Chrome in 2015 and from Firefox in 2017,
> > so I guess we are safe to remove it for Web content only.
> 
> Correction, we'll be safe to remove showModalDialog() ONCE we ship the
> <dialog> element.

Blink removed showModalDialog() in M43, while shipped <dialog> (on by default) in M37.
However, Gecko removed showModalDialog() in M55, yet still hasn't shipped the <dialog> on by default.

What am I missing?

[Chris Dumez]

(In reply to Alexey Shvayka from comment #4)
> (In reply to Chris Dumez from comment #3)
> > (In reply to Alexey Shvayka from comment #1)
> > > Thanks for detailed report, Mark.
> > > 
> > > The issue occurs due to some complicated logic for "showModalDialog" in
> > > Window's [[GetOwnProperty]], which is not aligned with other internal
> > > methods.
> > > 
> > > showModalDialog() was removed from Chrome in 2015 and from Firefox in 2017,
> > > so I guess we are safe to remove it for Web content only.
> > 
> > Correction, we'll be safe to remove showModalDialog() ONCE we ship the
> > <dialog> element.
> 
> Blink removed showModalDialog() in M43, while shipped <dialog> (on by
> default) in M37.
> However, Gecko removed showModalDialog() in M55, yet still hasn't shipped
> the <dialog> on by default.
> 
> What am I missing?

We've discussed removing showModalDialog several times in the past and every time we made the decision to delay until we have a good alternative implemented (<dialog> element). Not to say that you cannot get that particular discussion started again. I am just telling you removing showModalDialog() at this point will be controversial.

Also, I see good patches landing for <dialog> support so maybe we won't have to wait long?

[Alexey Shvayka]

(In reply to Chris Dumez from comment #5)
> (In reply to Alexey Shvayka from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > (In reply to Alexey Shvayka from comment #1)
> > > > Thanks for detailed report, Mark.
> > > > 
> > > > The issue occurs due to some complicated logic for "showModalDialog" in
> > > > Window's [[GetOwnProperty]], which is not aligned with other internal
> > > > methods.
> > > > 
> > > > showModalDialog() was removed from Chrome in 2015 and from Firefox in 2017,
> > > > so I guess we are safe to remove it for Web content only.
> > > 
> > > Correction, we'll be safe to remove showModalDialog() ONCE we ship the
> > > <dialog> element.
> > 
> > Blink removed showModalDialog() in M43, while shipped <dialog> (on by
> > default) in M37.
> > However, Gecko removed showModalDialog() in M55, yet still hasn't shipped
> > the <dialog> on by default.
> > 
> > What am I missing?
> 
> We've discussed removing showModalDialog several times in the past and every
> time we made the decision to delay until we have a good alternative
> implemented (<dialog> element). Not to say that you cannot get that
> particular discussion started again. I am just telling you removing
> showModalDialog() at this point will be controversial.
> 
> Also, I see good patches landing for <dialog> support so maybe we won't have
> to wait long?

Yeah, Tim has done great job with <dialog> implementation.

Either way, apart from web content, there might be iOS apps using it, and there is WebKit2 GTK+ API for it, meaning removing it will take some consideration even after we ship the <dialog>.

For this particular issue issue, going with [CustomEnabled] for showModalDialog() seems like a good move.

[Mathieu Hofman]

The point of this bug is regarding the consistency of `getOwnPropertyNames` and `getOwnPropertyDescriptor` for `showModalDialog`.

If iOS doesn't have that property (undefined descriptor), it should not be included in the list of global property names. That should be independent of alternative implementations for the feature, as the feature is already missing. Or am I missing something?

[Alexey Shvayka]

(In reply to Mathieu Hofman from comment #7)
> The point of this bug is regarding the consistency of `getOwnPropertyNames`
> and `getOwnPropertyDescriptor` for `showModalDialog`.
> 
> If iOS doesn't have that property (undefined descriptor), it should not be
> included in the list of global property names. That should be independent of
> alternative implementations for the feature, as the feature is already
> missing. Or am I missing something?

You are absolutely right, it's just that removing the feature completely is one way of achieving that consistency (but we are not doing that).

The other ways would be to either a) remove showModalDialog-specific code from Window's getOwnPropertyDescriptor() and expose it in some other way (like via WebIDL attribute we use for experimental features) or b) add some more method overrides for Window, including getOwnPropertyNames(), to make all the methods consistent.

I would very much like to do the a).

[Mark S. Miller]

I am puzzled about why this issue arises in the first place, and about the nature of the discussion about how to fix it. We're talking about object invariants that should be true for all objects reachable in the language, whether created by JS or by the host. As a universal regularity, it should not be the responsibility of each feature to separately decide whether to uphold the invariant. There should be a common mechanism they all use, so there would be little danger of any individual feature breaking these invariants.

[Alexey Shvayka]

(In reply to Mark S. Miller from comment #9)
> I am puzzled about why this issue arises in the first place, and about the
> nature of the discussion about how to fix it. We're talking about object
> invariants that should be true for all objects reachable in the language,
> whether created by JS or by the host. As a universal regularity, it should
> not be the responsibility of each feature to separately decide whether to
> uphold the invariant. There should be a common mechanism they all use, so
> there would be little danger of any individual feature breaking these
> invariants.

In WebKit, it's up for individual classes to ensure their overrides of ordinary methods abide to the invariants.
Some of those classes implement the spec directly, like ProxyObject, while some others override ordinary methods for performance.
While we are always happy to remove or fix these overrides (recently we resolved similar issues with RegExp legacy features and sloppy function's caller / arguments), I don't see how we can enforce the essential invariants in our code base except for rigorous testing and code review.

[Alexey Shvayka]

Another related and very troubling inconsistency in r288438 (MiniBrowser):

> window.showModalDialog
< undefined

> showModalDialog
< function showModalDialog() { [native code] }

STP 134 returns a function for both.

[Alexey Shvayka]

Created attachment 449961 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 449961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449961&action=review

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:132
> +    putDirectCustomAccessor(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().showModalDialogPublicName(), CustomGetterSetter::create(vm, showModalDialogGetter, nullptr), static_cast<unsigned>(PropertyAttribute::CustomValue));

Do we need to have custom-accessor instead of just defining a function as a method?
Since it is a function (not attribute), I think we can just define it via `JSC_NATIVE_FUNCTION`.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #13)
> Comment on attachment 449961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449961&action=review
> 
> > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:132
> > +    putDirectCustomAccessor(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().showModalDialogPublicName(), CustomGetterSetter::create(vm, showModalDialogGetter, nullptr), static_cast<unsigned>(PropertyAttribute::CustomValue));
> 
> Do we need to have custom-accessor instead of just defining a function as a
> method?
> Since it is a function (not attribute), I think we can just define it via
> `JSC_NATIVE_FUNCTION`.

I believe we to do support a case when modals are initially (on Window init) disabled (function is currently `undefined` in this case) and then get enabled via an API.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #14)
> (In reply to Yusuke Suzuki from comment #13)
> > Comment on attachment 449961 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=449961&action=review
> > 
> > > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:132
> > > +    putDirectCustomAccessor(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().showModalDialogPublicName(), CustomGetterSetter::create(vm, showModalDialogGetter, nullptr), static_cast<unsigned>(PropertyAttribute::CustomValue));
> > 
> > Do we need to have custom-accessor instead of just defining a function as a
> > method?
> > Since it is a function (not attribute), I think we can just define it via
> > `JSC_NATIVE_FUNCTION`.
> 
> I believe we to do support a case when modals are initially (on Window init)
> disabled (function is currently `undefined` in this case) and then get
> enabled via an API.

In other words, there may be some code that relies on showModalDialog() working if its non-undefined.

[Yusuke Suzuki]

Comment on attachment 449961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449961&action=review

Ah, I misunderstood. r=me

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:510
> +    return encodedJSUndefined();

Use { } in new code.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:528
> +        return encodedJSUndefined();

Use { } in new code.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #16)
> Comment on attachment 449961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449961&action=review
> 
> Ah, I misunderstood. r=me

Thank you!

> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:510
> > +    return encodedJSUndefined();
> 
> Use { } in new code.
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:528
> > +        return encodedJSUndefined();
> 
> Use { } in new code.

One clarification: isn't { } equivalent to encodedJSValue() (empty value), while we need to return encoded `undefined` here? Passing empty value to userland code may causes crashes, right?

[Yusuke Suzuki]

Comment on attachment 449961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449961&action=review

>>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:510
>>> +    return encodedJSUndefined();
>> 
>> Use { } in new code.
> 
> One clarification: isn't { } equivalent to encodedJSValue() (empty value), while we need to return encoded `undefined` here? Passing empty value to userland code may causes crashes, right?

Ah, oops. I was thinking it is encodedJSValue(). Is there any reason using `encodedJSUndefined` over `JSValue::encode(jsUndefined())`? I rarely see `encodedJSUndefined()`.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #18)
> Comment on attachment 449961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449961&action=review
> 
> >>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:510
> >>> +    return encodedJSUndefined();
> >> 
> >> Use { } in new code.
> > 
> > One clarification: isn't { } equivalent to encodedJSValue() (empty value), while we need to return encoded `undefined` here? Passing empty value to userland code may causes crashes, right?
> 
> Ah, oops. I was thinking it is encodedJSValue(). Is there any reason using
> `encodedJSUndefined` over `JSValue::encode(jsUndefined())`? I rarely see
> `encodedJSUndefined()`.

encodedJSUndefined() is indeed very rare outside jsc.cpp / JSDollarVM.cpp / ObjC API. Will change to `JSValue::encode(jsUndefined())`.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #18)
> Is there any reason using `encodedJSUndefined` over `JSValue::encode(jsUndefined())`?

No reason at all, it's the same thing.

[Alexey Shvayka]

Committed r288763 (246551@trunk): <https://commits.webkit.org/246551@trunk>