1046537 - crash in java.lang.NullPointerException: at org.mozilla.gecko.ChromeCast.pause(ChromeCast.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-2e3f09a8-1d4c-4e92-8253-db2f02140730.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.ChromeCast.pause(ChromeCast.java:231)
	at org.mozilla.gecko.MediaPlayerManager.handleMessage(MediaPlayerManager.java:151)
	at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:162)
	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2200)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:354)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:178)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

These are from a test build. The functionality is not in Nightly yet.

Comment 2

[Aaron Train [:aaronmt]]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> These are from a test build. The functionality is not in Nightly yet.

Minority Report

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

I managed to reproduce a crash here by using a video that has some sort of error:
http://www.w3.org/2010/05/video/mediaevents.html

That will (currently) result in the media bar never going away. If you tap "pause" while its up, we'll crash because we have no connection. This test may also have just hit it too early. This adds some safety checks in front of pause/play/end. It also returns errors from them, and updates the JS to 1.) Show the errors in a toast (worried about localization here) 2.) shuts down casting to the Chromecast when it gets them. To make that really work, we also have to call closeExternal when we shutdown in CastingApps (so that the casting button in the urlbar/videos is removed).

I also just updated the js code in a few other places to better report errors.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8474849 [details] [diff] [review]
Patch

A lot of this patch is about showing error toasts. Can you make a patch that only fixes the crash? The error toasts are an extra we can worry about later.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Catch some errors

No problem

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8475354 [details] [diff] [review]
Patch 1/2 - Catch some errors

Err. Actually, lets go a bit further. second patch coming.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Catch some errors

This doesn't include any of the toast or string changes.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Toast ui

And this is all of the toast/string changes.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8475367 [details] [diff] [review]
Patch 1/2 - Catch some errors

>diff --git a/mobile/android/base/ChromeCast.java b/mobile/android/base/ChromeCast.java

> class ChromeCast implements GeckoMediaPlayer {
>-    private static final boolean SHOW_DEBUG = false;
>+    private static final boolean SHOW_DEBUG = true;

true for release?

I am not sure about sending strings back to sendError. Seems like integers would be a better choice.

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js

>   onRemoteMediaStop: function(aRemoteMedia) {
>     sendMessageToJava({ type: "Casting:Stopped" });
>+    this.closeExternal();

I don't think this is right. What part of closeExternal do you need to be called? closeExternal will shutdown the remoteMedia, which leads us back to onRemoteMediaStop. "Cast:Stop" already calls closeExternal, so it seems like you need a part of closeExternal pulled out into it's own function and called from closeExternal and here.

>diff --git a/mobile/android/modules/MediaPlayerApp.jsm b/mobile/android/modules/MediaPlayerApp.jsm

>   mirror: function mirror(callback) {

>+      if (callback) callback(err == null);

braces

>   play: function play() {
>-    this._send("MediaPlayer:Play", {}, (result) => {
>+    this._send("MediaPlayer:Play", {}, (result, err) => {
>+      if (err) {
>+        Cu.reportError("Can't play " + err);
>+        this.shutdown();
>+        return;

Why using Cu.reportError here but log above?

r- for the closeExternal stuff and to think about ints vs strings and tweak Cu.reportError

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Catch some errors

(In reply to Mark Finkle (:mfinkle) from comment #9)
> I am not sure about sending strings back to sendError. Seems like integers
> would be a better choice.
Why? I'm not sure what Gecko would do with integers. I wanted to avoid situations where we tell the user "Received error code 6", but because the nice descriptions are often empty, I went this route where we get both the code and the string. I actually started to refactor to remove these since we don't show anything anymore and decided it was a waste (and our current Messaging api means its impossible to discern between a success state and an error state if you don't return something, so if we have to return something, the strings seemed simple).

> you need a part of closeExternal pulled out into it's own function and
> called from closeExternal and here.
Done. I fixed this up so that calling shutdown too many times was resilient, but seems good to not knowingly do it.

> Why using Cu.reportError here but log above?
In one case the error status is actually sent on forward, in the other it isn't. I assumed log was something we'd stub to do nothing in some cases. But since its not, I just removed the logging in the places where there is a callback.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8476159 [details] [diff] [review]
Patch 1/2 - Catch some errors

I am assuming this is a patch for review. I don't know how you got bugzilla to butcher the name.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8476159 [details] [diff] [review]
Patch 1/2 - Catch some errors

Let's see how this works out. Should be an improvement!

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8475369 [details] [diff] [review]
Patch 2/2 - Toast ui

I want to wait for Toast.jsm

Comment 14

[Wesley Johnston (:wesj)]

[Tracking Requested - why for this release]:

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/eb50b87d7685

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 8476159 [details] [diff] [review]
Patch 1/2 - Catch some errors

Approval Request Comment
[Feature/regressing bug #]: chromecast casting
[User impact if declined]: crashes at key points
[Describe test coverage new/current, TBPL]: none yet. working on it.
[Risks and why]: Pretty low risk. Just sending better error messaging to JS when things go wrong. Doing some more safety checks before trying to talk to the chromecast.
[String/UUID change made/needed]: None. There are strings here but they aren't presented in the UI. Just in the error console. The second patch here uses them, and will be delayed a bit. This just fails silently.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eb50b87d7685

Comment 18

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c4d96765e74

Comment 19

[Wesley Johnston (:wesj)]

backed out for bustage (left over mirroring code):
https://hg.mozilla.org/releases/mozilla-aurora/rev/52f54e8b5414

will reland shortly.

Comment 20

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3278a50ce13