1250770 - With HTTPS Everywhere installed, YouTube HTML5 embedded video control buttons are missing (& so are all SVG <use> elements on any page that HTTPS Everywhere redirects)

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 0. Install HTTPS Everywhere from https://www.eff.org/Https-everywhere and restart Firefox to complete installation.

 1. Visit http://www.youtube.com/embed/C4qgAaxB_pc and click the video to start playing.  (NOTE: this is HTTP, not HTTPS. That is important.)

ACTUAL RESULTS:
Controls (e.g. play button) are not visible.

EXPECTED RESULTS:
Controls should be visible.

This is extremely similar (symptom-wise) to bug 1244495, except bug 1244495 has been fixed, via bug 1247733.  But that fix doesn't seem to help if you have HTTPS Everywhere installed.

Comment 1

[Daniel Holbert [:dholbert]]

I strongly suspect this traces back to the "80" in this line:
>    newuri.init(CI.nsIStandardURL.URLTYPE_STANDARD, 80,
>                newurl, uri.originCharset, null);
https://github.com/EFForg/https-everywhere/blob/fbfaac74e0d2e15227ce69421028c2b337db66aa/src/chrome/content/code/HTTPSRules.js#L163

This ends up creating newuri (generally an HTTPS URI) with its mDefaultPort == 80, which is wrong. And this produces a URI-checking failure similar to the one in bug 1247733. (except in this case, it comes from this line of JS inserting the wrong hardcoded mDefaultPort, rather than HttpBaseChannel::GetSecureUpgradedURI)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: dholbert-to-miketaylr.xml helper-file

Here's an HTTPS ruleset to trigger this bug...
 - with a simpler testcase than YouTube.
 - with source & destination URLs that are *both* HTTPS.

STR with this XML file:
 1. Install HTTPS Everywhere.
 2. Copy this file into your profile's "HTTPSEverywhereUserRules" subdirectory.
 3. (re)start Firefox.
 4. Visit https://dholbert.org/ (note that HTTPS is just fine here)

EXPECTED RESULTS: I should be redirected to https://miketaylr.com/bzla/ytplay.html and the white "play" icon should show up.

ACTUAL RESULTS: I'm redirected to https://miketaylr.com/bzla/ytplay.html, and the white "play" icon *does not* show up.

The URI comparison that fails is here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsReferencedElement.cpp?rev=e8c7dfe727cd#90
...and just as in bug 1247733 comment 3, the only thing that's mismatching is mDefaultPort. (The element's document's GetDocumentURI() returns a nsStandardURL with mDefaultPort == 80, but with mSpec set to "https://miketaylr.com/bzla/ytplay.html")

Note that it's particularly odd that we've got something with port 80 in this scenario, because the URI we're redirecting from (https://dholbert.org) is HTTPS -- not HTTP.

From a bit of gdb debugging, I'm pretty sure this buggy "80" comes from the init() call mentioned in my previous comment. So, this is a HTTPS Everywhere add-on bug.

Comment 3

[Daniel Holbert [:dholbert]]

I can confirm that if I modify HTTPS Everywhere & replace "80" with "443" in that line, I get EXPECTED RESULTS for both sets of STR here. (comment 0 and comment 2)

Comment 4

[Daniel Holbert [:dholbert]]

I filed https://github.com/EFForg/https-everywhere/issues/4220 on this and just posted a pull request https://github.com/EFForg/https-everywhere/issues/4221

Comment 5

[avada]

I've been experiencing an issue like this sporadically in the past several months and am using this addon.

I can only reproduce it in mozilla-central (47) and stable (44) though, and not on aurora or beta. Addon versions 5.1.3/5.1.4. (It was recently updated)
It's rather weird that the two updated channels furtherst apart show this issue.

Which to me suggests it's not purely an addon bug. Particularly since I've been experiencing this since probably 44 was on nightly, or earlier.

To further complicate things the issue does not show when I disable https-everywhere on nightly. BUT still happens if I do this on stable.

Comment 6

[avada]

(In reply to avada from comment #5)

> I can only reproduce it in mozilla-central (47) and stable (44) though, and
> To further complicate things the issue does not show when I disable
> https-everywhere on nightly. BUT still happens if I do this on stable.

Ah, I forgot to check how far was 1247733 uplifted.
Since it was uplifted all the way to beta, but not stable, I guess my results on FF44 stable are irrelevant.

Anyway. The issue still doesn't show up on aurora and beta.

Comment 7

[Daniel Holbert [:dholbert]]

avada: It makes sense that this issue doesn't show up on Aurora & Beta, because the hackaround in bug 1249452 (fixing up potentially-busted URI objects) will hack around the HTTPS Everywhere issue as well.

The hackaround is not a correct fix, though; URI objects should really be created with the right default port (both in Firefox -- via the "correct" [non-hackaround] fix in bug 1247733 -- and in HTTPS Everywhere, via the fix that I posted on the GitHub issue here.)

Comment 8

[Daniel Holbert [:dholbert]]

(I didn't explicitly say, but meant to say: the hackaround in bug 1249452 *only* landed on Aurora & Beta. That's why this doesn't reproduce on those branches.)

Comment 9

[avada]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> (I didn't explicitly say, but meant to say: the hackaround in bug 1249452
> *only* landed on Aurora & Beta. That's why this doesn't reproduce on those
> branches.)

Huh. What's the rationale behind it? I've never seen anything done like this before.
Actually I was about to write that I can't reproduce it, when I decided to test with central. (I was on aurora)

BTW what will happen with this "hackaround"? Will it remain as 47 goes on to stable?

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to avada from comment #9)
> Huh. What's the rationale behind it?

That's off-topic for this bug -- if the comments on bug 1249452 don't already make the rationale clear (I think they do), please ask for clarification over there.

> BTW what will happen with this "hackaround"? Will it remain as 47 goes on to
> stable?

The hackaround only lives on our branches for 45 and 46; no other versions (including trunk, or future aurora/beta/release versions beyond these ones) will get it.

Comment 11

[Daniel Holbert [:dholbert]]

(See bug 1249452 comment 0 and bug 1249452 comment 5 in particular, as well as bug 1247733 comment 47 for a little more detail.)

Comment 12

[avada]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> (See bug 1249452 comment 0 and bug 1249452 comment 5 in particular, as well
> as bug 1247733 comment 47 for a little more detail.)

But isn't it worth having it around precisely because the issue brought light by this bug.
What if other extensions break this also? What if no-one's going to fix those/this addon?

Comment 13

[Daniel Holbert [:dholbert]]

We can address those problems when they come. There are an uncountable number of potential bugs that add-ons *could conceivably* have. Trying to predict & add hackarounds for them is not a winning strategy. :)

Adding short-term hackarounds for they may be a good strategy, but we don't want to keep those around forever; there's a performance, security, and code-readability/maintenance burden to hackarounds.

Comment 14

[Daniel Holbert [:dholbert]]

> Adding short-term hackarounds for they may be a good strategy

(er, s/for they/for actually-known add-on bugs/)

Comment 15

[Daniel Holbert [:dholbert]]

To be completely clear: the hackaround codepath in Aurora/Beta is basically:
 (1) Perform a security check for the SVG <use> element.
 (2) If that security check failed, *mess with the things we're comparing*, in case they might've been created with the wrong default port, and then *perform the security check again*.

The Aurora/Beta hackaround is step (2) here.

We absolutely do *not* want to keep that step around indefinitely -- it undeniably has a performance cost (which might be small, but is nonzero), and it's iffy from a security perspective [we don't know of a way to exploit it, but someone might be able to figure out a way in the future, particularly if the code changes slightly].

In the unlikely scenario that we uncover other add-ons that are helped by that hackaround, we can look into real, better fixes in the platform and/or the add-on. Keeping the hackaround (as you seem to be advocating) is not a good solution; and there's also no sense worrying about problems that we don't currently know that we have. (unmaintained add-ons that are affected by similar bugs)

Comment 16

[sjw]

https://github.com/EFForg/https-everywhere/commit/297fac574fce6bc2cb9519d1e3b39513c1ea30ca