HLS Player doesn't use the centralized Proxy Selector

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
Last year
2 months ago

People

(Reporter: igt0, Assigned: igt0)

Tracking

(Blocks 1 bug)

Trunk
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [tor-mobile])

Attachments

(1 attachment, 1 obsolete attachment)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180326160923

Steps to reproduce:

DefaultHttpDataSource, that it is used by the GeckoHlsPlayer, uses the default URL::openConnection (without Proxy) method provided by the Android SDK instead of the ProxySelector.

Following the same idea behing the issue https://bugzilla.mozilla.org/show_bug.cgi?id=1357997, all http connections in Fennec goes through a centralized proxy.
Flags: needinfo?(michael.l.comella)
Attachment #8973516 - Flags: review?(michael.l.comella)
Attachment #8973516 - Flags: review?(bugzilla)
Whiteboard: [tor-mobile]
Blocks: 1314793
Comment on attachment 8973516 [details] [diff] [review]
0001-Bug-1459420-HLS-Player-doesn-t-use-the-centralized-P.patch

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

This is too much code for me to adequately review this month. Over to you, Michael.
Attachment #8973516 - Flags: review?(bugzilla)
I'm only on fennec for critical reviews: Susheel, this patch is for the Tor project - can we find an alternative reviewer for this bug?
Flags: needinfo?(michael.l.comella) → needinfo?(sdaswani)
Over to David, not sure who can look at this.
Flags: needinfo?(sdaswani) → needinfo?(dbolter)
Wennie do you know who should be looking at Tor related patches?
Flags: needinfo?(dbolter) → needinfo?(wleung)
There is an alternative and simple solution. We can modify the /mobile/android/geckoview/src/thirdparty/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java to use the ProxySelector, the final patch would be around 10 lines of code. However I don't know how common is for Mozilla to change the third party frameworks.
> However I don't know how common is for Mozilla to change the third party frameworks.

We try to avoid it to make upgrading our libraries easier but if it's required or saves us from a lot of complexity, we do it. See [1] for some prior art.

When you make changes to these files, if practical please comment what you changed, like [2], and make a note that the file was modified, like [3]. Thanks!

> the final patch would be around 10 lines of code.

Provided those ten lines are not complex, this is something I should be able to review! Thanks Igor!

[1]: https://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid%2Fthirdparty+mozilla&path=

[2]: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/mobile/android/thirdparty/com/adjust/sdk/ActivityHandler.java#718
[3]: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/mobile/android/thirdparty/org/mozilla/apache/commons/codec/BinaryDecoder.java#1
Assignee: nobody → igt0
Comment on attachment 8973516 [details] [diff] [review]
0001-Bug-1459420-HLS-Player-doesn-t-use-the-centralized-P.patch

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

Realistically don't have time to review this and there's an open (albeit stalled) NI for a new reviewer: I'm going to clear my r?. Sorry Igor.

If the 10 line patch materializes, please let me know. If not, maybe ping :snorp – his team might be okay reviewing something like this.
Attachment #8973516 - Flags: review?(michael.l.comella)
As promised, a simplified version of the previous patch. It modifies the exoplayer in the third_party folder.
Attachment #8973516 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
Attachment #8983032 - Flags: review?(michael.l.comella)
Great, thanks Igor! I'll take a look at this tomorrow.
Flags: needinfo?(michael.l.comella)
Comment on attachment 8983032 [details] [diff] [review]
0001-Bug-1459420-HLS-Player-doesn-t-use-the-centralized-P.patch

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

lgtm - thanks Igor!

::: mobile/android/geckoview/src/thirdparty/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java
@@ +399,5 @@
> +     * Tor Project modified the way the connection object was created. For the sake of
> +     * simplicity, instead of duplicating the whole file we changed the connection object
> +     * to use the ProxySelector.
> +     */
> +    HttpURLConnection connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(url.toURI());

I'm concerned that url.toURI() could fail on the URLs passed in. However, my understanding is that 1) URL is validated and 2) URI is a superset of URL so this should never throw.
Attachment #8983032 - Flags: review?(michael.l.comella) → review+
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d1dcb9093cb419fbfc3751a00293da4cf60fa3f

Igor, please add the checkin-needed keyword when this goes green. Thanks!
Flags: needinfo?(igt0)
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b75e6bb295a9
HLS Player doesn't use the centralized Proxy Selector r=mcomella
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b75e6bb295a9
Status: UNCONFIRMED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Flags: needinfo?(igt0)
Flags: needinfo?(wleung)
You need to log in before you can comment on or make changes to this bug.