1663992 - Changes in bug 1652244 break Twitter image downloads with Strict TP enabled

Status: VERIFIED FIXED

(Core :: Privacy: Anti-Tracking, defect, P2)

Description

[Dennis Schubert [:denschub]]

STR:

1. Toggle TP to Strict in the settings.
2. Navigate to the direct URL of a Twitter image, like https://pbs.twimg.com/media/EhfHw9HXYAQ8HyW?format=png
3. Try saving it with Ctrl/Cmd-S, or right click and "Save Image As..."

Expected:

File is downloaded somewhere.

Actual:

File fails to download, with a "Failed" in the download manager. Looking at the browser console/browser devtools, it's indicated that the request got blocked for "social tracking".

---

This is a regression of the changes in bug 1652244. I'm marking this as such, but since that patch fixed another broken thing, it's probably not too easy to resolve. :)

Requesting tracking just for processes sake, Twitter images appear to be working fine otherwise, so this is probably a bit niche.

Comment 1

[Dennis Schubert [:denschub]]

I'd needinfo :dimi, but they are OOO until Sep 20. :timhuang, since you reviewed the patch, maybe you can redirect accordingly?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

This is a non-default configuration and we're about to build the final beta of the 81 cycle. I don't think this needs to be tracked but I'm open to stronger arguments for why it should.

Comment 3

[Tim Huang[:timhuang]]

The problem occurs in AntiTrackingUtils::ComputeIsThirdPartyToTopWindow(). Because the channel used to download the image doesn't have a browsing context. So, AntiTrackingUtils::ComputeIsThirdPartyToTopWindow won't update the flag 'IsThirdPartyToTopWindow' correctly. I think in this case, we should check if the principal of the channel is third-party to its loading principal.

Comment 4

[Tim Huang[:timhuang]]

Attachment: Bug 1663992 - Check loading principal if there is no browsing context in AntiTrackingUtils::ComputeIsThirdPartyToTopWindow(). r?dimi

Comment 5

[Tim Huang[:timhuang]]

Attachment: Bug 1663992 - Add a test. r?dimi

Depends on D90816

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71c87c0e1d24
Check loading principal if there is no browsing context in AntiTrackingUtils::ComputeIsThirdPartyToTopWindow(). r=dimi
https://hg.mozilla.org/integration/autoland/rev/0fc2e9fdf1f3
Add a test. r=dimi

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/71c87c0e1d24
https://hg.mozilla.org/mozilla-central/rev/0fc2e9fdf1f3

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:timhuang, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Tim Huang[:timhuang]]

Comment on attachment 9176815 [details]
Bug 1663992 - Check loading principal if there is no browsing context in AntiTrackingUtils::ComputeIsThirdPartyToTopWindow(). r?dimi

Beta/Release Uplift Approval Request

• User impact if declined: The 'Save Image As' will break in image documents of social media sites when Strict mode is enabled.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch only changes the behavior of documents that have no browsingContext. For documents that have browsingContext, the behavior remains the same. And It matches the behavior before Bug 1652244.
• String changes made/needed: None

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9176815 [details]
Bug 1663992 - Check loading principal if there is no browsing context in AntiTrackingUtils::ComputeIsThirdPartyToTopWindow(). r?dimi

approved for 82.0b4

Comment 11

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0775baee658f
https://hg.mozilla.org/releases/mozilla-beta/rev/3d60f0796c6e

Comment 13

[Julien Cristau [:jcristau]]

It sounds like this is the root cause behind bug 1665368 which does affect 81, so adjusting tracking flags.

Comment 14

[Pascal Chevrel:pascalc]

Tim, can the patch in this bug be uplifted to the 81 branch to fix bug 1665368? Could you request the uplift to release? Thanks

Comment 15

[Julien Cristau [:jcristau]]

Maybe Dimi can help here as well.

Comment 16

[Dimi Lee [:dimi]]

Comment on attachment 9176815 [details]
Bug 1663992 - Check loading principal if there is no browsing context in AntiTrackingUtils::ComputeIsThirdPartyToTopWindow(). r?dimi

Beta/Release Uplift Approval Request

• User impact if declined: Breakage in twitter.com when tracking protection strict mode is enabled
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. First set your privacy level to “standard”

2. Load twitter.com. Go to about:serviceworkers and confirm there’s a sw for twitter.com (via about:serviceworkers)
3. Now change your privacy level to “strict”
4. Go to twitter and try anything in the app that forces a full page load eg: logging out or switching accounts.
   Then you should see the error message (See Bug 1665368).

Details can be found in https://bugzilla.mozilla.org/show_bug.cgi?id=1665368#c27

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch has already been tested in 82 and 83. We have seen any issues so far.
  We also have a test to cover this.
• String changes made/needed:

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9176815 [details]
Bug 1663992 - Check loading principal if there is no browsing context in AntiTrackingUtils::ComputeIsThirdPartyToTopWindow(). r?dimi

Driver for 81.0.2, uplift approved for 81.0.2 thanks

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/bdb93bd9b26d

Comment 19

[Cristian Fogel [:cfogel]]

Confirmed issue with 82.0a1 (2020-09-09) on Windows 10.
Fix verified with :dcicas using 83.0a1 (2020-10-12), 82.0b9, 81.0.2 on Windows 10, macOS 10.15, Ubuntu 16x32.