Closed Bug 870333 Opened 7 years ago Closed 7 years ago

java.lang.NullPointerException: at android.view.inputmethod.BaseInputConnection.isBracketChar(BaseInputConnection.java)

Categories

(Firefox for Android :: Keyboards and IME, defect, critical)

ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 24
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
firefox24 --- fixed

People

(Reporter: scoobidiver, Assigned: briancecker)

References

Details

(Keywords: crash, Whiteboard: [native-crash][lang=java][mentor=cpeterson@mozilla.com])

Crash Data

Attachments

(1 file, 1 obsolete file)

There are one crash in 23.0a1, bp-4c2131bc-1b41-4ec5-85a8-c0ff72130509, 61 in 20.0.1, and 4 in 21.0b6.

java.lang.NullPointerException
	at android.view.inputmethod.BaseInputConnection.isBracketChar(BaseInputConnection.java:808)
	at android.view.inputmethod.BaseInputConnection.replaceText(BaseInputConnection.java:672)
	at android.view.inputmethod.BaseInputConnection.commitText(BaseInputConnection.java:196)
	at org.mozilla.gecko.GeckoInputConnection.commitText(GeckoInputConnection.java:683)
	at org.mozilla.gecko.GeckoInputConnection.performContextMenuAction(GeckoInputConnection.java:284)
	at com.android.internal.view.IInputConnectionWrapper.executeMessage(IInputConnectionWrapper.java:306)
	at com.android.internal.view.IInputConnectionWrapper$MyHandler.handleMessage(IInputConnectionWrapper.java:77)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:137)
	at org.mozilla.gecko.GeckoInputConnection$1.run(GeckoInputConnection.java:486)
	at java.lang.Thread.run(Thread.java:856)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+android.view.inputmethod.BaseInputConnection.isBracketChar%28BaseInputConnection.java%29
This is a problem when pasting from an empty clipboard. GeckoAppShell.getClipboardText() is returning null here:

    public boolean performContextMenuAction(int id) {
        ...
            case R.id.paste:
                commitText(GeckoAppShell.getClipboardText(), 1);
                break;
Whiteboard: [native-crash] → [native-crash][lang=java][mentor=cpeterson@mozilla.com]
I can take this one. 

Looking at both getClipboardText() and performContextMenuAction(..) there seems to be a couple options. Would it be preferable to make getClipboardText() return some empty string rather than null, or to make the R.id.paste case check the result of getClipboardText(), and fill with an empty string if null? The choice may seem a little irrelevant, but I'm worried that changing the return result of getClipboardText() might break other things that use it.
I think returning "" from getClipboardText() would be safer.

If getClipboardText() always returns a non-null string, then we can replace the callers' null and TextUtils.isEmpty() checks with simpler string.isEmpty() checks.

When you remove TextUtils.isEmpty(), you should also remove `import android.text.TextUtils` if the .java file has no other calls to TextUtils.
Assignee: nobody → briancecker
Status: NEW → ASSIGNED
Attached patch potential patch (obsolete) — Splinter Review
getClipboardText() now returns and empty string instead of null, and callers now use String.isEmpty() tests.
Attachment #748275 - Flags: review?(cpeterson)
Oh, vim also automatically removed some trailing whitespaces, so those are included as well.
Comment on attachment 748275 [details] [diff] [review]
potential patch

Review of attachment 748275 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. Here are some suggestions.

Also, there is an "erorr" typo in the patch description. The description should probably also mention that this fix is related to clipboard text.

::: mobile/android/base/GeckoAppShell.java
@@ +1215,5 @@
>              }
>          });
>          try {
>              String ret = sClipboardQueue.take();
> +            return (EMPTY_STRING.equals(ret) ? EMPTY_STRING : ret);

1. Can we just `return sClipboardQueue.take()` here without checking ret or EMPTY_STRING?

2. Can we remove the EMPTY_STRING definition altogether and just use ""? I think I tried that before and accidentally introduced some test failures. I can run your patch on our test servers to see if that is still a problem.

@@ +1220,2 @@
>          } catch (InterruptedException ie) {}
> +        return EMPTY_STRING;

Let's move this `return` inside the `catch` block. I think that would be clearer because an InterruptedException is the only reason we would actually reach this `return`.
Attachment #748275 - Flags: review?(cpeterson) → feedback+
Brian, I ran your patch on the "try" test servers and found a problem:

PROCESS-CRASH | java-exception | java.lang.NoSuchMethodError: java.lang.String.isEmpty at org.mozilla.gecko.BrowserToolbar$3.onCreateContextMenu(BrowserToolbar.java:186)

https://tbpl.mozilla.org/php/getParsedLog.php?id=22843250&tree=Try#error0


The String.isEmpty() method [1] was added in Gingerbread (Android API Level 9), but we still support Froyo (Android API Level 8). So we need to replace the new String.isEmpty() checks with `TextUtils.isEmpty()`.

[1] https://developer.android.com/reference/java/lang/String.html#isEmpty%28%29
Ah so that's why that was being used. What settings did you use for the try server? I ran it on the try server myself last night and didn't get a crash, but I was using limited testing options.
Oh, nevermind I see the try settings in the report.
Attached patch patch_v2Splinter Review
Fixed the things you commented on. Try server results are here:
https://tbpl.mozilla.org/?tree=Try&rev=e5f86b2cdfce
Attachment #748275 - Attachment is obsolete: true
Attachment #748596 - Flags: feedback?(cpeterson)
Duplicate of this bug: 830825
Comment on attachment 748596 [details] [diff] [review]
patch_v2

Review of attachment 748596 [details] [diff] [review]:
-----------------------------------------------------------------

The changes look good, but the tryserver run had some rc1 test failures. I'm rerunning the rc1 tests to see if they are intermittent failures or related to the getClipboardText() changes.
Attachment #748596 - Flags: feedback?(cpeterson) → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4012822438ce

Brian, I landed your patch on mozilla-inbound. I removed the trailing whitespace changes because, even though I agree trailing whitespace is sloppy, I wanted to make the patch smaller so it would be easier to uplift to the Aurora channel. You fixed an old crash that affects all release channels, so I think uplifting it will improve our crash stats. :D
Crash Signature: [@ java.lang.NullPointerException: at android.view.inputmethod.BaseInputConnection.isBracketChar(BaseInputConnection.java)] → [@ java.lang.NullPointerException: at android.view.inputmethod.BaseInputConnection.isBracketChar(BaseInputConnection.java)] [@ java.lang.NullPointerException: at org.mozilla.gecko.GeckoEditable.replace(GeckoEditable.java)]
Comment on attachment 748596 [details] [diff] [review]
patch_v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Longstanding (4+ months) but low volume clipboard crash goes unfixed.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Low risk because it is adding some null checks.
String or IDL/UUID changes made by this patch: N/A
Attachment #748596 - Flags: review+
Attachment #748596 - Flags: approval-mozilla-aurora?
Attachment #748596 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/4012822438ce
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Brian, thanks for fixing this crash! It's been long-standing problem. <:)
My pleasure :)
You need to log in before you can comment on or make changes to this bug.