1207872 - Saved search suggestions DB query in SearchEngineRow runs on UI thread

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Sergej Kravcenko]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/600.8.9 (KHTML, like Gecko) Version/8.0.8 Safari/600.8.9

Steps to reproduce:

Saved search suggestion query is done on UI thread which leads to StrictModeDiskReadViolation exception. Maybe query must be moved to background thread?

Comment 1

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

Let me know if you'd be interested in working on this Sergej – I think we should fix it before saved search suggestions ships.

Comment 2

[Allison Naaktgeboren :ally]

I agree, Sergej, do you have any interest in working on this? We'd like to ship the search history/saved search feature in 44.

Comment 3

[Sergej Kravcenko]

I will take a look, but not sure, how long it will take to get familiar with the code.

Comment 4

[Sergej Kravcenko]

After investigation, I've found that suggestions are broken even more. Try to enter single letter and you will see some suggestions from search engine starting with this letter. Hit backspace and enter different letter, you will see old results. Refresh is not triggered on single letter.

Even stranger bug, sometimes mSuggestClient in BrowserSearch.java turns null and search suggestions stops working completely.

This whole thing must be investigated and possible heavily bugfixed/redesigned. Do you want me to do this?

Comment 5

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

(In reply to Sergej Kravcenko from comment #4)
> After investigation, I've found that suggestions are broken even more. Try
> to enter single letter and you will see some suggestions from search engine
> starting with this letter. Hit backspace and enter different letter, you
> will see old results. Refresh is not triggered on single letter.
> 
> Even stranger bug, sometimes mSuggestClient in BrowserSearch.java turns null
> and search suggestions stops working completely.

We recently landed bug 1186683 which fixed an issue where we recreated BrowserSearch each time it was shown (which also took a few seconds to re-retrieve the search engines from Gecko). It seems there are a lot of bugs relating to how we restore BrowserSearch state.

This code is currently only in the Nightly version so we have about six weeks to fix all the issues. See the "Depends on" field for the regressions.

> This whole thing must be investigated and possible heavily
> bugfixed/redesigned. Do you want me to do this?

It can certainly be investigated but I disagree that it needs to be redesigned. You don't need to do an audit yourself though – just file bugs as you see them. :)

Can you file for the two bugs you listed? Have them block 1186683 and CC me please. Thanks!

Comment 6

[Sergej Kravcenko]

Do you want me to submit patch for this bug? I've moved saved suggestion query to BrowserSearch, it now works like default suggestions, using Loader.

Comment 7

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

(In reply to Sergej Kravcenko from comment #6)
> Do you want me to submit patch for this bug? I've moved saved suggestion
> query to BrowserSearch, it now works like default suggestions, using Loader.

Sure – no need to ask in the future (unless you're asking to be assigned)! Just post a patch and flag a reviewer.

Comment 8

[Allison Naaktgeboren :ally]

As mcomella noted in comment 1, we decided that this blocks ship, and so needs to be done very soon. Sergej, the patch you mention in comment 6, do you think you can have it ready for review in the next couple days?

Comment 9

[Sergej Kravcenko]

I'll try to upload it tomorrow.

Comment 10

[Sergej Kravcenko]

Attachment: saved suggestion on background thread

Here you go. Not sure who should review this.

Comment 11

[Allison Naaktgeboren :ally]

I'll take it. mcomella's review queue is on fire.

Comment 12

[Allison Naaktgeboren :ally]

Comment on attachment 8668221 [details] [diff] [review]
saved suggestion on background thread

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

Your logic looks solid. There's some clean up that needs to happen.

When running the code, I do notice that suggestions do not show up in as timely a fashion, so if I do 'dog' then 'dog leashes' I notice 'dog' won't be present until I wait a second or two. I don't think it can be helped, as it's the price of using an async loader and I don't think most of the user base will even notice.

::: mobile/android/base/home/BrowserSearch.java
@@ +140,5 @@
>      // Do not mutate this list.
>      // Access to this member must only occur from the UI thread.
>      private List<SearchEngine> mSearchEngines;
>  
> +    // Saved suggestions

This is also referred to as search history suggestions; we're not terribly consistent. Please change the comment to

// Search history suggestions

and the variable to mSavedSearchSuggestions

@@ +154,5 @@
>      // Callbacks used for the search loader
>      private CursorLoaderCallbacks mCursorLoaderCallbacks;
>  
>      // Callbacks used for the search suggestion loader
> +    private SearchSuggestionLoaderCallbacks mSearchSuggestionLoaderCallbacks;

In general, I would like the search engine suggestion variables to have 'engine' in their name and the saved search/search history to have either 'saved' or 'history' in their name, and abstract, shared, or general code to just be SearchSuggestion*

so

mSearchEngineSuggestionLoaderCallbacks
mSearchHistorySuggestionLoaderCallbacks or mSavedSearchSuggestionLoaderCallbacks

@@ +273,5 @@
>      @Override
>      public void onDestroyView() {
>          super.onDestroyView();
>  
> +        EventDispatcher.getInstance().unregisterGeckoThreadListener(this, "SearchEngines:Data");

nit: please remove the whitespace change.

@@ +347,5 @@
>              }
>          });
>  
>          registerForContextMenu(mList);
> +        EventDispatcher.getInstance().registerGeckoThreadListener(this, "SearchEngines:Data");

nit: please remove the whitespace change.

@@ +535,4 @@
>  
> +        // Start saved suggestion query ony in nightly. Bug 1201325
> +        if (AppConstants.NIGHTLY_BUILD) {
> +            // Saved sugegstions

nit: spelling. "suggestions"

@@ +730,5 @@
>          shrinkAnimation.setAnimationListener(new Animation.AnimationListener() {
>              @Override
>              public void onAnimationStart(Animation a) {
>                  // Increase the height of the view so a gap isn't shown during animation
> +                mView.getLayoutParams().height = mView.getHeight() + mSuggestionsOptInPrompt.getHeight();

nit: please remove the unnecessary whitespace change.

@@ +735,5 @@
>                  mView.requestLayout();
>              }
>  
>              @Override
> +            public void onAnimationRepeat(Animation a) {

nit: please remove the unnecessary whitespace change.

@@ +810,5 @@
>              }
>          }
>      }
>  
> +    abstract private static class SuggestionAsyncLoader extends AsyncTaskLoader<ArrayList<String>> {

Good, good.

@@ +853,5 @@
>              mSuggestions = null;
>          }
>      }
>  
> +    private static class SearchSuggestionAsyncLoader extends SuggestionAsyncLoader {

how about SearchEngineSuggestionAsyncLoader ?

@@ +881,5 @@
> +            String actualQuery = BrowserContract.SearchHistory.QUERY + " LIKE ?";
> +            String[] queryArgs = new String[] { '%' + mSearchTerm + '%' };
> +
> +            final int maxSavedSuggestions = getContext().getResources().getInteger(R.integer.max_saved_suggestions);
> +

nit: The whitespace in this block is slightly inconsistent.Either put another newline between sortOrderAndLimit and result or remove this one between maxSavedSearchSuggestions and sortOrderAndLimit

@@ +983,5 @@
>                  row.setOnEditSuggestionListener(mEditSuggestionListener);
>                  row.setSearchTerm(mSearchTerm);
>  
>                  final SearchEngine engine = mSearchEngines.get(position);
> +                final boolean animate = (mAnimateSuggestions && (engine.hasSuggestions() || !mSavedSuggestions.isEmpty()));

please move (engine.hasSuggestions() || !mSavedSuggestions.isEmpty()) to its own variable. 

final boolean suggestionsPresent = engine.hasSuggestions() || !mSavedSuggestions.isEmpty();

::: mobile/android/base/home/SearchEngineRow.java
@@ +244,5 @@
>       * @param suggestionCounter global index of where to start adding suggestion "buttons" in the search engine row
>       * @param animate whether or not to animate suggestions for visual polish
>       * @param recycledSuggestionCount How many suggestion "button" views we could recycle from previous calls
>       */
> +    private void updateFromSavedSearches(List<String> savedSuggestions, boolean animate, int suggestionCounter, int recycledSuggestionCount) {

Why List and not ArrayList or Iterable?

We could either couple it to BrowserSearch's ArrayList or go fully generalized with Iterable<String>. I favor going with Iterable.

List seems like an awkward middle.

@@ +309,5 @@
>       * Even if both suggestion types are disabled, we need to update the search engine, its image, and the content description.
>       *
>       * @param searchSuggestionsEnabled whether or not suggestions from the default search engine are enabled
>       * @param searchEngine the search engine to use throughout the SearchEngineRow class
> +     * @param savedSuggestions suggestions stored in local database

Well, not quite. the suggestions are now provided by BrowserSearch and SearchEngineRow no longer has any awareness of where they come from. 

How about 
 * @param savedSuggestions suggestions from previous searches

@@ +317,2 @@
>          mSearchEngine = searchEngine;
> +

nit: please remove the unnecessary whitespace change.

@@ +319,3 @@
>          // Set the search engine icon (e.g., Google) for the row.
>          mIconView.updateAndScaleImage(mSearchEngine.getIcon(), mSearchEngine.getEngineIdentifier());
> +

nit: please remove the unnecessary whitespace change.

Comment 13

[Sergej Kravcenko]

Attachment: search history on background thread

> When running the code, I do notice that suggestions do not show up in as
> timely a fashion, so if I do 'dog' then 'dog leashes' I notice 'dog' won't
> be present until I wait a second or two. I don't think it can be helped, as
> it's the price of using an async loader and I don't think most of the user
> base will even notice.

It's simple network delay, nothing is related to loaders. The only way to handle this, show some sort of progress UI.

> Why List and not ArrayList or Utterable?
.isEmpty()

Comment 14

[Allison Naaktgeboren :ally]

Comment on attachment 8669252 [details] [diff] [review]
search history on background thread

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

Looks dandy. Thanks so much! We really appreciate your work.

Ordinarily we use checkin-needed at this point, however since I'd like this in asap to bake and to unblock Bug 1201325, I'm going to land it for you.

::: mobile/android/base/home/SearchEngineRow.java
@@ +239,5 @@
>  
>      /**
>       * Displays search suggestions from previous searches.
>       *
> +     * @param savedSuggestions The List to iterate over for saved search suggestion to display

typo, 'suggestion' should be 'suggestions'. I'll fix this for you.

Comment 15

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/1831b1955278d055e7b6f3435d5236c0a8adf036
Bug 1207872 - Saved search suggestions DB query in SearchEngineRow runs on UI thread.r=ally

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1831b1955278