Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8924b3cd authored by Tobias Thierer's avatar Tobias Thierer
Browse files

android.net.Uri: Move NOT_CACHED constant to holder class. 

Commit b639ce9e (May 2019) introduced a
reference from PathPart.<init> to Uri.NOT_CACHED. If this happened before
Uri.<clinit> had first run, this led to an inconsistently constructed
Uri.EMPTY since that instance's path is PathPart.EMPTY, which at that
point would still have been null.

This issue was found in an environment where Zygote initializations and
preloaded classes had not been run, and the unit test included in this
CL already passes before the CL. This suggests that Uri.<clinit> already
ran in the correct order in Android Q and that the bug was only
observable on other platforms that do not perform this initialization.
Nonetheless, to avoid a fragile dependence on a particular
initialization order, this CL fixes the issue such that calling
Uri.Builder.path() prior to Uri.<clinit> is now safe.

Specifically, this CL moves the NOT_CACHED instance into its own holder
class. This way, accessing NOT_CACHED no longer triggers Uri.<clinit>.
This also makes an earlier comment (from 2009) obsolete which warned
that NOT_CACHED must be initialized before Uri.EMPTY.

I've made the holder class and its field package private to avoid the
overhead of a synthetic accessor method.

Note: This CL was uploaded with --no-verify to avoid complaints about
pre-existing style issues (private non-static fields whose name does
not start with "m").

Bug: 159907422
Test: atest FrameworksCoreTests:android.net.UriTest
Test: Manually checked that a corresponding change fixed the issue
      in an environment not forked from a Zygote process where
      the issue was previously observable.

Change-Id: I067a6c108aa8befd056818a8231930693f55d8cf
parent aeca778b

core/java/android/net/Uri.java

+41 −30
Original line number Diff line number Diff line
@@ -116,16 +116,27 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
    private static final String LOG = Uri.class.getSimpleName();



    /**

     * NOTE: EMPTY accesses this field during its own initialization, so this

     * field *must* be initialized first, or else EMPTY will see a null value!

     *

     * Placeholder for strings which haven't been cached. This enables us

     * Holds a placeholder for strings which haven't been cached. This enables us

     * to cache null. We intentionally create a new String instance so we can

     * compare its identity and there is no chance we will confuse it with

     * user data.

     *

     * NOTE This value is held in its own Holder class is so that referring to

     * {@link NotCachedHolder#NOT_CACHED} does not trigger {@code Uri.<clinit>}.

     * For example, {@code PathPart.<init>} uses {@code NotCachedHolder.NOT_CACHED}

     * but must not trigger {@code Uri.<clinit>}: Otherwise, the initialization of

     * {@code Uri.EMPTY} would see a {@code null} value for {@code PathPart.EMPTY}!

     *

     * @hide

     */

    static class NotCachedHolder {

        private NotCachedHolder() {

            // prevent instantiation

        }

        @SuppressWarnings("RedundantStringConstructorCall")

    private static final String NOT_CACHED = new String("NOT CACHED");

        static final String NOT_CACHED = new String("NOT CACHED");

    }



    /**

     * The empty URI, equivalent to "".
@@ -554,11 +565,11 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
            return findSchemeSeparator() == NOT_FOUND;

        }



        private volatile String scheme = NOT_CACHED;

        private volatile String scheme = NotCachedHolder.NOT_CACHED;



        public String getScheme() {

            @SuppressWarnings("StringEquality")

            boolean cached = (scheme != NOT_CACHED);

            boolean cached = (scheme != NotCachedHolder.NOT_CACHED);

            return cached ? scheme : (scheme = parseScheme());

        }
@@ -968,11 +979,11 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
            return -1;

        }



        private volatile String cachedString = NOT_CACHED;

        private volatile String cachedString = NotCachedHolder.NOT_CACHED;



        public String toString() {

            @SuppressWarnings("StringEquality")

            boolean cached = cachedString != NOT_CACHED;

            boolean cached = cachedString != NotCachedHolder.NOT_CACHED;

            if (cached) {

                return cachedString;

            }
@@ -1102,11 +1113,11 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
            return getUserInfoPart().getDecoded();

        }



        private volatile String host = NOT_CACHED;

        private volatile String host = NotCachedHolder.NOT_CACHED;



        public String getHost() {

            @SuppressWarnings("StringEquality")

            boolean cached = (host != NOT_CACHED);

            boolean cached = (host != NotCachedHolder.NOT_CACHED);

            return cached ? host : (host = parseHost());

        }
@@ -1305,12 +1316,12 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
            return this.path.getPathSegments();

        }



        private volatile String uriString = NOT_CACHED;

        private volatile String uriString = NotCachedHolder.NOT_CACHED;



        @Override

        public String toString() {

            @SuppressWarnings("StringEquality")

            boolean cached = (uriString != NOT_CACHED);

            boolean cached = (uriString != NotCachedHolder.NOT_CACHED);

            return cached ? uriString

                    : (uriString = makeUriString());

        }
@@ -1992,13 +2003,13 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
        private final int mCanonicalRepresentation;



        AbstractPart(String encoded, String decoded) {

            if (encoded != NOT_CACHED) {

            if (encoded != NotCachedHolder.NOT_CACHED) {

                this.mCanonicalRepresentation = REPRESENTATION_ENCODED;

                this.encoded = encoded;

                this.decoded = NOT_CACHED;

            } else if (decoded != NOT_CACHED) {

                this.decoded = NotCachedHolder.NOT_CACHED;

            } else if (decoded != NotCachedHolder.NOT_CACHED) {

                this.mCanonicalRepresentation = REPRESENTATION_DECODED;

                this.encoded = NOT_CACHED;

                this.encoded = NotCachedHolder.NOT_CACHED;

                this.decoded = decoded;

            } else {

                throw new IllegalArgumentException("Neither encoded nor decoded");
@@ -2009,7 +2020,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 


        final String getDecoded() {

            @SuppressWarnings("StringEquality")

            boolean hasDecoded = decoded != NOT_CACHED;

            boolean hasDecoded = decoded != NotCachedHolder.NOT_CACHED;

            return hasDecoded ? decoded : (decoded = decode(encoded));

        }
@@ -2023,7 +2034,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
                throw new IllegalArgumentException("Unknown representation: "

                    + mCanonicalRepresentation);

            }

            if (canonicalValue == NOT_CACHED) {

            if (canonicalValue == NotCachedHolder.NOT_CACHED) {

                throw new AssertionError("Canonical value not cached ("

                    + mCanonicalRepresentation + ")");

            }
@@ -2054,7 +2065,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 


        String getEncoded() {

            @SuppressWarnings("StringEquality")

            boolean hasEncoded = encoded != NOT_CACHED;

            boolean hasEncoded = encoded != NotCachedHolder.NOT_CACHED;

            return hasEncoded ? encoded : (encoded = encode(decoded));

        }
@@ -2085,7 +2096,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
         * @param encoded part string

         */

        static Part fromEncoded(String encoded) {

            return from(encoded, NOT_CACHED);

            return from(encoded, NotCachedHolder.NOT_CACHED);

        }



        /**
@@ -2094,7 +2105,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
         * @param decoded part string

         */

        static Part fromDecoded(String decoded) {

            return from(NOT_CACHED, decoded);

            return from(NotCachedHolder.NOT_CACHED, decoded);

        }



        /**
@@ -2105,7 +2116,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
         */

        static Part from(String encoded, String decoded) {

            // We have to check both encoded and decoded in case one is

            // NOT_CACHED.

            // NotCachedHolder.NOT_CACHED.



            if (encoded == null) {

                return NULL;
@@ -2159,7 +2170,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 


        String getEncoded() {

            @SuppressWarnings("StringEquality")

            boolean hasEncoded = encoded != NOT_CACHED;

            boolean hasEncoded = encoded != NotCachedHolder.NOT_CACHED;



            // Don't encode '/'.

            return hasEncoded ? encoded : (encoded = encode(decoded, "/"));
@@ -2265,7 +2276,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
         * @param encoded part string

         */

        static PathPart fromEncoded(String encoded) {

            return from(encoded, NOT_CACHED);

            return from(encoded, NotCachedHolder.NOT_CACHED);

        }



        /**
@@ -2274,7 +2285,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
         * @param decoded part string

         */

        static PathPart fromDecoded(String decoded) {

            return from(NOT_CACHED, decoded);

            return from(NotCachedHolder.NOT_CACHED, decoded);

        }



        /**
@@ -2301,7 +2312,7 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 
         */

        static PathPart makeAbsolute(PathPart oldPart) {

            @SuppressWarnings("StringEquality")

            boolean encodedCached = oldPart.encoded != NOT_CACHED;

            boolean encodedCached = oldPart.encoded != NotCachedHolder.NOT_CACHED;



            // We don't care which version we use, and we don't want to force

            // unneccessary encoding/decoding.
@@ -2314,14 +2325,14 @@ public abstract class Uri implements Parcelable, Comparable<Uri> { 


            // Prepend encoded string if present.

            String newEncoded = encodedCached

                    ? "/" + oldPart.encoded : NOT_CACHED;

                    ? "/" + oldPart.encoded : NotCachedHolder.NOT_CACHED;



            // Prepend decoded string if present.

            @SuppressWarnings("StringEquality")

            boolean decodedCached = oldPart.decoded != NOT_CACHED;

            boolean decodedCached = oldPart.decoded != NotCachedHolder.NOT_CACHED;

            String newDecoded = decodedCached

                    ? "/" + oldPart.decoded

                    : NOT_CACHED;

                    : NotCachedHolder.NOT_CACHED;



            return new PathPart(newEncoded, newDecoded);

        }

core/tests/coretests/src/android/net/UriTest.java

+17 −0
Original line number Diff line number Diff line
@@ -136,6 +136,23 @@ public class UriTest extends TestCase { 
        assertEquals(0, b.compareTo(b2));

    }



    /**

     * Check that {@link Uri#EMPTY} is properly initialized to guard against a

     * regression based on a problematic initialization order (b/159907422).

     *

     * The problematic initialization order happened when {@code Uri$PathPart<clinit>}

     * ran before {@code Uri.<clinit>}. De facto this test would probably never have

     * failed on Android because {@code Uri.<clinit>} will almost certainly have run

     * somewhere in the Zygote, but just in case and in case this test is ever run on

     * a platform that doesn't perform Zygote initialization, this test attempts to

     * trigger {@code Uri$PathPart<clinit>} prior to inspecting {@link Uri#EMPTY}.

     */

    @SmallTest

    public void testEmpty_initializerOrder() {

        new Uri.Builder().scheme("http").path("path").build();

        assertEquals("", Uri.EMPTY.toString());

    }



    @SmallTest

    public void testEqualsAndHashCode() {