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

[De-AMP] JS navigations from canonical -> AMP cause redirect loop #23320

Closed
ShivanKaul opened this issue Jun 8, 2022 · 3 comments · Fixed by brave/brave-core#13674
Closed

Comments

@ShivanKaul
Copy link
Collaborator

ShivanKaul commented Jun 8, 2022

Description

Some canonical pages manipulate window.location to direct a site back to the AMP page. We should catch when this is happening and stop De-AMPing.

Steps to Reproduce

  1. Make sure De-AMP pref is turned on
  2. Go to https://thehustle.co/06022022-elvis-vegas-weddings
  3. Alternatively, go to test-js-redirect-amp.html

Actual result:

Redirect loop

Expected result:

Not a loop, we should stop either on canonical page or AMP page.

Reproduces how often:

Easily

The issue here is that it's not technically a redirect that we can check for throttle-side. The page load for the canonical page completed successfully, and then the page is navigated away to the AMP page.

@kjozwiak
Copy link
Member

kjozwiak commented Jun 14, 2022

This will go out in the next 1.39.x release if we need to either push a HF (hotfix) or we get a minor C102 chromium bump. If we don't end up releasing another 1.39.x, we'll close off the 1.39.x - Release #5 milestone and move this issue into the 1.40.x milestone.

@Uni-verse
Copy link
Contributor

Uni-verse commented Jun 14, 2022

Verification PASSED on Samsung Tab S7 using

Brave	1.39.123 Chromium: 102.0.5005.125 (Official Build) (64-bit) 
Revision	07573b09e385116e620ed12d5ef2402c4bfa929f-refs/branch-heads/5005@{#1159}
OS	Android 12; Build/SP1A.210812.016
case 1 case 2
screenshot-1655226694771 screenshot-1655226635610

Verification PASSED on Pixel 6 running Android 13 running the following build(s):

Brave | 1.39.123 Chromium: 102.0.5005.125 (Official Build) (64-bit)
--- | ---
Revision | 07573b09e385116e620ed12d5ef2402c4bfa929f-refs/branch-heads/5005@{#1159}
OS | Android 12; Build/TPBB.220414.018

Went through all the STR/Cases (specifically the websites being listed) via brave/brave-core#13674 (comment) and ensured that all the websites mentioned loaded without running into any looping issues.

@kjozwiak
Copy link
Member

kjozwiak commented Jun 21, 2022

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.40.105 Chromium: 103.0.5060.53 (Official Build) (64-bit)
-- | --
Revision | a1711811edd74ff1cf2150f36ffa3b0dae40b17f-refs/branch-heads/5060@{#853}
OS | Windows 11 Version 21H2 (Build 22000.739)

Went through all the STR/Cases (specifically the websites being listed) via brave/brave-core#13674 (comment) and ensured that all the websites mentioned loaded without running into any looping issues.

de-amp enabled

Example Example
image image

de-amp disabled

Example Example
image image

Verification PASSED on PopOS 22.04 x64 using the following build(s):

Brave | 1.40.105 Chromium: 103.0.5060.53 (Official Build) (64-bit)
--- | ---
Revision | a1711811edd74ff1cf2150f36ffa3b0dae40b17f-refs/branch-heads/5060@{#853}
OS | Linux

Went through all the STR/Cases (specifically the websites being listed) via brave/brave-core#13674 (comment) and ensured that all the websites mentioned loaded without running into any looping issues.

de-amp enabled

Example Example
image image

de-amp disabled

Example Example
image image

Verification PASSED on macOS 12.4 Monterey x64 using the following build(s):

Brave | 1.40.105 Chromium: 103.0.5060.53 (Official Build) (x86_64)
--- | ---
Revision | a1711811edd74ff1cf2150f36ffa3b0dae40b17f-refs/branch-heads/5060@{#853}
OS | macOS Version 12.4 (Build 21F79)

Went through all the STR/Cases (specifically the websites being listed) via brave/brave-core#13674 (comment) and ensured that all the websites mentioned loaded without running into any looping issues.

de-amp enabled

Example Example
Screen Shot 2022-06-22 at 1 00 29 AM Screen Shot 2022-06-22 at 1 00 47 AM

de-amp disabled

Example Example
Screen Shot 2022-06-22 at 1 00 29 AM Screen Shot 2022-06-22 at 1 00 47 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment