1274901 - Crash in java.lang.NullPointerException: at android.text.SpannableString.<init>(SpannableString.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-77d4adb5-a480-4499-af65-136832160521.
=============================================================

18 occurrences of this in the past week in versions going back to 47.

java.lang.NullPointerException
	at android.text.SpannableString.<init>(SpannableString.java:30)
	at org.mozilla.gecko.toolbar.ToolbarDisplayLayout.updateFromTab(ToolbarDisplayLayout.java:218)
	at org.mozilla.gecko.toolbar.BrowserToolbar.onTabChanged(BrowserToolbar.java:496)
	at org.mozilla.gecko.Tabs$4.run(Tabs.java:647)
	at android.os.Handler.handleCallback(Handler.java:725)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:153)
	at android.app.ActivityThread.main(ActivityThread.java:5299)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:833)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:600)
	at dalvik.system.NativeStart.main(Native Method)

Margaret, can you please needinfo somebody appropriate for this one? I'm not sure who to ask. I also don't know which Bugzilla component is appropriate.

Comment 1

[:Margaret Leibovic]

Jonathan, want to take a look at this bug?

Comment 2

[Jonathan Almeida [:jonalmeida]]

Sure, looking at this today.

Comment 3

[Jonathan Almeida [:jonalmeida]]

For `updateFromTab` we have @NonNull checks for a tab being passed, so after looking at hg blame and the call stack I found out that in bug 1248908 it was determined that Tab couldn't be null but we can see this isn't true.

We get to the nullptr exception when we're passed a null Tab in Tabs::notifyListeners:
> while (items.hasNext()) {
>     items.next().onTabChanged(tab, msg, data);
> }

The current null check for this is also testing for restored tab event:
> if (tab == null &&
>     msg != TabEvents.RESTORED) {
>     throw new IllegalArgumentException("onTabChanged:" + msg + " must specify a tab.");
> }

One dxr search later, I found that BrowserApp also handles this situation but with a null check that just returns:
> if (tab == null) {
>     // Only RESTORED is allowed a null tab: it's the only event that
>     // isn't tied to a specific tab.
>     if (msg != Tabs.TabEvents.RESTORED) {
>         throw new IllegalArgumentException("onTabChanged:" + msg + " must specify a tab.");
>     }
>     return;
> }

Comment 4

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1274901 - NullPointerException thrown in Tabs.java should be checked r?mcomella

Review commit: https://reviewboard.mozilla.org/r/54944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54944/

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8756080 [details]
MozReview Request: Bug 1274901 - NullPointerException thrown in Tabs.java should be checked r?mcomella

https://reviewboard.mozilla.org/r/54944/#review51966

> One dxr search later, I found that BrowserApp also handles this situation but with a null check that just returns:

It looks like you applied the solution from `BrowserApp.onTabChanged` to `Tabs.notifyListeners`. However, they're at opposite ends of the event queue – BrowserApp is receiving but Tabs is sending – and in this case, they shouldn't be treated the same way.

You should be fixing the issue on the receiving end (and updating any incorrect annotations while you're at it!).

fwiw, in a more ideal solution, we would never send a null tab over the listener. Some possible solutions:
1) On RESTORED events, we send a dummy, empty tab (which, in theory, should never be used like a real tab and can throw if it is)
2) The RESTORED event listener is separated from the other tab event listeners so the other listeners are always have a valid tab and the restored events probably don't pass in a Tab at all.
3) We have an onTabChanged with a tab & onTabChanged w/o a tab (for RESTORED)

That being said, these solutions are outside the scope of this bug (e.g. what if the code was written to assume tabs will be null for RESTORED events, rather than checking the event type?) but I figured it'd be good to document them.

Comment 6

[Jonathan Almeida [:jonalmeida]]

Mainly notes and thoughts after talking with mcomella irl..

So I was going about it the wrong way. By using the early return method, I would be skipping all the listener calls because of the null check, since the nullptr exception is called by `Tab` method calls on a null Tab which causes the exception to be thrown at the SpannableString. 

We need to make sure that we add a null check earlier enough that we don't get to calls being made on the Tab. Earlier enough that `updateDisplayLayout` isn't being called but not further enough before `onTabChanged` where it affects the listeners. Since `updateDisplayLayout` already has a `@NonNull` annotation, we can add a null check where it's being called from (which just happens to be that one location in BrowserToolbar).

In my new patch now, I'm adding a null check in BrowserToolbar so that it fixes the issue but doesn't over-complicate the problem. I've also replaced the `@NonNull` annotation in `BrowserToolbar.onTabChanged` to be `@Nullable` since we now know that we can get null Tabs.

Comment 7

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8756080 [details]
MozReview Request: Bug 1274901 - NullPointerException thrown in Tabs.java should be checked r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54944/diff/1-2/

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8756080 [details]
MozReview Request: Bug 1274901 - NullPointerException thrown in Tabs.java should be checked r?mcomella

https://reviewboard.mozilla.org/r/54944/#review52668

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:632
(Diff revision 2)
>          notifyListeners(tab, msg, "");
>      }
>  
>      public void notifyListeners(final Tab tab, final TabEvents msg, final String data) {
>          if (tab == null &&
> -            msg != TabEvents.RESTORED) {
> +                msg != TabEvents.RESTORED) {

nit: unnecessary ws change

::: mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbar.java:496
(Diff revision 2)
>                  case SECURITY_CHANGE:
>                      flags.add(UpdateFlags.SITE_IDENTITY);
>                      break;
>              }
>  
> -            if (!flags.isEmpty()) {
> +            if (!flags.isEmpty() || tab != null) {

`... && tab != null`, I think?

Comment 9

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/54944/#review52668

> `... && tab != null`, I think?

Shoot, yes that's right.

Comment 10

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8756080 [details]
MozReview Request: Bug 1274901 - NullPointerException thrown in Tabs.java should be checked r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54944/diff/2-3/

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8756080 [details]
MozReview Request: Bug 1274901 - NullPointerException thrown in Tabs.java should be checked r?mcomella

https://reviewboard.mozilla.org/r/54944/#review54150

lgtm

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c537b77d3d25
NullPointerException thrown in Tabs.java should be checked r=mcomella

Comment 13

[Carsten Book [:Tomcat]]

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