SUTAgent is using Apache HttpClient library that has been removed in Android API Level 23

RESOLVED FIXED in Firefox 44

Status

()

Firefox for Android
Testing
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 44
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
SUTAgent uses org.apache.http.*. This package has been removed in Android API level 23 (M / 6.0).

Build errors:
https://pastebin.mozilla.org/8847539

Fennec switched from org.apache to ch.boye in bug 1169421.
(Assignee)

Updated

2 years ago
See Also: → bug 1169421
(Assignee)

Comment 1

2 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #0)
> Fennec switched from org.apache to ch.boye in bug 1169421.

Another option would be to switch to URLConnection to avoid adding a third-party dependency. I guess that depends on whether SUTAgent is just doing simple requests or uses some fancy HTTP features.

From the build log it looks like SUTAgent calls Notification.setLatestEventInfo() that is deprecated now. In Fennec we switched to using NotificationCompat.Builder (Bug 1195692).
(Assignee)

Comment 2

2 years ago
gbrown, jmaher: What do you think would be the best way to go forward here?
Flags: needinfo?(jmaher)
Flags: needinfo?(gbrown)
I suspect we might be able to get rid of the funciton GetInternetData:
https://dxr.mozilla.org/mozilla-central/search?q=GetInternetData&case=true

To be honest I am not sure if there is anyway to use the GetInternetData in other non obvious ways.  Most likely it isn't used at all and given the fact that nothing calls it there is high confidence we don't need it anytmore.

If you remove that entire function and the related imports, will the build work?
Flags: needinfo?(jmaher)
(Assignee)

Comment 4

2 years ago
Thanks! I'm going to try that (and refactor the Notification.setLatestEventInfo usage).
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
Created attachment 8668467 [details] [diff] [review]
SUTAgent: Remove unused method GetInternetData()
Attachment #8668467 - Flags: review?(jmaher)
(Assignee)

Comment 6

2 years ago
Created attachment 8668468 [details] [diff] [review]
SUTAgent: Replace deprecated Notification APIs with Notification.Builder
Attachment #8668468 - Flags: review?(michael.l.comella)
(Assignee)

Comment 7

2 years ago
Created attachment 8668469 [details] [diff] [review]
SUTAgent: Inline isIPv4Address()
Attachment #8668469 - Flags: review?(jmaher)
(Assignee)

Updated

2 years ago
Attachment #8668467 - Attachment description: Remove unused method GetInternetData() → SUTAgent: Remove unused method GetInternetData()
Comment on attachment 8668467 [details] [diff] [review]
SUTAgent: Remove unused method GetInternetData()

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

great!
Attachment #8668467 - Flags: review?(jmaher) → review+
Comment on attachment 8668469 [details] [diff] [review]
SUTAgent: Inline isIPv4Address()

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

other than my confusion on the regex, this is good

::: build/mobile/sutagent/android/SUTAgentAndroid.java
@@ +55,5 @@
>  public class SUTAgentAndroid extends Activity
>      {
>      final Handler mHandler = new Handler();
>  
> +    private static final Pattern IPV4_PATTERN = Pattern.compile("^(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)(\\.(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)){3}$");

I don't understand this pattern?  how does loopback work or other addresses work?  is there a reference for this?
Attachment #8668469 - Flags: review?(jmaher) → review-
(Assignee)

Comment 10

2 years ago
(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 8668469 [details] [diff] [review]
> SUTAgent: Inline isIPv4Address()
> 
> Review of attachment 8668469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> other than my confusion on the regex, this is good
> 
> ::: build/mobile/sutagent/android/SUTAgentAndroid.java
> @@ +55,5 @@
> >  public class SUTAgentAndroid extends Activity
> >      {
> >      final Handler mHandler = new Handler();
> >  
> > +    private static final Pattern IPV4_PATTERN = Pattern.compile("^(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)(\\.(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)){3}$");
> 
> I don't understand this pattern?  how does loopback work or other addresses
> work?  is there a reference for this?

I basically just inlined the code from InetAddressUtils:
http://androidxref.com/source/xref/external/apache-http/src/org/apache/http/conn/util/InetAddressUtils.java

I just saw that the latest HttpClient release has a refactored, more readabable, version:

> private static final String IPV4_BASIC_PATTERN_STRING =
>             "(([1-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){1}" + // initial first field, 1-255
>             "(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){2}" + // following 2 fields, 0-255 followed by .
>              "([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])"; // final field, 0-255

http://grepcode.com/file/repo1.maven.org/maven2/org.apache.httpcomponents/httpclient/4.5/org/apache/http/conn/util/InetAddressUtils.java#45
Comment on attachment 8668469 [details] [diff] [review]
SUTAgent: Inline isIPv4Address()

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

given that this is the same regex as used in the code we were importing, lets r+.  Please reference where you did get it from in a comment.
Attachment #8668469 - Flags: review- → review+
(Assignee)

Comment 12

2 years ago
Together with other API 23 build patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b51c8b5499
(Assignee)

Comment 13

2 years ago
Comment on attachment 8668468 [details] [diff] [review]
SUTAgent: Replace deprecated Notification APIs with Notification.Builder

Mike has so much in his review queue, maybe you have less? :)
Attachment #8668468 - Flags: review?(mhaigh)
(Assignee)

Comment 14

2 years ago
leave-open: I'm going to land the two reviewed patches.
Keywords: leave-open

Comment 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/488526a93c27
https://hg.mozilla.org/integration/fx-team/rev/43e9af141078
https://hg.mozilla.org/mozilla-central/rev/488526a93c27
https://hg.mozilla.org/mozilla-central/rev/43e9af141078
Comment on attachment 8668468 [details] [diff] [review]
SUTAgent: Replace deprecated Notification APIs with Notification.Builder

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

Seems reasonable to me.
Attachment #8668468 - Flags: review?(michael.l.comella)
Attachment #8668468 - Flags: review?(mhaigh)
Attachment #8668468 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 18

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/df5a6ba88fed
Sorry to be so late to the party! Patches look good to me.
Flags: needinfo?(gbrown)
https://hg.mozilla.org/mozilla-central/rev/df5a6ba88fed
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.