868342 - java.lang.NullPointerException: at org.mozilla.gecko.BrowserToolbar.setTitle(BrowserToolbar.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

There's one crash in 23.0a1/20130502: bp-557a0100-7553-4b9a-823e-72dd02130503.

Based on the stack trace, it's likely a regression from bug 857165.

java.lang.NullPointerException
	at org.mozilla.gecko.BrowserToolbar.setTitle(BrowserToolbar.java:989)
	at org.mozilla.gecko.BrowserToolbar.fromAwesomeBarSearch(BrowserToolbar.java:557)
	at org.mozilla.gecko.BrowserApp.onActivityResult(BrowserApp.java:680)
	at android.app.Activity.dispatchActivityResult(Activity.java:5293)
	at android.app.ActivityThread.deliverResults(ActivityThread.java:3315)
	at android.app.ActivityThread.handleSendResult(ActivityThread.java:3362)
	at android.app.ActivityThread.access$1100(ActivityThread.java:141)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1282)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:5041)
	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:793)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:560)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+org.mozilla.gecko.BrowserToolbar.setTitle%28BrowserToolbar.java%29

Comment 1

[:Margaret Leibovic]

It looks like tab.getURL() is returning null here:
http://hg.mozilla.org/mozilla-central/annotate/41ff3b67b692/mobile/android/base/BrowserToolbar.java#l908

I don't exactly follow all the logic in setTitle(), but if tab.getURL() is null, we probably just want to be calling mTitle.setText(null) (it looks like this is what would have happened before the patch for bug 857165 landed).

Comment 2

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #1)
> It looks like tab.getURL() is returning null here:
> http://hg.mozilla.org/mozilla-central/annotate/41ff3b67b692/mobile/android/
> base/BrowserToolbar.java#l908
> 
> I don't exactly follow all the logic in setTitle(), but if tab.getURL() is
> null, we probably just want to be calling mTitle.setText(null) (it looks
> like this is what would have happened before the patch for bug 857165
> landed).

Oh, actually, the tab must be null, since StringUtils.stripScheme(url) won't throw if url is null. Since there are other null checks on the tab in here, this could very well be true.

Comment 3

[:Margaret Leibovic]

Attachment: patch

This just moves all the tab-dependent stuff inside a null-checked if statement.

Reading through this code, the use of the title/displayTitle variables is a bit confusing. I think there's also a bug here that setContentDescription will be called with the wrong title on "about:home" or "about:privatebrowsing", since we're only nulling displayTitle now, not title.

Comment 4

[:Margaret Leibovic]

Attachment: (Part 2) Get rid of displayTitle variable

Not sure if you're going to like this, but this makes the logic clearer to me.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 746577 [details] [diff] [review]
patch

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

This function is getting a bit hairy...

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 746581 [details] [diff] [review]
(Part 2) Get rid of displayTitle variable

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

Nice, and thanks!

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8df5e637a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a23eea95bf64

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c8df5e637a2
https://hg.mozilla.org/mozilla-central/rev/a23eea95bf64

Comment 9

[Scoobidiver (away)]

The patch makes crashes worth with currently 26 crashes in 23.0a1/20130509.

Comment 10

[Scoobidiver (away)]

s/worth/worse in comment 9.

Comment 11

[:Margaret Leibovic]

:( Given the new crash stacks, StringUtils.stripScheme(tab.getURL()) must be returning null, which will if tab.getURL() returns null.

Comment 12

[:Margaret Leibovic]

Attachment: (Part 3) Check for null URL in BrowserToolbar.setTitle

Comment 13

[:Margaret Leibovic]

Comment on attachment 748082 [details] [diff] [review]
(Part 3) Check for null URL in BrowserToolbar.setTitle

Also flagging mfinkle for review in case he could get to this first. It would be nice to land this before the merge.

Comment 14

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

Comment on attachment 748082 [details] [diff] [review]
(Part 3) Check for null URL in BrowserToolbar.setTitle


>-            if (mShowUrl && title != null) {
>-                title = StringUtils.stripScheme(tab.getURL());
>-                title = StringUtils.stripCommonSubdomains(title.toString());
>+            String url = tab.getURL();
>+            if (mShowUrl && title != null && url != null) {
>+                url = StringUtils.stripScheme(url);
>+                title = StringUtils.stripCommonSubdomains(url);

Since we set 'title' right away in the block, do we care if it is null when entering the block?

Comment 15

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> Comment on attachment 748082 [details] [diff] [review]
> (Part 3) Check for null URL in BrowserToolbar.setTitle
> 
> 
> >-            if (mShowUrl && title != null) {
> >-                title = StringUtils.stripScheme(tab.getURL());
> >-                title = StringUtils.stripCommonSubdomains(title.toString());
> >+            String url = tab.getURL();
> >+            if (mShowUrl && title != null && url != null) {
> >+                url = StringUtils.stripScheme(url);
> >+                title = StringUtils.stripCommonSubdomains(url);
> 
> Since we set 'title' right away in the block, do we care if it is null when
> entering the block?

This check was part of Wes's original code, but yeah, let's thinks about why we're doing this... I think it's more of a "don't override a null title" check more than a "we need this so let's make sure it's not null" check. We do want to avoid this block of code on "about:home" or "about:privatebrowsing", where we set title to null, but we could do this by sticking this logic in an else statement. Apart from one instance, we always call setTitle with tab.getDisplayTitle(), which returns the tab title, or the url if the title is null. If the url is null, we'll catch that with the url check. And in the instance where we don't call pass in tab.getDisplayTitle(), we check to make sure the parameter we pass in isn't null. Come to think of it, there may actually be a bug here where we override what we pass in with that call with the current url.

In conclusion, I think we can get rid of this check, and I think we should look into refactoring this code to make it less confusing. Instead of passing in tab.getDisplayTitle() almost everywhere, it would be nice to just call that if necessary in setTitle, especially since it looks like it always gets overridden in the mShowUrl case.

Comment 16

[:Margaret Leibovic]

Attachment: (Part 4) Refactor BrowserToolbar.setTitle

Maybe I should split this off into another bug, but this patch does what I suggested in my last comment. I haven't been able to test yet because my build was messed up and I had to clobber it.

Comment 17

[:Margaret Leibovic]

Comment on attachment 748463 [details] [diff] [review]
(Part 4) Refactor BrowserToolbar.setTitle

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

::: mobile/android/base/BrowserToolbar.java
@@ +1130,5 @@
>  
>      public void refresh() {
>          Tab tab = Tabs.getInstance().getSelectedTab();
>          if (tab != null) {
>              String url = tab.getURL();

Huh, we should get rid of this line, too, since url isn't used.

Comment 18

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

Comment on attachment 748463 [details] [diff] [review]
(Part 4) Refactor BrowserToolbar.setTitle

Given the nuances you mentioned, like the null title check, I think Wes should take a hard look at this. Seems like we need more comments to make sure we don't break things.

We can always uplift this to Fx23 (Aurora) if we miss the merge.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 748463 [details] [diff] [review]
(Part 4) Refactor BrowserToolbar.setTitle

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

AFAICT, this is fine. Some nits on the comments and naming.

::: mobile/android/base/BrowserToolbar.java
@@ +145,5 @@
>              }
>  
>              @Override
>              public void onPostExecute(Void v) {
> +                setTitle();

If we're going to do this, its probably better to rename this something like updateTitle();

@@ +890,3 @@
>          Tab tab = Tabs.getInstance().getSelectedTab();
> +        // Keep the title unchanged if there's no selected tab, or if the tab is entering reader mode.
> +        if (tab == null || tab.isEnteringReaderMode()) {

Why do we want to stay unchanged if there's no tab at all?

@@ +902,2 @@
>  
> +        // If the pref to show the URL isn't shown, just use the tab's display title.

nit. "If the pref to show the URL isn't shown" doesn't sound right...

@@ +902,4 @@
>  
> +        // If the pref to show the URL isn't shown, just use the tab's display title.
> +        if (!mShowUrl || url == null) {
> +            setTitle(tab.getDisplayTitle());

We could probably just use getTitle() here, but getDisplayTitle is still used for the TabsTray and someday, it might be nice to have it abstracted still, so this seems good.

@@ +908,2 @@
>  
> +        // Strip the scheme and common subdomains from the URL.

nit I'm torn on comments like this where we're basically just repeating exactly what the code says verbatim...

@@ +912,2 @@
>  
> +        // Highlight the domain name if we find one.

not your code, but I'm sad I typed "if we find one".

Comment 20

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #19)

> ::: mobile/android/base/BrowserToolbar.java
> @@ +145,5 @@
> >              }
> >  
> >              @Override
> >              public void onPostExecute(Void v) {
> > +                setTitle();
> 
> If we're going to do this, its probably better to rename this something like
> updateTitle();

Good point, I agree.

> @@ +890,3 @@
> >          Tab tab = Tabs.getInstance().getSelectedTab();
> > +        // Keep the title unchanged if there's no selected tab, or if the tab is entering reader mode.
> > +        if (tab == null || tab.isEnteringReaderMode()) {
> 
> Why do we want to stay unchanged if there's no tab at all?

This is consistent with what the current code does, and I assume it's because we wouldn't know what title to use if there's no tab. Although I guess in that case setting it to null (which shows the placeholder "Enter Search or Address") is probably more of a correct thing to do.
 
> @@ +902,2 @@
> >  
> > +        // If the pref to show the URL isn't shown, just use the tab's display title.
> 
> nit. "If the pref to show the URL isn't shown" doesn't sound right...

Heh, oops, I'll update that to make sense.

> @@ +908,2 @@
> >  
> > +        // Strip the scheme and common subdomains from the URL.
> 
> nit I'm torn on comments like this where we're basically just repeating
> exactly what the code says verbatim...

Yeah, I was just on a comment rampage, so this probably isn't necessary.

> @@ +912,2 @@
> >  
> > +        // Highlight the domain name if we find one.
> 
> not your code, but I'm sad I typed "if we find one".

Now's the chance to fix it! How about "Highlight the base domain." Or no comment, since that comment is pretty redundant with the code :)

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5991b3688770
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f45f5f6cc29

Comment 22

[:Margaret Leibovic]

Comment on attachment 748082 [details] [diff] [review]
(Part 3) Check for null URL in BrowserToolbar.setTitle

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 857165 (and this bug)
User impact if declined: crashes
Testing completed (on m-c, etc.): landing on m-c today
Risk to taking this patch (and alternatives if risky): low-risk, adds a null check 
String or IDL/UUID changes made by this patch: n/a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5991b3688770
https://hg.mozilla.org/mozilla-central/rev/5f45f5f6cc29

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/470da31aeb16

Part 4 didn't apply cleanly, so I held off on that. Based on the approval request, I'm not entirely sure it was supposed to be uplifted anyway.

Comment 25

[:Margaret Leibovic]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/470da31aeb16
> 
> Part 4 didn't apply cleanly, so I held off on that. Based on the approval
> request, I'm not entirely sure it was supposed to be uplifted anyway.

Thanks, Ryan. We don't need to uplift Part 4, it's some refactoring to make things clearer, but isn't necessary to fix the crash.