Tracking protection breaks share (Twitter specifically)

VERIFIED FIXED in Firefox 42
(NeedInfo from)

Status

defect
VERIFIED FIXED
4 years ago
3 months ago

People

(Reporter: sheppy, Assigned: mixedpuppy, NeedInfo)

Tracking

unspecified
Firefox 44
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox41 unaffected, firefox42+ verified, firefox43 verified, firefox44 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
For a couple of weeks now, both dev edition and nightly have been unable to share on Twitter using the built-in sharing feature. Clicking the button and choosing Twitter as the destination results in a window which is mostly empty but has the text "Nightly is unable to connect with Twitter right now."
(Assignee)

Comment 1

4 years ago
I haven't tried nightly lately, but dev edition (osx) is working just fine for me.  That error page would be the result of some network error (404s, unable to connect to server, etc).  Do you see any issues in the network tab in the browser toolbox when you load that page?
Flags: needinfo?(eshepherd)
(Reporter)

Comment 2

4 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> I haven't tried nightly lately, but dev edition (osx) is working just fine
> for me.  That error page would be the result of some network error (404s,
> unable to connect to server, etc).  Do you see any issues in the network tab
> in the browser toolbox when you load that page?

I get this error in the browser toolbox:

Content Security Policy: The page's settings blocked the loading of a resource at self ("script-src 'unsafe-inline' 'nonce-zunGgsYU2sHOcS1MpXLe7g==' 'unsafe-eval' https://twitter.com https://*.twimg.com https://twitter.com https://ton.twitter.com https://platform.twitter.com https://syndication.twitter.com https://analytics.twitter.com https://www.google-analytics.com https://ssl.google-analytics.com https://connect.facebook.net https://cm.g.doubleclick.net https://api.twitter.com https://graph.facebook.com").
Flags: needinfo?(eshepherd)
(Assignee)

Comment 3

4 years ago
Do you have any content blocking enabled?  e.g. adblock plus, tracking protection, etc.  If so, does the page work when they are disabled and can you narrow it down further?  

I tried a clean profile (dev ed) with and without e10s, it still works for me.
Flags: needinfo?(eshepherd)
(Reporter)

Comment 4

4 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> Do you have any content blocking enabled?  e.g. adblock plus, tracking
> protection, etc.  If so, does the page work when they are disabled and can
> you narrow it down further?  
> 
> I tried a clean profile (dev ed) with and without e10s, it still works for
> me.

Yes, I have Firefox's built-in tracking protection enabled. This tracking protection feature seems to break a lot of things. We should either remove it or at least work around the most popular sites somehow. Or at least around our built-in features like this one.
Flags: needinfo?(eshepherd)
(Reporter)

Comment 5

4 years ago
I have confirmed that disabling tracking protection lets this feature work correctly.

We should either dim/disable Twitter if tracking protection is on, or work around tracking protection in this special case of using our own UI to tweet.
(Assignee)

Updated

4 years ago
Summary: Twitter sharing integration has been broken for a while; error "unable to connect with Twitter right now" → Tracking protection breaks share (Twitter specifically)
(Assignee)

Comment 6

4 years ago
Javaun, is there a way to either make TP ignore UI iframes we use, or can we add certain domains to be ignored?
Flags: needinfo?(jmoradi)
Sorry I'm late to this one. I'd want a priv/sec review on this one, it's unclear if the breakage here is a bug or a feature. If something is fingerprintable from a UI frame, even one we call, it seems like it might be intended.
Flags: needinfo?(jmoradi)
Does FB work, btw?
(Reporter)

Comment 9

4 years ago
(In reply to Javaun Moradi [:javaun] from comment #8)
> Does FB work, btw?

Yes. Facebook works fine.
(Assignee)

Comment 11

4 years ago
(In reply to Javaun Moradi [:javaun] from comment #7)
> Sorry I'm late to this one. I'd want a priv/sec review on this one, it's
> unclear if the breakage here is a bug or a feature. If something is
> fingerprintable from a UI frame, even one we call, it seems like it might be
> intended.


Its a bug in the social error handler [which is way over aggressive] that is enhanced by tp blocking some scripts on the share pages, IMO a priv/sec review is not necessary.  Essentially, TP blocks ga.js from being loaded.  the social error handler considers any load failure an utter failure, which isn't true [ie. twitter works fine if ga.js fails to load].  The fix will be to ignore errors on resources being loaded by the primary page.  This is how the same page loaded in a tab works.

STR:
- turn on TP globally
- load https://twitter.com/intent/tweet/?url=http://www.mozilla.org in tab, it will load and work
- open the share panel and activate twitter

expected:
- twitter share panel should load

got:
- twitter share panel does not load, an error page appears

With the patch in comment #10 the twitter share panel now properly works with TP enabled.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1203323
(Assignee)

Comment 13

4 years ago
Assignee: nobody → mixedpuppy
Attachment #8669107 - Flags: review?(markh)
(Assignee)

Comment 14

4 years ago
[Tracking Requested - why for this release]:
With TP entering release with 42, I'd like to uplift this up through 42.
Comment on attachment 8669107 [details] [diff] [review]
make social error handler much less aggressive

Review of attachment 8669107 [details] [diff] [review]:
-----------------------------------------------------------------

Do we have anecdotes that we are hitting this problem for non-tracking related "minor" errors? If not, ISTM that it might be safer to use the STATE_BLOCKED_TRACKING_CONTENT flag. I guess I'm worried that if some important sub-resource failed to load for other reasons, we'll be leaving an un-refresh-able and broken panel in place, and that flag gives a mechanism to fix just the reported bug in a more focused way.

I don't have a real problem though, so if there's some reason we can't do that I'm still open to r+ :)
Tracking as it is a regression of a major new feature of 42.
Please request the uplift as soon as it landed in m-c.
(Assignee)

Comment 17

4 years ago
(In reply to Mark Hammond [:markh] from comment #15)
> Comment on attachment 8669107 [details] [diff] [review]
> make social error handler much less aggressive
> 
> Review of attachment 8669107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we have anecdotes that we are hitting this problem for non-tracking
> related "minor" errors? If not, ISTM that it might be safer to use the
> STATE_BLOCKED_TRACKING_CONTENT flag. I guess I'm worried that if some
> important sub-resource failed to load for other reasons, we'll be leaving an
> un-refresh-able and broken panel in place, and that flag gives a mechanism
> to fix just the reported bug in a more focused way.
> 
> I don't have a real problem though, so if there's some reason we can't do
> that I'm still open to r+ :)

My initial attempt was to use that flag.  That flag only happens in onSecurityChange.  During onStateChange we only get STATE_STOP.  When TP blocks, onSecurityChange doesn't get aRequest so I'm unable to examine the url (e.g. to keep track of what is blocked).  I have however made a small modification that will only ignore these load failures if the document is marked that it has blocked content.
(Assignee)

Comment 18

4 years ago
Attachment #8669107 - Attachment is obsolete: true
Attachment #8669107 - Flags: review?(markh)
Attachment #8669913 - Flags: review?(markh)
Cc'ing Francois and Elan.

Is there any risk to 42 here? Does the code to fix the social error handler affect anything else in TP? We do not fully support running TP in regular mode, that's an about:config pref change.
Flags: needinfo?(francois)
Flags: needinfo?(elancaster)
(Assignee)

Comment 20

4 years ago
(In reply to Javaun Moradi [:javaun] from comment #19)
> Cc'ing Francois and Elan.
> 
> Is there any risk to 42 here? Does the code to fix the social error handler
> affect anything else in TP? We do not fully support running TP in regular
> mode, that's an about:config pref change.

This touch anything in TP and wont affect anything in TP.
(Assignee)

Comment 21

4 years ago
@javaun: however, on second thought, if tp is not supported normally in 42, we don't need to uplift to 42.  If it will be supported in 43, then we should uplift to that.
Flags: needinfo?(jmoradi)
Comment on attachment 8669913 [details] [diff] [review]
make social error handler much less aggressive

Review of attachment 8669913 [details] [diff] [review]:
-----------------------------------------------------------------

sgtm

::: browser/base/content/social-content.js
@@ +91,5 @@
> +      if (docShell.hasTrackingContentBlocked) {
> +        let frame = docShell.chromeEventHandler;
> +        let src = frame.getAttribute("src");
> +        if (aRequest && aRequest.name != src) {
> +          Cu.reportError("ignoring error for "+aRequest.name);

nit: spaces around operators
Attachment #8669913 - Flags: review?(markh) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> @javaun: however, on second thought, if tp is not supported normally in 42,
> we don't need to uplift to 42.  If it will be supported in 43, then we
> should uplift to that.

TP in Private Browsing is not only supported in 42, but it's also turned ON by default for everyone. Wouldn't this bug affect that?
Flags: needinfo?(francois)
(Assignee)

Comment 25

4 years ago
Social stuff was never enabled in Private Browsing mode due to the Frameworker which could leak between PB and non-PB.  IMO this bug has to do with people who have toggled the pref until such a time that TP is supported in normal windows (or frameworker is removed)
Ok, that makes sense.

I think it still makes sense to uplift as much as we can if it's not too risky. Users enabling TP globally is not something we should discourage even if we're not officially promoting it yet.
(Assignee)

Comment 27

4 years ago
I think the risk is very low and am personally fine with uplifting the patch.
Agree that we don't want to break functionality for users who have enabled TP in normal mode (via about:config), even though we don't fully support TP in normal mode. That said, my concern is introducing risk to 42. TP has been available in normal mode via about:config since Fx35 or so. I'm wondering if this fix should instead ride the trains. We are moving towards beta 5, and don't have a lot of time to catch new regressions. 

Could this changing of the social error handler break something else in Social API? Or could it break anything else? 

What is the overall risk to stability of 42 here vs. the gain for fixing this edge case for users running TP in normal mode, which is currently believed to be in the thousands.
Flags: needinfo?(jmoradi)
(Assignee)

Comment 29

4 years ago
The risk with this patch, while never zero, is smaller than this comment.
https://hg.mozilla.org/mozilla-central/rev/f354f556b591
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Assignee)

Comment 32

4 years ago
Comment on attachment 8669913 [details] [diff] [review]
make social error handler much less aggressive

Approval Request Comment
[Feature/regressing bug #]:tracking protection
[User impact if declined]:some social widgets may not work if a user has enabled tp via about:config
[Describe test coverage new/current, TreeHerder]:social error handler has numerous current tests, no new test for tp specifically
[Risks and why]: very low, code changes are only hit if an error loading happens in a social panel (eg. share), and most of the code change also requires user to have turned on tp in about:config
[String/UUID change made/needed]:none

On the fence about uplifting to beta since it does require a user flipping a pref via about:config, but I also have a feeling we'll see more users turning on tp once word gets out.
Attachment #8669913 - Flags: approval-mozilla-beta?
Attachment #8669913 - Flags: approval-mozilla-aurora?
Comment on attachment 8669913 [details] [diff] [review]
make social error handler much less aggressive

Sure, I don't think this is going to affect many people but there is just a tiny risk that it will break anything.
Should be in 42 beta 5
Attachment #8669913 - Flags: approval-mozilla-beta?
Attachment #8669913 - Flags: approval-mozilla-beta+
Attachment #8669913 - Flags: approval-mozilla-aurora?
Attachment #8669913 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Reproduced with Nightly 2015-07-27 using the steps from comment 11.

Verified as fixed on Firefox 42 beta 6, Aurora 43.0a2 and Nightly 44.0a1 2015-10-12 under Win 7 64-bit and Mac OS X 10.9.5.

Updated

3 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.