Closed Bug 868342 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect, critical)

23 Branch
ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 24
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 --- fixed

People

(Reporter: scoobidiver, Assigned: Margaret)

References

Details

(Keywords: crash, regression, topcrash, Whiteboard: [native-crash])

Crash Data

Attachments

(4 files)

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
Whiteboard: [native-crash] → [native-crash][startupcrash]
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).
(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.
Assignee: nobody → margaret.leibovic
Attached patch patchSplinter Review
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.
Attachment #746577 - Flags: review?(wjohnston)
Not sure if you're going to like this, but this makes the logic clearer to me.
Attachment #746581 - Flags: review?(wjohnston)
Comment on attachment 746577 [details] [diff] [review]
patch

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

This function is getting a bit hairy...
Attachment #746577 - Flags: review?(wjohnston) → review+
Comment on attachment 746581 [details] [diff] [review]
(Part 2) Get rid of displayTitle variable

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

Nice, and thanks!
Attachment #746581 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/1c8df5e637a2
https://hg.mozilla.org/mozilla-central/rev/a23eea95bf64
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
The patch makes crashes worth with currently 26 crashes in 23.0a1/20130509.
Status: RESOLVED → REOPENED
tracking-fennec: --- → ?
Keywords: topcrash
Resolution: FIXED → ---
Whiteboard: [native-crash][startupcrash] → [native-crash]
s/worth/worse in comment 9.
:( Given the new crash stacks, StringUtils.stripScheme(tab.getURL()) must be returning null, which will if tab.getURL() returns null.
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.
Attachment #748082 - Flags: review?(mark.finkle)
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?
Attachment #748082 - Flags: review?(mark.finkle) → review+
(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.
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.
Attachment #748463 - Flags: review?(mark.finkle)
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 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.
Attachment #748463 - Flags: review?(mark.finkle) → review?(wjohnston)
Attachment #748082 - Flags: review?(wjohnston) → review+
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".
Attachment #748463 - Flags: review?(wjohnston) → review+
(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 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
Attachment #748082 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5991b3688770
https://hg.mozilla.org/mozilla-central/rev/5f45f5f6cc29
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 23 → Firefox 24
Attachment #748082 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
(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.
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.