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

RESOLVED FIXED in Firefox 33

Status

()

Firefox for Android
Screencasting
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aaronmt, Assigned: wesj)

Tracking

({crash})

Trunk
Firefox 34
All
Android
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, fennec33+)

Details

(crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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)
(Reporter)

Updated

4 years ago
Blocks: 901803
These are from a test build. The functionality is not in Nightly yet.
Assignee: nobody → wjohnston
(Reporter)

Comment 2

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

Minority Report
(Assignee)

Comment 3

4 years ago
Created attachment 8474849 [details] [diff] [review]
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.
Attachment #8474849 - Flags: review?(mark.finkle)
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.
Attachment #8474849 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 5

4 years ago
Created attachment 8475354 [details] [diff] [review]
Patch 1/2 - Catch some errors

No problem
Attachment #8474849 - Attachment is obsolete: true
Attachment #8475354 - Flags: review?(mark.finkle)
(Assignee)

Comment 6

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

Err. Actually, lets go a bit further. second patch coming.
Attachment #8475354 - Flags: review?(mark.finkle)
(Assignee)

Comment 7

4 years ago
Created attachment 8475367 [details] [diff] [review]
Patch 1/2 - Catch some errors

This doesn't include any of the toast or string changes.
Attachment #8475354 - Attachment is obsolete: true
Attachment #8475367 - Flags: review?(mark.finkle)
(Assignee)

Comment 8

4 years ago
Created attachment 8475369 [details] [diff] [review]
Patch 2/2 - Toast ui

And this is all of the toast/string changes.
Attachment #8475369 - Flags: review?(mark.finkle)
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
Attachment #8475367 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 10

4 years ago
Created attachment 8476159 [details] [diff] [review]
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 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.
Attachment #8476159 - Attachment description: attachment.cgi?id=8475367 → Patch 1/2 - Catch some errors
Attachment #8476159 - Attachment is patch: true
Attachment #8475367 - Attachment is obsolete: true
Comment on attachment 8476159 [details] [diff] [review]
Patch 1/2 - Catch some errors

Let's see how this works out. Should be an improvement!
Attachment #8476159 - Flags: review+
Comment on attachment 8475369 [details] [diff] [review]
Patch 2/2 - Toast ui

I want to wait for Toast.jsm
Attachment #8475369 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 14

4 years ago
[Tracking Requested - why for this release]:
tracking-firefox33: --- → ?
tracking-fennec: --- → 33+
status-firefox33: --- → affected
status-firefox34: --- → affected
tracking-firefox33: ? → ---
(Assignee)

Comment 16

4 years ago
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.
Attachment #8476159 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/eb50b87d7685
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
status-firefox34: affected → fixed
Attachment #8476159 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox33: affected → fixed
(Assignee)

Comment 19

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

will reland shortly.
status-firefox33: fixed → affected
(Assignee)

Updated

4 years ago
status-firefox33: affected → fixed
Blocks: 1046536
You need to log in before you can comment on or make changes to this bug.