1443942 - Block mid-flight non CORS same origin redirects during media load

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

There's no compelling use case for supporting mid-flight cross site redirects during the download of src=url video. Chrome stopped supporting this in M45 but in M47 they relaxed that to allow cross-site redirects to CORS same-origins.[1]

We may as well do the same.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=532569

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Block mid-flight redirects to cross origin destinations during media loads.

There's no compelling use case for mid-flight redirects, and Chrome
already blocks it, so there's little point in maintaining it.

Add a hidden pref to toggle blocking, so we can toggle it off during
testing to ensure that we're blocking a working mid-flight redirect.

Review commit: https://reviewboard.mozilla.org/r/225948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225948/

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Test for blocking midflight redirects in media elements.

Test that playback works if we don't block, doesn't if we do block, and does
if we do block and CORS is used.

Review commit: https://reviewboard.mozilla.org/r/225950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225950/

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8956984 [details]
Bug 1443942 - Block mid-flight redirects to cross origin destinations during media loads.

https://reviewboard.mozilla.org/r/225948/#review232536

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8956985 [details]
Bug 1443942 - Test for blocking midflight redirects in media elements.

https://reviewboard.mozilla.org/r/225950/#review232538

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2257546ec484
Block mid-flight redirects to cross origin destinations during media loads. r=jya
https://hg.mozilla.org/integration/autoland/rev/fa5bcc246d0f
Test for blocking midflight redirects in media elements. r=jya

Comment 6

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (bug 1443942) for bustage on /testing/xpcshell/selftest.py on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fa5bcc246d0f83b5260944740f8f27cf11f9ff4c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Log failure: https://treeherder.mozilla.org/logviewer.html#?job_id=167529938&repo=autoland&lineNumber=33431

Backout: https://hg.mozilla.org/integration/autoland/rev/f505a9fa69d180fd8562404c402330b12fa85d86

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pretty sure the culprit is here:
https://treeherder.mozilla.org/logviewer.html#?job_id=167529938&repo=autoland&lineNumber=33516

PID 32540 | Hit MOZ_CRASH(accessing non-early pref media.block-midflight-redirects before late prefs are set) at /builds/worker/workspace/build/src/modules/libpref/Preferences.cpp:932

I added a MediaPref, but didn't add it to the list of early prefs that get sent between processes.

The pref is read on the main thread, so it doesn't need to be a MediaPref.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8956984 [details]
Bug 1443942 - Block mid-flight redirects to cross origin destinations during media loads.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225948/diff/1-2/

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8956985 [details]
Bug 1443942 - Test for blocking midflight redirects in media elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225950/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/446bfe8412cb
Block mid-flight redirects to cross origin destinations during media loads. r=jya
https://hg.mozilla.org/integration/autoland/rev/e2f69088b1d7
Test for blocking midflight redirects in media elements. r=jya

Comment 11

[Eliza Balazs [:ebalazs_]]

Backed out 2 changesets (bug 1443942) for mda assertion failures in /build/build/src/dom/media/ChannelMediaResource.cpp

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e2f69088b1d7074deeda9acd0b912d520de6b4ff&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success

Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e2f69088b1d7074deeda9acd0b912d520de6b4ff&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=167576893

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ce0bde354931425a042449771d1d81f54fa316b9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8956984 [details]
Bug 1443942 - Block mid-flight redirects to cross origin destinations during media loads.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225948/diff/2-3/

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8956985 [details]
Bug 1443942 - Test for blocking midflight redirects in media elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225950/diff/2-3/

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Ensure redirect SJS' serve the correct content types.

dynamic_redirect.sjs and midflight-redirect.sjs are serving files
with "Content-Type: video/ogg", which is incorrect and could lead
to problems given that we're not always asking it to serve Ogg
files. So include the type be to served as a query parameter.

Review commit: https://reviewboard.mozilla.org/r/234808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234808/

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Make redirect SJS' serve with headers to prevent Necko caching.

Try to prevent Necko from caching the results of our SJS media responses, as
some of the test that use it rely on the server being hit and serving a
redirect. Sometimes the tests which rely on hitting a redirect in an SJS
where timing out without this, as Necko would cache the response and not
hit the server, and so not hit the redirect.

Also include a noise parameter to increase the likelihood that the URL is
unique, to further reduce the chance that Necko caches the result.

Review commit: https://reviewboard.mozilla.org/r/234810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234810/

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Rewrite test_mixed_principals.

The original test is failing, as it assumed we'd not error when
origins were mixed without CORS, and the original test was using
outdated practises, so rewrite it.

Review commit: https://reviewboard.mozilla.org/r/234812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234812/

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Switch over to midflight redirect for all redirect media tests.

We have two SJS files; midflight-redirect.sjs and dynamic_redirect.sjs,
which are very similar, but dynamic_redirect.sjs is buggy, so we should
just use midflight-redirect.sjs.

dynamic_redirect.sjs is buggy because it relies on the client doing multiple
HTTP requests to it in order to redirect, but we can't actually guarantee
this. Previously users of it would try things like setting a small MediaCache
size, or only using Ogg for which we expect a seek to the end to calculate
the duration, but I have observed the entire resource being downloaded in
one hit before the media element has finished loading metadata, meaning the
seek (in the Ogg case) can happen without another HTTP request. This is even
with a small MediaCache.

midfligh-redirect.sjs solves this problem by explicitly only returning a
partial response, so the client is forced to make another HTTP request,
which we will serve a redirect to.

Review commit: https://reviewboard.mozilla.org/r/234814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234814/

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Fix dom/media/test/midflight-redirect.sjs.

Problems here:
* The variable `to` is undefined for byte range requests to the end of
the resource, making the math fail. Firefox normally makes ranges requests like
this.
* The bytes.length/4 calculation may not be a whole number, so can
result in a byte range header part of the way between two bytes. We need to
round the length off.
* The contentLength calculation is off-by-one, so use the size of the
actual byte substring being returned, rather than trying to (re-)calculate the
length being returned.
* test_midflight_redirect_blocked needs the redirect to happen
before metadata has completed loading, but other tests require the redirect to
happen *after* metadata is loaded. So add a redirectAt query parameter for the
requester to control when to redirect.

Review commit: https://reviewboard.mozilla.org/r/234816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234816/

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Ensure MediaCacheStreams are initialized with the length of the resource, not the length of the byte range response.

I'm seeing intermittent failures of test_midflight_redirect_blocked. In this
test, our custom server responds to Firefox's 0- HTTP Byte Range request with a
[0,200] response. When Firefox requests 200-, the server responds with a cross
origin redirect, and then the remainder of the resource.

However sometimes while running test_midflight_redirect_blocked the MP4 demuxer
reads through all 200 bytes while trying to parse metadata before the redirect
has occurred and fed more data into the cache, and so the demuxer thinks it's
hit end of stream, and reports a failure. The demuxer thinks it's hit end of
stream, because we initialize the MediaCacheStream length in
ChannelMediaResource::Open() with the value of the Content-Length HTTP header.
But in an HTTP byte range response, the Content-Length header tells you the
length of the range returned, not the length of the entire resource. The length
of the resource is in the Content-Range header, we need to use that if
available.

So to fix this intermittent test failure, we need to also parse the
Content-Range header in ChannelMediaResource::Open(), and use the length from
that if available.

Review commit: https://reviewboard.mozilla.org/r/234818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234818/

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Rewrite test_mediarecorder_principals.

I changed this test earlier in this set of commits to use
midflight-redirect.sjs so that we get more reliable and predictable cross
origin redirects during the download. Unfortunately this test now times out on
Windows.

This test times out on Windows because midflight-redirect.sjs redirects at 1/4
through the resource, whereas this test expects to be able to play through to
1/5 through the resource, and on Windows that seems to be not reached during
playback. This is likely due to decode latency being higher on Windows.

On top of that, the test's first case can sometimes call MediaRecorder.start()
before the redirect has happened, and before the principal has changed, and so
start() doesn't throw a SecurityError as expected, and the test intermittently
fails.

Additionally, the test's code could be clearer if we used async/await.

So rewrite the test to use async/await, and take advantage of
midflight-redirect.sjs's redirect being more predictable than the old
dynamic_redirect.sjs. Basically, we can be careful to wait for either
"loadedmetadata" or "error" on the media element in order to be more confident
the redirect has or hasn't happened yet.

We still can't be 100% sure that the redirect won't have already happened by
the time our "loadedmetadata" handlers run. It's quite possible that the
download has reached 1/4 through the resource by the time the loadedmetadata
handler has run, so we need to handle the "error" and "loadedmetadata" events
racing.

Review commit: https://reviewboard.mozilla.org/r/234820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234820/

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1443942 - Move code to toggle high res timers into VideoSink.

We have code in the MDSM to toggle on high resolution timers on Windows when we
start/stop playing because the VideoSink relies on being awoken by timers to
update the set of current frames in the compositor's queue, and on Windows 7 we
end up dropping frames due to the timer lag without this.

We assert in the MDSM's destructor that we've turned off high res timers (as
they cause needless battery drain, so we only want them on when we need them),
and the new test_mediarecorder_principals is hitting that assert on Windows. I
think we're missing turning them off when we create a new VideoSink for
outputting to the MSG. That affects the value returned by
MediaDecoderStateMachine->mVideoSink->IsPlaying(), which is what we use to
decide whether we should enable high resolution timers. We track whether we've
enabled high res timers in MDSM::mHiResTimersRequested, and that gets out of
sync with IsPlaying() when we re-create the MediaSink.

Rather than trying to handle all the permutations of places where we need to
turn off high resolution timers in the MDSM, we're better to move the code to
toggle high res timers into the VideoSink, as that's actually where we need to
be sure that we have high resolution timers enabled anyway. It's the VideoSink
after all that is relying on timers for frame update, not the MDSM.

Also remove the media.hi-res-timers.enabled pref, as we haven't needed it.

Review commit: https://reviewboard.mozilla.org/r/234822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234822/

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8956984 [details]
Bug 1443942 - Block mid-flight redirects to cross origin destinations during media loads.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225948/diff/3-4/

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8956985 [details]
Bug 1443942 - Test for blocking midflight redirects in media elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225950/diff/3-4/

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8966076 [details]
Bug 1443942 - Ensure redirect SJS' serve the correct content types.

https://reviewboard.mozilla.org/r/234808/#review240488

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8966077 [details]
Bug 1443942 - Make redirect SJS' serve with headers to prevent Necko caching.

https://reviewboard.mozilla.org/r/234810/#review240490

::: commit-message-27dd6:10
(Diff revision 1)
> +redirect. Sometimes the tests which rely on hitting a redirect in an SJS
> +where timing out without this, as Necko would cache the response and not
> +hit the server, and so not hit the redirect.
> +
> +Also include a noise parameter to increase the likelihood that the URL is
> +unique, to further reduce the chance that Necko caches the result.

that seems unecessary....

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8966077 [details]
Bug 1443942 - Make redirect SJS' serve with headers to prevent Necko caching.

https://reviewboard.mozilla.org/r/234810/#review240492

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8966078 [details]
Bug 1443942 - Rewrite test_mixed_principals.

https://reviewboard.mozilla.org/r/234812/#review240494

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8966079 [details]
Bug 1443942 - Switch over to midflight redirect for all redirect media tests.

https://reviewboard.mozilla.org/r/234814/#review240496

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8966080 [details]
Bug 1443942 - Fix dom/media/test/midflight-redirect.sjs.

https://reviewboard.mozilla.org/r/234816/#review240500

::: commit-message-c42be:10
(Diff revision 1)
> +the resource, making the math fail. Firefox normally makes ranges requests like
> +this.
> +* The bytes.length/4 calculation may not be a whole number, so can
> +result in a byte range header part of the way between two bytes. We need to
> +round the length off.
> +* The contentLength calculation is off-by-one, so use the size of the

is this a bug in the contentLength value?

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8966081 [details]
Bug 1443942 - Ensure MediaCacheStreams are initialized with the length of the resource, not the length of the byte range response.

https://reviewboard.mozilla.org/r/234818/#review240504

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8966083 [details]
Bug 1443942 - Move code to toggle high res timers into VideoSink.

https://reviewboard.mozilla.org/r/234822/#review240506

Comment 32

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8966082 [details]
Bug 1443942 - Rewrite test_mediarecorder_principals.

https://reviewboard.mozilla.org/r/234820/#review240554

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #29)
> Comment on attachment 8966080 [details]
> Bug 1443942 - Fix dom/media/test/midflight-redirect.sjs.
> 
> https://reviewboard.mozilla.org/r/234816/#review240500
> 
> ::: commit-message-c42be:10
> (Diff revision 1)
> > +the resource, making the math fail. Firefox normally makes ranges requests like
> > +this.
> > +* The bytes.length/4 calculation may not be a whole number, so can
> > +result in a byte range header part of the way between two bytes. We need to
> > +round the length off.
> > +* The contentLength calculation is off-by-one, so use the size of the
> 
> is this a bug in the contentLength value?

Actually I'm wrong, contentLength is correctly calculated in this patch. I'd made it off-by-one with another change that I'd made but subsequently removed, so it's no longer wrong. I think it's clearer and safer to use byterange.length here rather than re-calculate the length, so I'll leave that change in, but I'll adjust the commit message to no longer say the contentLength was wrong.

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #25)
> Comment on attachment 8966077 [details]
> Bug 1443942 - Make redirect SJS' serve with headers to prevent Necko caching.
> 
> https://reviewboard.mozilla.org/r/234810/#review240490
> 
> ::: commit-message-27dd6:10
> (Diff revision 1)
> > +redirect. Sometimes the tests which rely on hitting a redirect in an SJS
> > +where timing out without this, as Necko would cache the response and not
> > +hit the server, and so not hit the redirect.
> > +
> > +Also include a noise parameter to increase the likelihood that the URL is
> > +unique, to further reduce the chance that Necko caches the result.
> 
> that seems unecessary....

Indeed, it is unnecessary now, and in the other tests too. Thanks!

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c358ea16eb
Block mid-flight redirects to cross origin destinations during media loads. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/038cca3ed2ca
Test for blocking midflight redirects in media elements. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee486ce2d660
Make redirect SJS' serve with headers to prevent Necko caching. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/482dbcc619ce
Rewrite test_mixed_principals. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/e02b7f9e296d
Switch over to midflight redirect for all redirect media tests. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/79bd527dd8c8
Fix dom/media/test/midflight-redirect.sjs. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/f23060397905
Ensure MediaCacheStreams are initialized with the length of the resource, not the length of the byte range response. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3f66ef0cc4
Rewrite test_mediarecorder_principals. r=bryce
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a39d6a9a949
Move code to toggle high res timers into VideoSink. r=jya

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Also:

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d478be2796
Ensure redirect SJS' serve the correct content types. r=jya

Accidentally I had bug 1443943 in this commits commit message (vi increments numbers under the cursor if you press 'a', not the first time I've run afoul of this...)

Comment 38

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d8c358ea16eb
https://hg.mozilla.org/mozilla-central/rev/038cca3ed2ca
https://hg.mozilla.org/mozilla-central/rev/ee486ce2d660
https://hg.mozilla.org/mozilla-central/rev/482dbcc619ce
https://hg.mozilla.org/mozilla-central/rev/e02b7f9e296d
https://hg.mozilla.org/mozilla-central/rev/79bd527dd8c8
https://hg.mozilla.org/mozilla-central/rev/f23060397905
https://hg.mozilla.org/mozilla-central/rev/8b3f66ef0cc4
https://hg.mozilla.org/mozilla-central/rev/2a39d6a9a949

Comment 39

[Jean-Yves Avenard [:jya]]

This breaks facebook videos.

Comment 40

[Jean-Yves Avenard [:jya]]

"There's no compelling use case for supporting mid-flight cross site redirects during the download of src=url video" well, actually, from the chrome bug report 
"Most media CDNs do dynamic load balancing by spewing users across many servers. This traffic spewing is usually intelligent, but almost always non-deterministic; the necessary state to map users consistently to the same node in a way that was also traffic-fair is too big."

so anything with load balancing will be broken the way it was implemented.

Comment 41

[Djfe]

An additional info:
Facebook videos play fine for me when logged in and surfing on desktop Facebook.

If I'm using mobile or desktop Firefox on the mobile website though, then there problems arive.

This issue mostly arises for mobile users, using Facebook in their browser (instead of the app).

Comment 42

[Djfe]

*then problems arise