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.
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.
<rdar://problem/86749078>
(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.
(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?
(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?
(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.
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?
(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).
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 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.
Another related and very troubling inconsistency in r288438 (MiniBrowser): > window.showModalDialog < undefined > showModalDialog < function showModalDialog() { [native code] } STP 134 returns a function for both.
Created attachment 449961 [details] Patch
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`.
(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 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.
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.
(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?
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()`.
(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())`.
(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.
Committed r288763 (246551@trunk): <https://commits.webkit.org/246551@trunk>