1155331 - MediaPlayerManager may be stripped by ProGuard because it's only accessed via reflection

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Forked from bug 1154836 - we're missing some annotations here.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Chris, if @RobocopTarget is applied to a class, can its inner classes be stripped out by ProGuard?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: MozReview Request: bz://1155331/mcomella

/r/7139 - Bug 1155331 - Add ReflectionTarget annotation. r=nalexander
/r/7141 - Bug 1155331 - Annotate MediaPlayerManager with @ReflectionTarget. r=nalexander

Pull down these commits:

hg pull -r 9d1d5fd04d67f4c6eb8d02711f955bcbd24d2ed3 https://reviewboard-hg.mozilla.org/gecko/

Comment 3

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

If Chris doesn't come up with an answer soon, we'll just land this as is (assuming r+).

Comment 4

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/7141/#review5927

Ship It!

Comment 5

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

I just realized that MediaPlayerManager.newInstance and MediaPlayerManager.MEDIA_PLAYER_TAG, the two attributes accessed via reflection, have @JNITarget tags - these changesets may be unnecessary (though imo @ReflectionTarget is worthwhile). Let's wait to see what Chris has to say if this doesn't cause any crashes.

Comment 6

[Chris Kitching [:ckitching]]

(In reply to Michael Comella (:mcomella) from comment #1)
> Chris, if @RobocopTarget is applied to a class, can its inner classes be
> stripped out by ProGuard?

Yes, they can. Alas I couldn't find a way to configure the annotation to recursively keep everything.

This is one of the reasons why I was working towards desugaring our Proguard annotations in a bunch of literal Proguard config rules, because it lets us work around the more silly things like this in Proguard's config system.


Sorry for the slow reply!

Comment 7

[Chris Kitching [:ckitching]]

s/annotations in/annotations to/

*ahem*

Comment 8

[Chris Kitching [:ckitching]]

(It also looks like you forgot to add your new annotation to proguard.cfg: see comment on reviewboard)

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8593547 [details]
MozReview Request: bz://1155331/mcomella

mcomella: RB sucks.  This had a Ship It! but not the right one, I guess.  Do what you will with this r+; you understand this better than me.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

NI self to do this later.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: MozReview Request: Bug 1155331 - Add ReflectionTarget annotation. r=nalexander

Comment 13

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: MozReview Request: Bug 1155331 - Annotate MediaPlayerManager with @ReflectionTarget. r=nalexander

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8620067 [details]
MozReview Request: Bug 1155331 - Add ReflectionTarget annotation. r=nalexander

Bug 1155331 - Add ReflectionTarget annotation. r=nalexander

This is a clone of @JNITarget and @RobocopTarget.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8620068 [details]
MozReview Request: Bug 1155331 - Annotate MediaPlayerManager with @ReflectionTarget. r=nalexander

Bug 1155331 - Annotate MediaPlayerManager with @ReflectionTarget. r=nalexander

The annotated methods were using @JNITarget, which would do the same thing.
However, @ReflectionTarget is clearer (like a good variable name).

proguard.cfg states a class with an annotated member will be kept so we
shouldn't need to worry about not being able to retrieve the class via
reflection.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620067 [details]
MozReview Request: Bug 1155331 - Add ReflectionTarget annotation. r=nalexander

https://reviewboard.mozilla.org/r/7139/#review15567

Looks good!

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620068 [details]
MozReview Request: Bug 1155331 - Annotate MediaPlayerManager with @ReflectionTarget. r=nalexander

https://reviewboard.mozilla.org/r/7141/#review15569

Rock and roll.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

url:        https://hg.mozilla.org/integration/fx-team/rev/de820c428f6c794b45c72230dc694f632612b2fe
changeset:  de820c428f6c794b45c72230dc694f632612b2fe
user:       Michael Comella <[e-mail]>
date:       Thu Apr 16 10:10:25 2015 -0700
description:
Bug 1155331 - Add ReflectionTarget annotation. r=nalexander

This is a clone of @JNITarget and @RobocopTarget.

url:        https://hg.mozilla.org/integration/fx-team/rev/9c204c225f639821922dc6afffa7a41b3de07abe
changeset:  9c204c225f639821922dc6afffa7a41b3de07abe
user:       Michael Comella <[e-mail]>
date:       Wed Apr 15 11:46:20 2015 -0700
description:
Bug 1155331 - Annotate MediaPlayerManager with @ReflectionTarget. r=nalexander

The annotated methods were using @JNITarget, which would do the same thing.
However, @ReflectionTarget is clearer (like a good variable name).

proguard.cfg states a class with an annotated member will be kept so we
shouldn't need to worry about not being able to retrieve the class via
reflection.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de820c428f6c
https://hg.mozilla.org/mozilla-central/rev/9c204c225f63