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

RESOLVED FIXED in Firefox 43

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 43
All
Android
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Forked from bug 1154836 - we're missing some annotations here.
(Assignee)

Updated

3 years ago
Assignee: nobody → michael.l.comella
(Assignee)

Comment 1

3 years ago
Chris, if @RobocopTarget is applied to a class, can its inner classes be stripped out by ProGuard?
Flags: needinfo?(chriskitching)
(Assignee)

Comment 2

3 years ago
Created attachment 8593547 [details]
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/
Attachment #8593547 - Flags: review?(nalexander)
(Assignee)

Comment 3

3 years ago
If Chris doesn't come up with an answer soon, we'll just land this as is (assuming r+).
https://reviewboard.mozilla.org/r/7141/#review5927

Ship It!
(Assignee)

Comment 5

3 years ago
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.
(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!
Flags: needinfo?(chriskitching)
s/annotations in/annotations to/

*ahem*
(It also looks like you forgot to add your new annotation to proguard.cfg: see comment on reviewboard)
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.
Attachment #8593547 - Flags: review?(nalexander) → review+
(Assignee)

Comment 10

3 years ago
NI self to do this later.
Flags: needinfo?(michael.l.comella)
(Assignee)

Updated

3 years ago
Flags: needinfo?(michael.l.comella)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8593547 [details]
MozReview Request: bz://1155331/mcomella
Attachment #8593547 - Attachment is obsolete: true
Attachment #8620067 - Flags: review+
Attachment #8620068 - Flags: review+
(Assignee)

Comment 12

3 years ago
Created attachment 8620067 [details]
MozReview Request: Bug 1155331 - Add ReflectionTarget annotation. r=nalexander
(Assignee)

Comment 13

3 years ago
Created attachment 8620068 [details]
MozReview Request: Bug 1155331 - Annotate MediaPlayerManager with @ReflectionTarget. r=nalexander
(Assignee)

Comment 14

2 years ago
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.
Attachment #8620067 - Flags: review+ → review?(nalexander)
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8620068 - Flags: review+ → review?(nalexander)
Comment on attachment 8620067 [details]
MozReview Request: Bug 1155331 - Add ReflectionTarget annotation. r=nalexander

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

Looks good!
Attachment #8620067 - Flags: review?(nalexander) → review+
Attachment #8620068 - Flags: review?(nalexander) → review+
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.
(Assignee)

Comment 18

2 years ago
url:        https://hg.mozilla.org/integration/fx-team/rev/de820c428f6c794b45c72230dc694f632612b2fe
changeset:  de820c428f6c794b45c72230dc694f632612b2fe
user:       Michael Comella <michael.l.comella@gmail.com>
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 <michael.l.comella@gmail.com>
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.
https://hg.mozilla.org/mozilla-central/rev/de820c428f6c
https://hg.mozilla.org/mozilla-central/rev/9c204c225f63
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.