Closed Bug 1443942 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(10 files)

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
Priority: -- → P2
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 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+
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)
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 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 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 on attachment 8966077 [details]
Bug 1443942 - Make redirect SJS' serve with headers to prevent Necko caching.

https://reviewboard.mozilla.org/r/234810/#review240492
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 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 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 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 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 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!
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.
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).
*then problems arise
Regressions: 1566572
You need to log in before you can comment on or make changes to this bug.