ctm / mb2-doc

Mb2, poker software
https://devctm.com
7 stars 2 forks source link

Pop-out chat breakage #1403

Closed ctm closed 1 month ago

ctm commented 2 months ago

Inspect the pop-out code to see if we do anything obviously stupid and if so, fix it. If not, send email asking for help.

A user reported:

Opened the game, watched a bit of a couple hands...

Hit pop-out on the chat, got some unintelligible error popup, and it froze the game. I watched for a minute or so and so no updates of table or chat. Closing the chat window that opened didn't change anything.

Note that I am on FireFox with popups disabled (which joining the table scolded me about), but it all seemed to work anyway before popping the chat.

I thought this would be trivially reproducible, but I haven't been able to reproduce it locally using Firefox 125.0.3 on macOS 14.4.1.

I know that we jump through hoops to deal with blocked table pop-ups, but I don't remember what we do if a chat window is blocked, so I'll look at that "now". However, when trying to reproduce, none of my attempts to open a pop-out chat window failed, even though Firefox was blocking pop-out Table windows. So that means if there's broken error handling of pop-out chat windows (e.g., unwrap()), I personally wouldn't run into it.

ctm commented 2 months ago

Since this was reported on Firefox, I decided to revisit a known problem with Firefox's Web Socket implementation (#827). After following some links in Bugzilla, I see that the issue that caused me to write the disclaimer has been assigned. So, I'm going to reopen that issue and verify that it's still present so that I can volunteer to help.

ctm commented 2 months ago

Sure enough:

            Msg::Undock => {
                self.undocked_chat = Some(
                    chat::Parent::new(self.link.clone(), &format!("{}/chat", self.to_track))
                        .unwrap(),
                );
            }

Obviously, unwrap() is not the right thing to do here. It may be possible to generalize the code that I wrote for dealing with blocked table pop-ups and use it here, but I'll need to figure a way to trigger this locally so that I can actually test whatever solution I come up with.

ctm commented 2 months ago

FWIW, I tried the PopUp Blocker (strict) Firefox extension and although that interfered with pop-out chat, it didn't trigger the bug. So, I've asked the original reporter for some more details in hope that I can reproduce exactly what is happening, but in the meantime, I can simply have my Msg::Undock arm pretend that chat::Parent::new failed and have it do something.

ctm commented 2 months ago

I've made it so that a failure brings up a warning notification that says "Could not create chat window" and dumps an unformatted JsValue if there's an error code. We're trying to open the window after a user has clicked a button and that's typically the magic necessary to get a pop-up blocker to not block a popped up window. IOW, we don't have a fallback like we do when the lobby fails to open a table window.

So, I've deployed the code with the notification, but will revisit this issue when I hear back from the original reporter. If a pop-up blocker extension is involved, I may be able to either read the extension source or correspond with the extension writer to find a magic dance to do to get a chat window to pop-up.

Whee!

AngryCochinoPup commented 2 months ago

image

Ubuntu 23.10 - no specific popup-blockers, just the built-in mozilla one.

ctm commented 2 months ago

Thanks! I'll try to get it running under VirtualBox.

ctm commented 2 months ago

I couldn't get Mb2 to fail to create the pop-out chat window. I tried with a fresh Ubuntu.10 install, using the version of Firefore that was initially installed, then upgrading to 125.0.3.

Screenshot 2024-05-05 at 19 17 06

@AngryCochinoPup Can you use demo mode and then try pop-out chat from there? I'm guessing it'll fail, but with a notification of a failure to create the window, rather than the crash. OTOH, maybe it'll just work. Thanks!

ctm commented 1 month ago

I'm closing this, because I fixed the problem I created by using unwrap(), but I can't get Firefox to test my fix. I'll reopen this issue if I learn anything more.