889185 - Send OrderedBroadcast.js token to Java and distinguish between null and default permissions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: part-1-OrderedBroadcast-token.patch

Patch 1 was written by nalexander.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-2-OrderedBroadcast-permissions.patch

Part 2: Distinguish between OrderedBroadcasts with null and default/undefined permissions.

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 769989 [details] [diff] [review]
part-2-OrderedBroadcast-permissions.patch

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

Address tests, r=nalexander.

::: mobile/android/base/OrderedBroadcastHelper.java
@@ +78,5 @@
>  
> +            // A missing (undefined) permission means the intent will be limited
> +            // to the current package. A null means no permission, so any
> +            // package can receive the intent.
> +            final String permission = message.has("permission")

Does this look better with isNull ? optString(..., PER_ANDROID_PACKAGE_PERMISSION)?

::: mobile/android/modules/OrderedBroadcast.jsm
@@ +79,5 @@
>      type: "OrderedBroadcast:Send",
>      action: action,
>      responseEvent: responseEvent,
>      token: { callbackId: callbackId, data: token || null },
> +    permission: permission,

This should break the tests.  If it doesn't, something is seriously wrong.  Please verify and update the tests.

Comment 3

[Chris Peterson [:cpeterson]]

Green try tests:
https://tbpl.mozilla.org/?tree=Try&rev=23111a39ccd9

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Peterson (:cpeterson) from comment #3)
> Green try tests:
> https://tbpl.mozilla.org/?tree=Try&rev=23111a39ccd9

Fair enough.  Looking at the tests again, I see how this happens.  Bonus points for testing the new functionality, but you'd need to add an additional permission protected test receiver and I'm not that concerned about it.  This is mostly for calling Fennec receivers anyway, and JS callers will quickly find out if their permissions are incorrect.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Nick Alexander :nalexander from comment #2)
> Does this look better with isNull ? optString(..., PER_ANDROID_PACKAGE_PERMISSION)?

I don't think this code can be simplified with optString(). If a permission value exists and is null, optString() will return the string "null" instead of null. isNull() will return null if a value exists and is null or if a value does not exist (undefined), so we still need to check has("permission") to differentiate between null and undefined values.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 769989 [details] [diff] [review]
part-2-OrderedBroadcast-permissions.patch

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

::: mobile/android/base/OrderedBroadcastHelper.java
@@ +80,5 @@
> +            // to the current package. A null means no permission, so any
> +            // package can receive the intent.
> +            final String permission = message.has("permission")
> +                                      ? (message.isNull("permission") ? null : message.getString("permission"))
> +                                      : GlobalConstants.PER_ANDROID_PACKAGE_PERMISSION;

Nit: prefer operators at end of line, like the rest of the file.

::: mobile/android/modules/OrderedBroadcast.jsm
@@ +84,1 @@
>    });

For the record, this direct attributing works because of rule JO#8 in <http://es5.github.io/#x15.12.3>, which specifies that undefined-valued attributes aren't serialized... because JSONObject will parse them as the string "undefined", which would get treated as a permission.

Glad I checked that :P

Comment 7

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f43a6971bb1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bf318e6a90

Fixed operators at end of line noted in comment 6.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f43a6971bb1b
https://hg.mozilla.org/mozilla-central/rev/b6bf318e6a90