Closed Bug 975837 Opened 10 years ago Closed 10 years ago

crash in java.lang.IllegalArgumentException: Cannot handle null URLs in enterEditingMode at org.mozilla.gecko.BrowserApp.enterEditingMode(BrowserApp.java)

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

30 Branch
All
Android
defect
Not set
critical

Tracking

(firefox34 wontfix, firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
Firefox 36
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: rnewman, Assigned: mcomella)

References

Details

(Keywords: crash, Whiteboard: [native-crash][startupcrash])

Crash Data

Attachments

(1 file)

Filing a new bug rather than reopening Bug 911295.

This is crashing on 30, infrequently:

https://crash-stats.mozilla.com/report/index/db7357a8-505b-47b2-b682-963152140218
This is becoming a much more frequent crash. It's #19 on Fx33, but doesn't show up on Fx34. Did we change the code?
Assignee: nobody → michael.l.comella
FYI: I've done changes to toolbar in Fx34 as part of the toolbar refresh (bug 1052004). Can't remember if I changed the editing mode code though...
Mark, did this become a more frequent crash in 33 in general, or after a certain point in 33?

---

I don't remember anything, nor does anything pop out by skimming BrowserApp's blame, nor does the code for enterEditingMode (and the call points) look dramatically different in release (33) or beta (34), according to MXR. The code change must either run deeper, or the two channels have varying enough usage patterns.

I did notice an issue in all channels, however, where tab.getURL() may return null [1][2]. Specifically:

> // may be null if user-entered query hasn't yet been resolved to a URI

Without looking into what this means further, this seems like it could be possible if a user enters a URL and then immediately clicks the url bar, or enters the browser from an external app and clicks the url bar (due to gecko CPU usage and/or startup time, this could make the window for error larger). Both of these issues are more likely to affect slower devices, which I would expect would be a larger audience in release than in beta.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=95751725d539#1940
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java?rev=b83cdceb5b0c#158
Status: NEW → ASSIGNED
Flags: needinfo?(mark.finkle)
Note: A band-aid fix would be to use `""` instead of `tab.getURL()` if the latter is null.

We only use the url to set the text of the url bar EditText [1] and getting an empty url bar is probably better than crashing.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java?rev=53d7570e20f2#792
Here is a link to the set of crashes:
https://crash-stats.mozilla.com/report/list?product=FennecAndroid&signature=java.lang.IllegalArgumentException%3A+Cannot+handle+null+URLs+in+enterEditingMode+at+org.mozilla.gecko.BrowserApp.enterEditingMode%28BrowserApp.java%29

I looked at a few of the crashes, and they seems like crashes that happen before Gecko is up and running. Based on the stack, it looks like people are tapping the URLBar, a tab (maybe a stub) is selected, but has a null URL, and we send that to enterEditingMode.

I few thoughts:
1. I'm fine with throwing exceptions for exceptional situations, but I question whether we should let the application crash on any exceptional situation. Somewhere, the code should be dealing with the exception.
2. BrowserApp.enterEditingMode() assume it can fallback to an empty string. I think it should check the URL before it passes it to BrowserApp.enterEditingMode(String) and fallback.
3. If we assume we can fallback to an empty string, how exceptional is the IllegalArgumentException in BrowserApp.enterEditingMode(String) ?
Flags: needinfo?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #5)
> 3. If we assume we can fallback to an empty string, how exceptional is the
> IllegalArgumentException in BrowserApp.enterEditingMode(String) ?

Doesn't seem very exceptional at all.

This patch falls back to the empty string when receiving null. Lucas has the
review because he may have insight from originally adding the exception in
bug 905088.
Attachment #8526332 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8526332 [details] [diff] [review]
Fallback to the empty string when receiving null urls in enterEditingMode

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

Fair enough.
Attachment #8526332 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Michael Comella (:mcomella) from comment #6)
> Created attachment 8526332 [details] [diff] [review]
> Fallback to the empty string when receiving null urls in enterEditingMode
> 
> (In reply to Mark Finkle (:mfinkle) from comment #5)
> > 3. If we assume we can fallback to an empty string, how exceptional is the
> > IllegalArgumentException in BrowserApp.enterEditingMode(String) ?
> 
> Doesn't seem very exceptional at all.
> 
> This patch falls back to the empty string when receiving null. Lucas has the
> review because he may have insight from originally adding the exception in
> bug 905088.

IIRC, I only added it to catch potential bugs in toolbar. Nothing specific.
Comment on attachment 8526332 [details] [diff] [review]
Fallback to the empty string when receiving null urls in enterEditingMode

Approval Request Comment
[Feature/regressing bug #]: Unknown.
[User impact if declined]:
  Users who are waiting for gecko to load and click the toolbar may send to null value to our method which will throw an exception - this patch prevents this crash.

[Describe test coverage new/current, TBPL]: None
[Risks and why]: 
  Not throwing on this exception may cover up other bugs where null is given to this method, but at the very least the browser does not crash

[String/UUID change made/needed]: None
Attachment #8526332 - Flags: approval-mozilla-aurora?
re comment 10: too scary to uplift to beta, I think.

---

Just want to note that I'm feeling slightly conflicted over some alternatives of this patch (but not enough to change what we did). We could alternatively:
  * Do what we're doing but also log the fallback
  * Fallback for the null returned from tab.getURL(), but continue to throw in enterEditingMode(String)
https://hg.mozilla.org/mozilla-central/rev/50456b05b041
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Attachment #8526332 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.