1188378 - Tracking protection breaks share (Twitter specifically)

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Eric Shepherd [:sheppy]]

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."

Comment 1

[Shane Caraveo (:mixedpuppy)]

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?

Comment 2

[Eric Shepherd [:sheppy]]

(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").

Comment 3

[Shane Caraveo (:mixedpuppy)]

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.

Comment 4

[Eric Shepherd [:sheppy]]

(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.

Comment 5

[Eric Shepherd [:sheppy]]

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.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Javaun, is there a way to either make TP ignore UI iframes we use, or can we add certain domains to be ignored?

Comment 7

[Javaun Moradi [:javaun]]

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.

Comment 8

[Javaun Moradi [:javaun]]

Does FB work, btw?

Comment 9

[Eric Shepherd [:sheppy]]

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

Yes. Facebook works fine.

Comment 10

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb388401a6e7

Comment 11

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: make social error handler much less aggressive

Comment 14

[Shane Caraveo (:mixedpuppy)]

[Tracking Requested - why for this release]:
With TP entering release with 42, I'd like to uplift this up through 42.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

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+ :)

Comment 16

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: make social error handler much less aggressive

Comment 19

[Javaun Moradi [:javaun]]

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.

Comment 20

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 21

[Shane Caraveo (:mixedpuppy)]

@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.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

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

Comment 23

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebf3945ae378

Comment 24

[François Marier [:francois]]

(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?

Comment 25

[Shane Caraveo (:mixedpuppy)]

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)

Comment 26

[François Marier [:francois]]

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.

Comment 27

[Shane Caraveo (:mixedpuppy)]

I think the risk is very low and am personally fine with uplifting the patch.

Comment 28

[Javaun Moradi [:javaun]]

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.

Comment 29

[Shane Caraveo (:mixedpuppy)]

The risk with this patch, while never zero, is smaller than this comment.

Comment 30

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/f354f556b59136bc052e5b2c57d7d28e7242de37
Bug 1188378 fix loading share pages when TP is turned on, r=markh

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f354f556b591

Comment 32

[Shane Caraveo (:mixedpuppy)]

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.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

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

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8dc067c112dc

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5953cc971e4c

Comment 36

[Petruta Horea [:phorea], Desktop QA]

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.