1459420 - HLS Player doesn't use the centralized Proxy Selector

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Igor Oliveira]

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.

Comment 1

[Igor Oliveira]

Attachment: 0001-Bug-1459420-HLS-Player-doesn-t-use-the-centralized-P.patch

Comment 2

[Richard Newman [:rnewman]]

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.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 4

[:sdaswani]

Over to David, not sure who can look at this.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Wennie do you know who should be looking at Tor related patches?

Comment 6

[Igor Oliveira]

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.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> 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

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 9

[Igor Oliveira]

Attachment: 0001-Bug-1459420-HLS-Player-doesn-t-use-the-centralized-P.patch

As promised, a simplified version of the previous patch. It modifies the exoplayer in the third_party folder.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Great, thanks Igor! I'll take a look at this tomorrow.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d1dcb9093cb419fbfc3751a00293da4cf60fa3f

Igor, please add the checkin-needed keyword when this goes green. Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b75e6bb295a9
HLS Player doesn't use the centralized Proxy Selector r=mcomella

Comment 14

[Narcis Beleuzu [:NarcisB]]

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