1232608 - After transferring bookmarks and history, Fennec crashes when input something in the address bar

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Bingqing Li]

Environments:
----------------------------
Fennec 43 build2 (both regular fennec build and Mozilla China build)


Steps to reproduce:
----------------------------
1. Install Fennec 43 build2 for the first time

2. Launch Fennec for the first time

3. Transfer bookmarks and history

4. Input url/keywords in the address bar
   --> Fennec crashes


Crash report:
-----------------------------------------------
https://crash-stats.mozilla.com/report/index/fd08e13d-eb32-42a8-b4c6-eeaf42151215

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Device for this report is a HUAWEI M2-801W does not seem to be device specific. Top devices are

samsung	GT-I9505
LENOVO	Lenovo TAB 2 A8-50L
OnePlus	A0001
HTC	HTC One
LENOVO	Lenovo P70-A

Comment 2

[Richard Newman [:rnewman]]

Do we have a log?

This implies that there's a bookmark with a NULL URL. The importer shouldn't import those, and we shouldn't query for them, so there are at least two bugs here…

Comment 3

[Bingqing Li]

Attachment: logcat

(In reply to Richard Newman [:rnewman] from comment #2)
> Do we have a log?
> 
> This implies that there's a bookmark with a NULL URL. The importer shouldn't
> import those, and we shouldn't query for them, so there are at least two
> bugs here…

log attached.

Comment 4

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

I see a lot of java.io.FileNotFoundException exceptions, thrown from the NavigatorPanel.

Comment 5

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> I see a lot of java.io.FileNotFoundException exceptions, thrown from the
> NavigatorPanel.

We don't have a NavigatorPanel in our codebase, so this would be an issue related to the China fork.

Comment 6

[Richard Newman [:rnewman]]

Are you sure that log covers the crash you reported?

Still, I think my diagnosis in Comment 2 is correct.

Comment 7

[Bingqing Li]

Attachment: logcat-regular-fennec

(In reply to :Margaret Leibovic from comment #5)
> (In reply to Mark Finkle (:mfinkle) from comment #4)
> > I see a lot of java.io.FileNotFoundException exceptions, thrown from the
> > NavigatorPanel.
> 
> We don't have a NavigatorPanel in our codebase, so this would be an issue
> related to the China fork.

Here is the log from a regular fennec build.

Comment 8

[Bingqing Li]

Attachment: logcat-regular-fennec-build-mi

I tried with a regular fennec build on another device:
MI PAD ( MI is a famous manufacturer in China, whose products are very popular)
Android 4.4.4
Fennec 43


Here are crash reports:
https://crash-stats.mozilla.com/report/index/422b805f-9fe6-4418-9515-d8a2e2151218

https://crash-stats.mozilla.com/report/index/b74e35cf-18dd-4e9a-906f-18f9e2151218

And log is attached.

Comment 9

[:Margaret Leibovic]

Chenxia, can you take a look at this? Our crash rate is bad, every little bit can help.

Comment 10

[:Margaret Leibovic]

We're going to back out the import panel, so that should help reduce the incidence of this issue.

It may still happen if you import from settings, but it seems like that scenario is less likely to cause a race like this.

Comment 11

[Richard Newman [:rnewman]]

That'll avoid new cases, but Fennec will crash 100% of the time for users who already imported bad data, so I don't think this can be deprioritized.

Comment 12

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #11)
> That'll avoid new cases, but Fennec will crash 100% of the time for users
> who already imported bad data, so I don't think this can be deprioritized.

Okay, then we should figure out what to do here.

Looking at UI telemetry, people do use the import feature in settings quite a bit, although those numbers might be skewed towards power users on release who are turning on telemetry.

Chenxia, do you have one of these devices in the SF office where you can reproduce?

Comment 13

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1232608 - After transferring bookmarks and history, Fennec crashes when input something in the address bar. r=rnewman

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

Comment 14

[Chenxia Liu [:liuche]]

We don't have any of these devices in SF, but these should address the two points rnewman made in comment #2.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8714561 [details]
MozReview Request: Bug 1232608 - After transferring bookmarks and history, Fennec crashes when input something in the address bar. r=rnewman

https://reviewboard.mozilla.org/r/33147/#review29909

::: mobile/android/base/java/org/mozilla/gecko/home/SearchLoader.java:37
(Diff revision 1)
> -            final String searchTerm = args.getString(KEY_SEARCH_TERM);
> +            final String searchTerm = args.getString(KEY_SEARCH_TERM, "");

I'm not sure how this is related. I mean, yes, it's good to be safe… can you explain?

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:76
(Diff revision 1)
> -                               LegacyBrowserProvider.BookmarkColumns.BOOKMARK + " = 1");
> +                               LegacyBrowserProvider.BookmarkColumns.BOOKMARK + " = 1 AND +" +

I think you have an extra '+' inside the quotes. Did you test this?

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:77
(Diff revision 1)
> +                               LegacyBrowserProvider.BookmarkColumns.URL + " IS NOT NULL");

I assume this doesn't import folders… if it does, would that cause this bug?

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8714561 [details]
MozReview Request: Bug 1232608 - After transferring bookmarks and history, Fennec crashes when input something in the address bar. r=rnewman

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

Comment 17

[Chenxia Liu [:liuche]]

> ::: mobile/android/base/java/org/mozilla/gecko/home/SearchLoader.java:37
> (Diff revision 1)
> > -            final String searchTerm = args.getString(KEY_SEARCH_TERM);
> > +            final String searchTerm = args.getString(KEY_SEARCH_TERM, "");
> 
> I'm not sure how this is related. I mean, yes, it's good to be safe… can you
> explain?
> 

searchTerm gets passed into a cursor query as the selection - if it's null, doesn't it return all columns? I'd assume that this is why we're getting urls that are malformed.

> :::
> mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:76
> (Diff revision 1)
> > -                               LegacyBrowserProvider.BookmarkColumns.BOOKMARK + " = 1");
> > +                               LegacyBrowserProvider.BookmarkColumns.BOOKMARK + " = 1 AND +" +
> 
> I think you have an extra '+' inside the quotes. Did you test this?
> 

Odd, it ran just fine when I tested it. Removed.

> :::
> mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:77
> (Diff revision 1)
> > +                               LegacyBrowserProvider.BookmarkColumns.URL + " IS NOT NULL");
> 
> I assume this doesn't import folders… if it does, would that cause this bug?

Changed this to Combined.URL.

Comment 18

[Chenxia Liu [:liuche]]

To be specific, the crash log is in BrowserSearch.

E/GeckoCrashHandler( 8561): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E/GeckoCrashHandler( 8561): java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.String.startsWith(java.lang.String)' on a null object reference
E/GeckoCrashHandler( 8561): 	at org.mozilla.gecko.home.BrowserSearch.access$1700(BrowserSearch.java:66)
E/GeckoCrashHandler( 8561): 	at org.mozilla.gecko.home.BrowserSearch$CursorLoaderCallbacks.onLoadFinished(BrowserSearch.java:928)

This is kind of confusing because at this specific point it's not loading anything, but maybe the loader already has some bad urls, because it called loadCursor with a "null" searchTerm, as would happen here, and returned everything because no selection was actually specified:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/SearchLoader.java#101

Does that seem reasonable, or am I barking up the wrong tree?

Comment 19

[Chenxia Liu [:liuche]]

Hmm, though on more detailed inspection, filter() doesn't actually take a selection, and searchTerm is used as a constraint which does get null-checked.

I'll take that part out then, because that's not causing us to query for anything that has null urls.

Comment 20

[Richard Newman [:rnewman]]

(In reply to Chenxia Liu [:liuche] from comment #17)

> searchTerm gets passed into a cursor query as the selection - if it's null,
> doesn't it return all columns?

That's the behavior for projection = null, not selection = null.

> I'd assume that this is why we're getting
> urls that are malformed.

Based on my reading of BrowserSearch.java, mSearchTerm -- with which we initialize the loader -- should never be null.

And even if it is, when it reaches LocalBrowserDB.filter (and filterAllSites) via SearchCursorLoader, it's correctly handled with TextUtils.isEmpty.

I think you're thinking "if this is null, is it going to return all rows"? Well, that's what history usually does -- it's one of the two cases handled in LocalBrowserDB.filterAllSites.

I don't think the issue here is to do with querying at all.


> > I assume this doesn't import folders… if it does, would that cause this bug?
> 
> Changed this to Combined.URL.

I wasn't implying something needed to be changed; I was questioning whether this might be the root cause. That is: if some vendor's importer *did* include folders here, with null URLs…


(In reply to Chenxia Liu [:liuche] from comment #18)

> org.mozilla.gecko.home.BrowserSearch$CursorLoaderCallbacks.
> onLoadFinished(BrowserSearch.java:928)
> 
> This is kind of confusing because at this specific point it's not loading
> anything, but maybe the loader already has some bad urls, because it called
> loadCursor with a "null" searchTerm, as would happen here, and returned
> everything because no selection was actually specified:

I think the resultant cursor contains rows that have null URLs, and we don't expect that.

We grab the URL:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#533

never test it for null-ness, then call .startsWith:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#542

That code -- handleAutocomplete -- is called in onLoadFinished:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#1130

so it seems like it agrees with that log.


If so, your fix should be:

* Provider: Don't ever include in filter result sets bookmarks or history items with null URLs.
* Importer: Don't import bookmarks or history items with null URLs.
* BrowserSearch: Check for null before dereferencing data retrieved from a cursor. (This is belt-and-braces.)
* DB: Do a DB migration to mark as deleted bookmarks with null URLs.

Comment 21

[:Margaret Leibovic]

What's going on with this bug? It hasn't been touched in a while, but it's tracking 47.

Comment 22

[Chenxia Liu [:liuche]]

This bug was made tracking because it was part of first run, but once we removed it from first run, it's still possible that this happens but it was less likely, so I de-proritized it in my list.

Clearing tracking and renom-ing, but I can also look at this right now.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.