Closed Bug 1154836 Opened 5 years ago Closed 5 years ago

Crash on HC - java.lang.ClassNotFoundException: org.mozilla.gecko.MediaPlayerManager$GeckoPresentation

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
firefox38 --- affected
firefox39 --- affected
firefox40 --- fixed
fennec 40+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(2 files, 1 obsolete file)

Found from the crash in bug 1152314 comment 14:

 java.lang.ClassNotFoundException: org.mozilla.gecko.MediaPlayerManager$GeckoPresentation

It's only accessed via reflection in BrowserApp.getMediaPlayerManager so the class should be annotated with @RobocopTarget. If the class is annotated, the inner class shouldn't need to be annotated either because I think all elements in an annotated class should be preserved.
This code path exists in 38 & 40 so I assume it's also in 39.
Assignee: nobody → michael.l.comella
/r/7081 - Bug 1154836 - Annotate MediaPlayerManager with @RobocopTarget. r=nalexander

Pull down this commit:

hg pull -r b4875b267e7acd4ac0cdabfc069c9e528a7e0077 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592944 - Flags: review?(nalexander)
tracking-fennec: --- → ?
Attachment #8592944 - Flags: review?(nalexander)
Comment on attachment 8592944 [details]
MozReview Request: bz://1154836/mcomella

https://reviewboard.mozilla.org/r/7079/#review5881

::: mobile/android/base/MediaPlayerManager.java
(Diff revision 1)
> +@RobocopTarget

I am not certain this annotation applies to inner classes, and I cannot find a reference.  If you can, roll on.  If not, be safe and annotate both inner classes.
Comment on attachment 8592944 [details]
MozReview Request: bz://1154836/mcomella

https://reviewboard.mozilla.org/r/7079/#review5885

Ship It!
Attachment #8592944 - Flags: review+
Chris, if @RobocopTarget is applied to a class, can its inner classes be stripped out by ProGuard?
Flags: needinfo?(chriskitching)
I think @JNITarget (or a clone of it: @ReflectionTarget?) is a better annotation than @RobocopTarget.

If we manage to rework how we do testing, @RobocopTarget will go away, and this fix with it.

For inner classes, I think you'd need -keepattributes InnerClasses. Put the annotation on the inner class, too. No harm in being explicit.
I have updated to latest Nightly(2015-04-16) on Acer Iconia (Android 3.2) and keeps crashing and becomes unusable. I cannot see the crash in about:crashes, but here are the logs: https://pastebin.mozilla.org/8830301. The same logs were mentioned also in bug 1152314 comment 14:
(In reply to Nick Alexander :nalexander from comment #3)
> I am not certain this annotation applies to inner classes, and I cannot find
> a reference.  If you can, roll on.  If not, be safe and annotate both inner
> classes.

Threw up a test build with my change here in bug 1152314 comment 17 but Catalin said it also failed in bug 1152314 comment 19. Same exception but some additional info I noticed in the logs, which existed in the old logs:

 W/dalvikvm( 3943): Unable to resolve superclass of Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation; (56)
 W/dalvikvm( 3943): Link of class 'Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation;' failed
 I/dalvikvm( 3943): Could not find method org.mozilla.gecko.MediaPlayerManager$GeckoPresentation.getDisplay, referenced from method org.mozilla.gecko.MediaPlayerManager.updatePresentation
 W/dalvikvm( 3943): VFY: unable to resolve virtual method 23128: Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation;.getDisplay ()Landroid/view/Display;

Seems like we'll need to annotate the inner class too.
Flags: needinfo?(chriskitching)
(In reply to Michael Comella (:mcomella) from comment #9)
>  W/dalvikvm( 3943): Unable to resolve superclass of
> Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation; (56)
>  W/dalvikvm( 3943): Link of class
> 'Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation;' failed
>  I/dalvikvm( 3943): Could not find method
> org.mozilla.gecko.MediaPlayerManager$GeckoPresentation.getDisplay,
> referenced from method
> org.mozilla.gecko.MediaPlayerManager.updatePresentation
>  W/dalvikvm( 3943): VFY: unable to resolve virtual method 23128:
> Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation;.getDisplay
> ()Landroid/view/Display;

I started to think that maybe we died in class loading because we couldn't find GeckoPresentation's super class (Presentation, which is API 17+ - 4.2.*), but I tried out a 4.1.2 device and didn't crash.
(In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Michael Comella (:mcomella) from comment #9)
> >  W/dalvikvm( 3943): Unable to resolve superclass of
> > Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation; (56)
> >  W/dalvikvm( 3943): Link of class
> > 'Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation;' failed
> >  I/dalvikvm( 3943): Could not find method
> > org.mozilla.gecko.MediaPlayerManager$GeckoPresentation.getDisplay,
> > referenced from method
> > org.mozilla.gecko.MediaPlayerManager.updatePresentation
> >  W/dalvikvm( 3943): VFY: unable to resolve virtual method 23128:
> > Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation;.getDisplay
> > ()Landroid/view/Display;
> 
> I started to think that maybe we died in class loading because we couldn't
> find GeckoPresentation's super class (Presentation, which is API 17+ -
> 4.2.*), but I tried out a 4.1.2 device and didn't crash.

There is code to prevent the GeckoPresentation from being created if API < 17.
tracking-fennec: ? → 40+
(In reply to Randall Barker [:rbarker] from comment #11)
> There is code to prevent the GeckoPresentation from being created if API <
> 17.

Right. I don't know the details, but in the Java runtime, at the first access of a class, it gets loaded by a ClassLoader (e.g. this is when the values of the static variables are assigned). 4.1.2 seems to handle this fine but, since the Android 3.* source is closed and PathClassLoader seems to have lost a few hundred lines, I have a theory that 3.* does this differently and throws when it can't access these other resources.

Alternatively, there's the existing theory that ProGuard is munching some symbols. However, this crashes on my local builds and, as far as I know, ProGuard doesn't run (or does minimal passes) on local builds.
(In reply to Michael Comella (:mcomella) from comment #5)
> Chris, if @RobocopTarget is applied to a class, can its inner classes be
> stripped out by ProGuard?

This is good to know in any case.
Flags: needinfo?(chriskitching)
I think the MediaPlayer stuff is all optionally built. We wanted to be able to build without Google Player Services if I remember? Maybe its worth ending that. Then the reflection could just disappear...
I think we still want to be able to build without GPS. But there are solutions for this that don't involve reflection: two different implementations of the same class. Conditionally build two different files.
Attached file Successful test fix
It seems the issue is solved on HC when we branch before ever accessing the MediaPlayerManager class so I think my class loading theory is correct.

Fix tested by nalexander is attached.
> Alternatively, there's the existing theory that ProGuard is munching some
> symbols. However, this crashes on my local builds and, as far as I know,
> ProGuard doesn't run (or does minimal passes) on local builds.

PG does run on local builds, but does fewer passes: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#148
(In reply to Richard Newman [:rnewman] from comment #15)
> I think we still want to be able to build without GPS. But there are
> solutions for this that don't involve reflection: two different
> implementations of the same class. Conditionally build two different files.

To clarify, this crashes on HC, not 4.1.* and seemingly any other commonly tested devices (4.4+?). I don't think we can conditionally build the file because we'd need to have a HC-specific APK first.

We could inherit from an interface and only create the real MediaPlayerManager on > HC devices, but that sounds overly complicated compared to the fix I crafted.

nalexander and I noticed that this should be caught as an Exception, but it's not - it seems like an exception to Exceptions. [1] has a similar issue (trying to catch ClassNotFoundException fails at compile time because it's "not thrown") and has a terribly hacky fix. We could go that route, but I think I prefer my work-around. I'll try to make it a bit cleaner.

[1]: http://www.ninthavenue.com.au/how-to-catch-a-classnotfoundexception
Filed bug 1155331 to handle the ProGuard stuff.
Summary: MediaPlayerManager may be stripped by PG because it's only accessed via reflection → Crash on HC - java.lang.ClassNotFoundException: org.mozilla.gecko.MediaPlayerManager$GeckoPresentation
-> bug 1155331 comment 1.
Flags: needinfo?(chriskitching)
Attachment #8592944 - Attachment is obsolete: true
Comment on attachment 8592944 [details]
MozReview Request: bz://1154836/mcomella

/r/7081 - Bug 1154836 - Prevent access to MediaPlayerManager at API < 17. r=nalexander

Pull down this commit:

hg pull -r 80a2c34dbc44e2a85e35c5b75ab76ea9cf6b3a99 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592944 - Attachment is obsolete: false
Attachment #8592944 - Flags: review+ → review?(nalexander)
Comment on attachment 8592944 [details]
MozReview Request: bz://1154836/mcomella

https://reviewboard.mozilla.org/r/7079/#review5929

Ship It!
Attachment #8592944 - Flags: review?(nalexander) → review+
Wes brought up a good point that this patch disables casting on all devices. The thing we want to avoid, the level 17 API Presentation, is just a small part of this. We can probably do better here than disabling casting on all devices - I'll look for a fix.
Presentation is confined to the updatePresentation method, which uses the GeckoPresentation class and the SurfaceListener. We can remove these elements from the base MediaPlayerManager and extend MediaPlayerManager for API 17+ to use the Presentation APIs, overriding updatePresentation. We can use a static get method to construct a specific MediaPlayerManager instance. Working on a patch...
Comment on attachment 8592944 [details]
MozReview Request: bz://1154836/mcomella

/r/7081 - Bug 1154836 - Move Presentation code to PresentationMediaPlayerManager from MediaPlayerManager. r=nalexander

Pull down this commit:

hg pull -r 811560dfab525461a9f65b57dcb9d51624484ff8 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592944 - Flags: review+ → review?(nalexander)
Attachment #8592944 - Flags: review?(nalexander) → review+
BTW, Nick tested the latest build on his HC-device via IRC.
https://hg.mozilla.org/mozilla-central/rev/13a4700da4d5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Duplicate of this bug: 1155678
Attachment #8592944 - Attachment is obsolete: true
Attachment #8620049 - Flags: review+
You need to log in before you can comment on or make changes to this bug.