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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(10 attachments)

59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
bryce
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: -- → P2

Comment 3

Last year
mozreview-review
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
Attachment #8956984 - Flags: review?(jyavenard) → review+

Comment 4

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

https://reviewboard.mozilla.org/r/225950/#review232538
Attachment #8956985 - Flags: review?(jyavenard) → review+

Comment 5

Last year
Pushed by cpearce@mozilla.com:
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
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.
Flags: needinfo?(cpearce)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

Last year
Pushed by cpearce@mozilla.com:
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

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

https://reviewboard.mozilla.org/r/234808/#review240488
Attachment #8966076 - Flags: review?(jyavenard) → review+

Comment 25

Last year
mozreview-review
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....
Attachment #8966077 - Flags: review?(jyavenard) → review+

Comment 26

Last year
mozreview-review
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

Last year
mozreview-review
Comment on attachment 8966078 [details]
Bug 1443942 - Rewrite test_mixed_principals.

https://reviewboard.mozilla.org/r/234812/#review240494
Attachment #8966078 - Flags: review?(jyavenard) → review+

Comment 28

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

https://reviewboard.mozilla.org/r/234814/#review240496
Attachment #8966079 - Flags: review?(jyavenard) → review+

Comment 29

Last year
mozreview-review
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?
Attachment #8966080 - Flags: review?(jyavenard) → review+

Comment 30

Last year
mozreview-review
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
Attachment #8966081 - Flags: review?(jyavenard) → review+

Comment 31

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

https://reviewboard.mozilla.org/r/234822/#review240506
Attachment #8966083 - Flags: review?(jyavenard) → review+

Comment 32

Last year
mozreview-review
Comment on attachment 8966082 [details]
Bug 1443942 - Rewrite test_mediarecorder_principals.

https://reviewboard.mozilla.org/r/234820/#review240554
Attachment #8966082 - Flags: review?(bvandyk) → review+
(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.
(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 36

Last year
Pushed by cpearce@mozilla.com:
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
Also:

Pushed by cpearce@mozilla.com:
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...)
Flags: needinfo?(cpearce)
Depends on: 1441153
No longer depends on: 1441153
Depends on: 1477649
This breaks facebook videos.
"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

10 months ago
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

10 months ago
*then problems arise
You need to log in before you can comment on or make changes to this bug.