Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression(252759@main) Unable to log into pandora.com #5307

Closed
wants to merge 0 commits into from

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Oct 12, 2022

93d2762

Regression(252759@main) Unable to log into pandora.com
https://bugs.webkit.org/show_bug.cgi?id=246430
rdar://100243111

Reviewed by Geoffrey Garen and Ryosuke Niwa.

We're unable to log into pandora.com since un-exposing window.showModalDialog()
in 252759@main. Pandora does not actually seem to call showModalDialog() but
they are calling its getter on the Window object. It is unclear why this is
causing log in to fail since there is no JS error (and other browsers don't
expose showModalDialog either).

To resolve the issue for now, I am adding a quirk to re-expose showModalDialog
on pandora.com. However, since they're not calling it, I am merely using
`undefined` as value for the property.

* Source/WebCore/bindings/js/JSDOMWindowBase.cpp:
(WebCore::JSDOMWindowBase::finishCreation):
* Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSC_DEFINE_CUSTOM_GETTER):
* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::shouldExposeShowModalDialog const):
* Source/WebCore/page/Quirks.h:

Canonical link: https://commits.webkit.org/255521@main

01c02dc

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl ❌ πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios   πŸ§ͺ api-mac   πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim ❌ πŸ§ͺ mac-wk2
❌ πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@cdumez cdumez self-assigned this Oct 12, 2022
@cdumez cdumez added WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build labels Oct 12, 2022
Copy link
Contributor

@geoffreygaren geoffreygaren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

Do we have to expose the real functionality, or can we expose a dummy function that doesn't do anything? (That would be nice, because it would keep us in the state where we can delete the implementation.)

@cdumez
Copy link
Contributor Author

cdumez commented Oct 12, 2022

r=me

Do we have to expose the real functionality, or can we expose a dummy function that doesn't do anything? (That would be nice, because it would keep us in the state where we can delete the implementation.)

I'll check. Since they're not calling it, odds are we can return a dummy function.

@cdumez
Copy link
Contributor Author

cdumez commented Oct 12, 2022

r=me
Do we have to expose the real functionality, or can we expose a dummy function that doesn't do anything? (That would be nice, because it would keep us in the state where we can delete the implementation.)

I'll check. Since they're not calling it, odds are we can return a dummy function.

We don't even need a dummy function, it seems that having the property exist but be undefined is sufficient to fix the log in issue. This is what I did in this latest iteration.

@geoffreygaren
Copy link
Contributor

We don't even need a dummy function, it seems that having the property exist but be undefined is sufficient to fix the log in issue. This is what I did in this latest iteration.

LOL awesome.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 13, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 13, 2022
@cdumez cdumez requested review from rniwa and a team as code owners October 13, 2022 15:09
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 13, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Oct 13, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 13, 2022
@cdumez cdumez added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Oct 13, 2022
@webkit-commit-queue webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Oct 14, 2022
@cdumez cdumez added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Oct 14, 2022
@webkit-commit-queue
Copy link
Collaborator

Committed 255521@main (93d2762): https://commits.webkit.org/255521@main

Reviewed commits have been landed. Closing PR #5307 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Oct 14, 2022
@karlcow
Copy link
Member

karlcow commented Oct 28, 2022

for the paper trail.
#2658

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
7 participants