1436271 - Replace calls to String.getBytes(String) with String.getBytes(Charset)

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[Andrew Gaul]

String.getBytes(String) throws UnsupportedEncodingException even though it cannot fail with well-known charsets like UTF-8.  Instead call String.getBytes(Charset) using StringUtils.UTF_8 constant.  This will remove some cannot-happen exception handling.

Comment 1

[Andrew Gaul]

Attachment: Replace calls to String.getBytes(String) with String.getBytes(Charset)

Also replace calls to String(byte[], String) with String(byte[], Charset).  This
removes some cannot-happen exception handling.

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8948894 [details] [diff] [review]
Replace calls to String.getBytes(String) with String.getBytes(Charset)

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

At first I thought this wouldn't build --without-gradle, but now I think it will.  A try build will tell -- the Bng task is the interesting one.

I'm assuming this was machine generated using Android Studio's rewriting capability, so I only checked a couple.  Rock on!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1889332abc68
Replace calls to String.getBytes(String) with String.getBytes(Charset) r=nalexander

Comment 4

[Andreea Pavel [:apavel]]

Backed out for android test failure

push that caused the failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1889332abc68656da75d1c9f0253e27b95f140ad

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=160901071&repo=mozilla-inbound&lineNumber=3161

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef299d3c4d519a6348e186f92127d70917df113

Comment 5

[Andrew Gaul]

Apparently mach build does not build all sources and I must also run mach android test.

Comment 6

[Andrew Gaul]

Attachment: Replace calls to String.getBytes(String) with String.getBytes(Charset)

Also replace calls to String(byte[], String) with String(byte[], Charset).  This
removes some cannot-happen exception handling.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8949960 [details] [diff] [review]
Replace calls to String.getBytes(String) with String.getBytes(Charset)

Hi Andrew -- sorry for the long delay before review.  I've been busy and haven't been able to look at these fixes.  I'm going to redirect to mcomella, since I'm PTO for a few days and these should be addressed by someone with Fennec-related  bandwidth.  Sorry again.

Comment 8

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

Comment on attachment 8949960 [details] [diff] [review]
Replace calls to String.getBytes(String) with String.getBytes(Charset)

Sorry I haven't been able to get to this. Fennec isn't my primary responsibility and I'm assigned for only critical fennec reviews so this keeps getting pushed back. :(

Adding nalexander back just in case he gets to it first.

Comment 9

[:sdaswani]

Nick, Snorp - I can't review this, and Mike is only doing review of items flagged for work by Product. Please flag me for review instead of Mike if you need a front end review. 

Andreas, Barbara - please let us know if Mike should review this or not. I don't think there is a significant product impact.

Comment 10

[:sdaswani]

Vlad, can one of you take a look at this and give us your opinion on this code?

Comment 11

[Vlad Baicu]

Upon reviewing, I'd like to bring up some points for discussion:

- I've found two StringUtils classes with the same constant defined "UTF-8" - org.mozilla.mozstumbler.service.utils & org.mozilla.gecko.util

> diff --git a/mobile/android/app/src/test/java/org/mozilla/gecko/push/TestPushState.java b/mobile/android/app/src/test/java/org/mozilla/gecko/push/TestPushState.java
> --- a/mobile/android/app/src/test/java/org/mozilla/gecko/push/TestPushState.java
> +++ b/mobile/android/app/src/test/java/org/mozilla/gecko/push/TestPushState.java
> @@ -2,16 +2,17 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  package org.mozilla.gecko.push;
>  
>  import org.junit.Assert;
>  import org.junit.Test;
>  import org.junit.runner.RunWith;
>  import org.mozilla.gecko.background.testhelpers.TestRunner;
> +import org.mozilla.gecko.util.StringUtils;
>  import org.robolectric.RuntimeEnvironment;
>  
>  import java.io.File;
>  import java.io.FileOutputStream;
>  
>  @RunWith(TestRunner.class)
>  public class TestPushState {
>      @Test
> @@ -52,17 +53,17 @@ public class TestPushState {
>      @Test
>      public void testCorruptedJSON() throws Exception {
>          // Write some malformed JSON.
>          // TODO: use mcomella's helpers!
>          final File file = new File(RuntimeEnvironment.application.getApplicationInfo().dataDir, "testCorruptedJSON.json");
>          FileOutputStream fos = null;
>          try {
>              fos = new FileOutputStream(file);
> -            fos.write("}".getBytes("UTF-8"));
> +            fos.write("}".getBytes(StringUtils.UTF_8));
>          } finally {
>              if (fos != null) {
>                  fos.close();
>              }
>          }
 
- Wouldn't a better approach be to use java.nio's StandardCharsets.UTF_8 included in the JDK, instead of a constant defined in an util class ? This would remove a dependency among modules and should be more explicit. In the above example there is a dependency to the constant from another module.

> diff --git a/mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccount20CreateDelegate.java b/mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccount20CreateDelegate.java
> --- a/mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccount20CreateDelegate.java
> +++ b/mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccount20CreateDelegate.java
> 
>    public FxAccount20CreateDelegate(byte[] emailUTF8, byte[] quickStretchedPW, boolean preVerified) throws UnsupportedEncodingException, GeneralSecurityException {
>      this.emailUTF8 = emailUTF8;
>      this.authPW = FxAccountUtils.generateAuthPW(quickStretchedPW);
>      this.preVerified = preVerified;
>    }

- At line 37 method exception throw seems redundant

- The patch's scope is to clear hardcoded usages of utf-8 encodings from String to Charset in order to clean up exceptions that cannot happen. After searching through the latest revision of the project I have found many other usages where these still occur (FxAccountClient20.java:139 & 141; BookmarkRecord.java:434; IntentHelper.java:512 and the list goes on)

Comment 12

[:sdaswani]

NI'ing Andrew for review feedback.

Comment 13

[Nick Alexander :nalexander [he/him]]

I feel badly because Andrew's patches have largely been ignored or bounced around between reviewers, and that has probably been frustrating.  Andrew, I'm sorry: Fennec is not being maintained well at this time, and cleanup work of the type you are doing has slipped through the cracks.  In most cases, I think this is the correct approach, since we are focusing on other things.  In light of this, I'm going to clear my review.  Apologies again.

Comment 14

[:sdaswani]

(In reply to Nick Alexander :nalexander from comment #13)
> I feel badly because Andrew's patches have largely been ignored or bounced
> around between reviewers, and that has probably been frustrating.  Andrew,
> I'm sorry: Fennec is not being maintained well at this time, and cleanup
> work of the type you are doing has slipped through the cracks.  In most
> cases, I think this is the correct approach, since we are focusing on other
> things.  In light of this, I'm going to clear my review.  Apologies again.

Nick - Vlad did give some feedback but we haven’t heard back from Andrew.

Comment 15

[Andrew Gaul]

Attachment: Replace calls to String.getBytes(String) with String.getBytes(Charset)

Also replace calls to String(byte[], String) with String(byte[], Charset).  This
removes some cannot-happen exception handling.

Comment 16

[Andrew Gaul]

> - Wouldn't a better approach be to use java.nio's StandardCharsets.UTF_8 included in the JDK, instead of a constant defined in an util class ? This would remove a dependency among modules and should be more explicit. In the above example there is a dependency to the constant from another module.

I believe that StandardCharsets requires Android API 19 while mobile/android/confvars.sh defines MOZ_ANDROID_MIN_SDK_VERSION=16.  I may misunderstand the Android support libraries though.

> - At line 37 method exception throw seems redundant

Fixed.

> - The patch's scope is to clear hardcoded usages of utf-8 encodings from String to Charset in order to clean up exceptions that cannot happen. After searching through the latest revision of the project I have found many other usages where these still occur (FxAccountClient20.java:139 & 141; BookmarkRecord.java:434; IntentHelper.java:512 and the list goes on)

I replaced all occurrences at the time of submission but new ones were added after.

Comment 17

[Andrew Gaul]

(In reply to Nick Alexander :nalexander from comment #13)
> I feel badly because Andrew's patches have largely been ignored or bounced
> around between reviewers, and that has probably been frustrating.  Andrew,
> I'm sorry: Fennec is not being maintained well at this time, and cleanup
> work of the type you are doing has slipped through the cracks.  In most
> cases, I think this is the correct approach, since we are focusing on other
> things.  In light of this, I'm going to clear my review.  Apologies again.

This is the wrong venue for this commentary, but learning the Bugzilla workflow tries my patience more than long review times.  It would be great to offer a GitHub workflow for casual contributors.

Comment 18

[:sdaswani]

(In reply to Andrew Gaul from comment #17)
> (In reply to Nick Alexander :nalexander from comment #13)
> > I feel badly because Andrew's patches have largely been ignored or bounced
> > around between reviewers, and that has probably been frustrating.  Andrew,
> > I'm sorry: Fennec is not being maintained well at this time, and cleanup
> > work of the type you are doing has slipped through the cracks.  In most
> > cases, I think this is the correct approach, since we are focusing on other
> > things.  In light of this, I'm going to clear my review.  Apologies again.
> 
> This is the wrong venue for this commentary, but learning the Bugzilla
> workflow tries my patience more than long review times.  It would be great
> to offer a GitHub workflow for casual contributors.

That's useful feedback, thanks Andrew. We do have some of our newer projects on GitHub.

Comment 19

[Vlad Baicu]

Looks good Andrew, however I was talking about StandardCharsets.UTF-8 from the java.nio package which is included in JDK since 1.7 which we are currently running, it is not related to Android. r+

https://docs.oracle.com/javase/7/docs/api/java/nio/charset/StandardCharsets.html

Comment 20

[Andrew Gaul]

(In reply to Vlad Baicu from comment #19)
> Looks good Andrew, however I was talking about StandardCharsets.UTF-8 from
> the java.nio package which is included in JDK since 1.7 which we are
> currently running, it is not related to Android. r+
> 
> https://docs.oracle.com/javase/7/docs/api/java/nio/charset/StandardCharsets.
> html

I do not understand your JDK 1.7 reference since Android has its own Java run-time.
Android documents this API as added in API version 19:

https://developer.android.com/reference/java/nio/charset/StandardCharsets.html

Comment 21

[Vlad Baicu]

My bad, I was thrown off by the javadocs, indeed using StandardCharsets crashes below API 19. Patch looks good to me

Comment 22

[:sdaswani]

Nick, Vlad has approved this patch, but he can't r+ items - any suggestions on how to proceed? Can you do the r+?

Comment 23

[Nick Alexander :nalexander [he/him]]

(In reply to :sdaswani from comment #22)
> Nick, Vlad has approved this patch, but he can't r+ items - any suggestions
> on how to proceed? Can you do the r+?

Yes.

Comment 24

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #23)
> (In reply to :sdaswani from comment #22)
> > Nick, Vlad has approved this patch, but he can't r+ items - any suggestions
> > on how to proceed? Can you do the r+?
> 
> Yes.

Wait -- Vlad has marked the r+.  This is checkin-needed, although I'd feel better with a green try build, which I will push myself tomorrow.  NI to me to do that.

Comment 25

[:sdaswani]

Please Nick, thanks!

Comment 26

[Nick Alexander :nalexander [he/him]]

This builds locally for me.  Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4416908245b9cb975f6131e44b0560c61ea0027

I'll keep the NI so that I set checkin-needed if it's green.

Comment 27

[Nick Alexander :nalexander [he/him]]

Andrew: this is busted at build time in some Sync tests: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=176460359.  Can you investigate?

Comment 28

[Andrew Gaul]

Attachment: Replace calls to String.getBytes(String) with String.getBytes(Charset)

Also replace calls to String(byte[], String) with String(byte[], Charset).  This
removes some cannot-happen exception handling.

Comment 29

[Andrew Gaul]

I addressed the compile error in the tests and verified with ./mach gradle app:test .

Comment 30

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8972498 [details] [diff] [review]
Replace calls to String.getBytes(String) with String.getBytes(Charset)

This is working for me locally.  THanks!

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45642b2381b5
Replace calls to String.getBytes(String) with String.getBytes(Charset). r=nalexander

Comment 32

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/45642b2381b5