Closed Bug 1249452 Opened 7 years ago Closed 7 years ago

Come up with a targeted backportable workaround for bug 1247733 in SVG <use> code (to fix missing buttons in YouTube <embed>s, now that they've upgraded to use HSTS)

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- unaffected

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files, 1 obsolete file)

We need to address bug 1247733 on branches (to get buttons showing up in embedded http YouTube videos again). But that bug's patches (and the patches for the helper-bugs that it requires) are probably too risky/sizeable to backport, at least for beta.

I think we can come up with a more targeted fix which would be more backport-friendly, as described in bug 1247733 comment 48.
Bug 1247733's mochitest (its "part 3" patch) should serve as an appropriate automated test for this bug, too.

I'm mostly-offline for the next few days, but I may be able to come up with a patch here this evening.
bz, what do you think about this as a targeted band-aid for bug 1247733, for Aurora & Beta?

Basically, this is just directly targeting the comparison that was failing in bug 1247733 -- when we have a URI comparison failure there, we try again with the (expected) port 443 hardcoded in to our URI (if our URI is already using its default port). This should only make a difference if we've got the wrong mDefaultPort.

(I'm uncomfortable backporting the actual full fix for bug 1247733, because it's an API change and a refactoring of internal networking code, and it requires us to fix a handful of unrelated callsites to avoid breaking them.)

This targeted fix adds a bit of overhead, but we only experience this overhead when we have a nsReferencedElement uri-comparison failure, which should not be a usual case.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8721170 - Flags: review?(bzbarsky)
Here's the mochitest from bug 1247733, which will do just fine as a mochitest for this bug as well (on Aurora/Beta).
Minor tweaks to patch:
 - Fixed a bug caught during local testing; I was using the wrong URI during a check at the end of my helper-function, and I had an equality comparison flipped.
 - Also improved some naming & added constants for port numbers of interest, while I was at it.
Attachment #8721170 - Attachment is obsolete: true
Attachment #8721170 - Flags: review?(bzbarsky)
Attachment #8721192 - Flags: review?(bzbarsky)
Comment on attachment 8721192 [details] [diff] [review]
part 1, v2 if <use> URI comparison fails, and we're HTTPS & using default port, try again with hardcoded port 443

r=me.   This is sadmaking, but I agree it's less potentially-regressing than the real fix...
Attachment #8721192 - Flags: review?(bzbarsky) → review+
Comment on attachment 8721192 [details] [diff] [review]
part 1, v2 if <use> URI comparison fails, and we're HTTPS & using default port, try again with hardcoded port 443

Approval Request Comment
[Feature/regressing bug #]: bug 495115 (strict transport security). That feature seems to have always had this issue (per bug 1247733 comment 22), but it hasn't caused problems until recently, because it only shows up on pages that have SVG <use>, and only when you exercise the HTTP->HTTPS upgrading (which happens with youtube embedded videos). 

[User impact if declined]: On some embedded youtube videos, a bunch of buttons (including play/pause button) won't show up. See attachment 8714110 [details] for a screenshot.

[Describe test coverage new/current, TreeHerder]:
 - Mochitest included to verify that this is actually fixed.
 - Tested locally as well, with a youtube video that's known to trigger the bug
 - Did a Try run, which seems to only have unrelated intermittent failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20a5d2d2e5c6&selectedJob=16935489 

[Risks and why]: Low-risk -- it's (intentionally) a very targeted band-aid.  (It gives us a second chance at matching SVG <use> URIs, if our first attempt fails.) 

[String/UUID change made/needed]: None.
Attachment #8721192 - Flags: approval-mozilla-beta?
Attachment #8721192 - Flags: approval-mozilla-aurora?
Summary: Come up with a targeted backportable workaround for bug 1247733 in SVG <use> code → Come up with a targeted backportable workaround for bug 1247733 in SVG <use> code (to fix missing buttons in YouTube <embed>s, now that they've upgraded to use HSTS)
Comment on attachment 8721192 [details] [diff] [review]
part 1, v2 if <use> URI comparison fails, and we're HTTPS & using default port, try again with hardcoded port 443

Impact on an important website (Youtube, see bug 1244495), taking it.
Attachment #8721192 - Flags: approval-mozilla-beta?
Attachment #8721192 - Flags: approval-mozilla-beta+
Attachment #8721192 - Flags: approval-mozilla-aurora?
Attachment #8721192 - Flags: approval-mozilla-aurora+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.