1378760 - video playback stops working on twit.tv

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: twit.tv video would not play back with new profile

The problem can be reproduced with the new profile.

Reproducible: always

Steps to reproduce:
1. Start Nightly with new profile
2. Open https://twit.tv/
3. Click a thumbnail
4. Click play button in video container

Actual results:
Video would not play.
"Error loading media: File could not be played" is shown


Expected Results:
Video should play back


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3794aebfdef8582e9b2bb17d8e89df7e205250ca&tochange=6d909485099a6c99427c1ea944f820e20af96ee5

Regressed by: 6d909485099a	Honza Bambas — Bug 1319111 - Expose 'result principal URI' on LoadInfo as a source for NS_GetFinalChannelURI (removes some use of LOAD_REPLACE flag). r=bz, r=mikedeboer

Comment 1

[Alice0775 White]

Error in Web console:
Error playing media:  MediaError { code: 4, message: "" }  http://www.podtrac.com/pts/redirect.mp4/cdn.twit.tv/video/hn/hn0307/hn0307_h264m_864x480_500.mp4
cLal8L3uEeSYsRJtO5t17w.js:13:153

CAPTIONS([object Object])
cLal8L3uEeSYsRJtO5t17w.js:13:153

Comment 2

[Alice0775 White]

Attachment: about support

Comment 3

[Jean-Yves Avenard [:jya]]

I doubt the provided regression range is correct.

Comment 4

[Jean-Yves Avenard [:jya]]

plays fine on mac

Comment 5

[Alice0775 White]

I can also reproduce the problem on Nightly56.0a1 Ubuntu16.04 64bit.

https://hg.mozilla.org/mozilla-central/rev/11755fd63168581e194258d04bb6a7337779ec78
Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0 ID:20170705170343

Comment 6

[Honza Bambas (:mayhemer)]

Reproducible for me with current nightly locally.

The URL referenced by the player redirects.  The redirect target channel is canceled with 805e0006 - NS_ERROR_CONTENT_BLOCKED.  The error comes from a call chain of nsHttpChannel::TryHSTSPriming -> HSTSPrimingListener::StartHSTSPriming -> primingChannel->AsyncOpen2(listener) -> DoContentSecurityChecks -> NS_CheckContentLoadPolicy -> shouldLoad == -1 => return the error;

I'll try with a code before and after bug 1319111 if that has an effect.

Comment 7

[Honza Bambas (:mayhemer)]

Confirming this is a regression from bug 1319111.  Going to investigate.

Comment 8

[Honza Bambas (:mayhemer)]

LoadInfo::CloneForNewRequest must clear the result principal URI.  That's not a small change but makes sense.  I confirmed locally that fixed the problem.  Pushing to try...



I'm still a bit confused, tho.  The URI to the video resource is a redirect (http://www.podtrac.com/pts/redirect.mp4/... -> http://cdn.twit.tv/...)   

W/ bug 1319111 we *don't* try to do priming for the podtrac (source) channel, but we *do* try for the redirect target channel.  And because there is the incorrect final channel URI - load info's result principal URI points to http://cdn.twit.tv/..., the priming channel asyncOpen fails because [1] doesn't pass (we check against an insecure URI!) and later there is no loading node/context at [2], so we bail.

W/o bug 1319111 (or with the fix suggested at the top) we *do* try priming for the podtrac (source) channel with a full https upgraded URI (leading to a 302 to an https://cdn.twit.tv/... uri, w/o any HSTS headers, hence we continue with the plain redirect) and we *don't* try priming for the redirect target channel (http://cdn.twit.tv/... URI), which is kinda weird.

Kate, what is exactly expected to happen here?  How does priming deal with redirects?


[1] https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/dom/security/nsMixedContentBlocker.cpp#653
[2] https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/dom/security/nsMixedContentBlocker.cpp#752

Comment 9

[Honza Bambas (:mayhemer)]

(I'm also a bit surprised we don't have a test for this...)

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1 (drop rpURI in LoadInfo::CloneForNewRequest)

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

LoadInfo::CloneForNewRequest is used for new requests, hence the result principal URI (rpURI) should be initially null (it's generally set by the protocol handler creating the channel or we do some play with rpURI on target channels about to be redirected to)

bz, doesn't accept r?, Christoph, can you take a look?  The background is in bug 1319111 comment 17 and bug 1319111 comment 169.  Thanks.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> Created attachment 8884020 [details] [diff] [review]
> v1 (drop rpURI in LoadInfo::CloneForNewRequest)

I think https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/netwerk/protocol/res/ExtensionProtocolHandler.cpp#491 will break with this patch.  We may need two methods: clone-for-new-req and clone-for-new-req-carry-rpURI.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 56, looks like a recent regression.

Comment 13

[Blake Wu [:bwu][:blakewu]]

I can see this problem on my Mac, OS 10.12.5, with the latest nightly, 56.0a1 (2017-07-06) (64-bit).

Comment 14

[anleck]

I think the problem started around 28 Jun 2017

Comment 15

[anleck]

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

Try build is working - twit videos playing
Tested win x64 Zip

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8884020 [details] [diff] [review]
v1 (drop rpURI in LoadInfo::CloneForNewRequest)

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

Sounds reasonable, thanks.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8884020 [details] [diff] [review]
v1 (drop rpURI in LoadInfo::CloneForNewRequest)

Kris, this is a sensitive change, could you please double check this is right?  Specifically concerning comment 11.  Before AND also after bug 1319111 the result principal URI (NS_GetFinalChannelURI result) was not affected by the code at [1], but now it will be.  It will default to the result channel's originalURI, what may not always be the right thing.  

I first need to understand why the substitution there creates a new loadinfo for the new request at the first place and then whether the result principal URI should be preserved.

Thanks.

[1] https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/netwerk/protocol/res/ExtensionProtocolHandler.cpp#491

Comment 18

[Kate McKinley [:kmckinley, :Kate]]

From a fresh install, it looks like we do try priming on www.podtrac.com, which fails, so then cache SecurityPropertyNegative for www.podtrac.com. After we get the redirect, we do prime the redirect URI on cdn.twit.tv, and that also comes back SecurityPropertyNegative since there is no HSTS header on that host. So it seems to be working as intended.

Comment 19

[Honza Bambas (:mayhemer)]

Thanks Kate.  Based on that it seems it works as expected.

kmag is unresponsive.  Going to land, try looks good.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff324c369c9c
LoadInfo::CloneForNewRequest must not carry result principal URI, r=bz

Comment 21

[Kris Maglione [:kmag]]

(In reply to Honza Bambas (:mayhemer) from comment #19)
> kmag is unresponsive.  Going to land, try looks good.

Sorry, I've been putting out fires most of the week. This looks fine to me.

(In reply to Honza Bambas (:mayhemer) from comment #17)
> Kris, this is a sensitive change, could you please double check this is
> right?  Specifically concerning comment 11.  Before AND also after bug
> 1319111 the result principal URI (NS_GetFinalChannelURI result) was not
> affected by the code at [1], but now it will be.  It will default to the
> result channel's originalURI, what may not always be the right thing.  
> 
> I first need to understand why the substitution there creates a new loadinfo
> for the new request at the first place and then whether the result principal
> URI should be preserved.

We clone the load info at this point because we end up creating two channels in order to handle the request: the original channel that reads the underlying jar: or file: resource, and wrapper channel that filters the underlying channel's data through a stream converter.

The wrapper channel is the one that matters for the result principal URI, and it's also the channel that inherits the original load info, so this change shouldn't have any effect on our behavior in this situation.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 23

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx 55.0a1 (2017-07-06), on Windows 10 x64.
I can confirm the issue is fixed, I verified using Fx 57.0a1 (20170821100350) and Fx 56.0b4, on Windows 10 x64, macOS X 10.13 and Ubuntu 14.04 LTS.

Cheers!