search

mozilla-mobile / firefox-ios

Firefox for iOS
Mozilla Public License 2.0
12.12k stars 2.89k forks source link

Incorrect redirect to the latest visited YouTube video, after undoing close tab action #15837

Open alinamoldovan84 opened 1 year ago

alinamoldovan84 commented 1 year ago

Steps to reproduce

  1. Navigate to m.youtube.com
  2. Open a video
  3. Open the tab tray and close the YouTube tab
  4. Tap to undo close tab action

Expected behavior

The video opened in step 2 is playing

Actual behavior

User is redirected to m.youtube.com

Device & build information

┆Issue is synchronized with this Jira Task

purifier1990 commented 1 year ago

Hi I did some debug with our App and seems that what we do is right, I think the problem is from youtube side, chatGPT seems to say that Youtube's security policy is not allowed to restore video links, to do that, we should use Youtube player's api to do that which is not we want. I'm working on a sample project to repro this, and if it can, then maybe this is not what we can fix from our side.

data-sync-user commented 1 year ago

➤ Daniela Arcese commented:

Seems like a timing issue. If you background the app before re-opening the tab, it usually comes back to the video page.

data-sync-user commented 2 months ago

➤ Alexandru Farcasanu commented:

Update: Closed the PR for this. Most probably we can’t fix it because of some Youtube security policies.

Will still investigate it.

denschub commented 2 months ago

tl;dr: This is a clear browser bug, and not a WebCompat issue, and not specific to YouTube.

I'm in the habit of writing GitHub comments while I debug, and I usually share the whole thing in WebCompat bugs, so it's clear how I got to where I am, so I'll share the full comment here, too.

I'm looking at this after chatting with Alex on Slack, where this was suspected to be a WebCompat issue. Alex suggested that something happens inside YouTube that would redirect us back to the home page when restoring a YouTube tab, but apparently only on YouTube.

I still don't understand what "Youtube's security policy" should be. Initially, after the chat on Slack, I assumed it could maybe refer to CSP, but that'd make no sense, as CSPs cannot influence page navigation. In fact, very little things can affect page navigation at all. This "security policy" is most likely something ChatGPT made up to provide an answer, but - like most things generated by LLMs - it's not attached to reality.

YouTube isn't doing super fancy. They're not using native browser navigation to switch from the front page to the video page, but they do use pushState to do the navigation, which is a standard thing used in a lot of web apps. And indeed, this just seems to be an issue where the undo feature simply doesn't work when we changed the URL via pushState.

I just so happened to have a testcase for exactly this already on my servers for an earlier WebCompat diagnosis. If you open this URL, you'll see a big 0. If you click the "next" button, it will show a 1, and the URL changes to 1.html. This goes up to 3. You'll note that you can use the borwser's back/forward buttons just fine, even though the page never reloads. If you try this testcase on iOS, it behaves like the YouTube issue. The number increases, and the URL changes (even though you can't see it), but closing the tab and undo'ing it will restore the wrong number.

So this is not specific to YouTube.

I will note that the browser's history and the "recently closed" view accurately track the state, and do restore the right URL when clicking on it, so this issue is isolated to the "undo" button.

Looking at the implementation, it looks like we end up in the function undoCloseTab() from LegacyTabManager. This function gets self.backupCloseTab and restores that. Oddly enough, if I look at backupCloseTab.url, the tab has the correct URL set, but that's not what we restore. For example, if I click "next" until I'm at 2, looking at backupCloseTab.url shows https://stuff.overengineer.dev/mozilla/testcases/pushstate-nav/2.html, but the tab that gets restored shows a zero, and the URL bar says https://stuff.overengineer.dev/mozilla/testcases/pushstate-nav/0.html. So somehow that tab's url property does not match the URL that actually gets loaded.

Since I'm looking at a file with legacy in its name, I'll stop here and not dig further. But this is very clearly a product bug, and not a WebCompat issue.

data-sync-user commented 2 months ago

➤ Alexandru Farcasanu commented:

I still tried to find a fix for this issue. Unfortunately, I can’t identify the problem. Don’t want to waste too much time on it. Someone else may take look on it.

cc: Norberto Andres Furlan

data-sync-user commented 2 months ago

➤ Roux Buciu commented:

Nishant Bhasin to fill in details re: investigation

data-sync-user commented 5 days ago

➤ Diana Andreea Barladeanu commented:

Verified as fixed on v130 (44347), with iPhone 15 (17.5).

!RPReplay_Final1723453365.mp4|width=590,height=1280,alt="RPReplay_Final1723453365.mp4"!