962047 - Default search engines are not displayed after turning on search suggestions

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

Attachment: The file is a screen capture

Environment: 
Device: Motorola Atrix (Android 2.3.4);
Build: 27 Beta 8.

Steps to reproduce:
1. Launch fennec;
2. Type a word in the url bar;
3. Turn on search suggestion;
4. Tap on any suggestion;
5. Go to settings -> Customize -> Search settings.

Expected result:
Show search suggestions is enabled; All the default search engines are present in the list.

Actual result:
Show search suggestions in not enabled, and only google search engine is present in the list.


Note:
Please check the attached screen shot.

Logs:
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): handleGeckoMessage throws android.view.ViewRoot$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): android.view.ViewRoot$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.view.ViewRoot.checkThread(ViewRoot.java:3036)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.view.ViewRoot.requestLayout(ViewRoot.java:634)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.view.View.requestLayout(View.java:8313)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.view.View.requestLayout(View.java:8313)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.view.View.requestLayout(View.java:8313)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.view.View.requestLayout(View.java:8313)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.widget.AbsListView.requestLayout(AbsListView.java:1128)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.widget.AdapterView$AdapterDataSetObserver.onChanged(AdapterView.java:796)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.database.DataSetObservable.notifyChanged(DataSetObservable.java:31)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.widget.BaseAdapter.notifyDataSetChanged(BaseAdapter.java:50)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.preference.PreferenceGroupAdapter.onPreferenceChange(PreferenceGroupAdapter.java:238)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.preference.Preference.notifyChanged(Preference.java:949)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at android.preference.Preference.setSummary(Preference.java:560)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.preferences.SearchEnginePreference.setIsDefaultEngine(SearchEnginePreference.java:152)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.preferences.SearchPreferenceCategory.handleMessage(SearchPreferenceCategory.java:92)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:96)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:58)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2304)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:356)
01-21 14:28:00.938: E/GeckoEventDispatcher(19608): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:176)

Comment 1

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

By default, Gecko messages are passed to Java on the Gecko thread. You can't do UI stuff on the Gecko thread. I assume the code here needs to be moved to the UI thread:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/SearchPreferenceCategory.java#57

Like this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/SearchEnginePreference.java#215

Comment 2

[Aaron Train [:aaronmt]]

29 is unaffected?

Comment 3

[bhavana bajaj [:bajaj]]

And just to confirm this is reproducible on release(Firefox 26) ?

Comment 4

[Flaviu Cos]

(In reply to bhavana bajaj [:bajaj] from comment #3)
> And just to confirm this is reproducible on release(Firefox 26) ?
Yes, I can reproduce the bug on release Firefox 26 with the following devices:
- Samsung Galaxy S (Android 2.3.4);
- HTC Desire (Android 2.2);
- Motorola Atrix (Android 2.3.4).
Could not reproduce on any devices with android 4.x
It may be specific to gingerbread devices.

Comment 5

[Flaviu Cos]

Good build 10.16.2013;
Bad build 10.17.2013;
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9f63bbc00527&tochange=423b9c30c73d

Tested on Motorola Atrix (Android 2.3.4).

Comment 6

[Aaron Train [:aaronmt]]

Regression from bug 925427.

Comment 7

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #6)
> Regression from bug 925427.

If this is the case, it's likely that bug just exposed an underlying issue originally introduced with the search settings UI.

We probably need to do what mfinkle suggested and just make sure we do the UI updating on the main thread.

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Patch: Move View changes to UI thread

That seems like a correct assumption, from the logs. Here's a patch that runs the setIsDefault on the UI thread.

An apk is here: http://people.mozilla.org/~liuche/bug-962047/fennec-29.0a1.en-US.android-arm.apk

Flaviu, can you test this on your device? I don't have one here that I can repro it on.

Comment 9

[Flaviu Cos]

Can not reproduce the bug using the build from comment 8. 
Tested on Motorola Atrix (Android 2.3.4).

Comment 10

[bhavana bajaj [:bajaj]]

Tracking it for both Fx27/Fx28 since this is a recent fallout but uplift on beta will happen if this is absoultely low risk and we are sure there will be no fallouts as we only have our final beta left which goes to build monday.
If we have ways to mitigate risk by getting more testing we can probably think of beta uplift else this may be a good canditate for aurora(Fx28).

Comment 11

[Chenxia Liu [:liuche]]

Attachment: Move View changes to UI thread

Comment 12

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

Any ideas for the "world's simplest test" that would catch a regression like this? #worlds #simplest

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Aurora patch: move View changes to UI thread

Comment 14

[:Margaret Leibovic]

Comment on attachment 8364644 [details] [diff] [review]
Move View changes to UI thread

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

::: mobile/android/base/preferences/CustomListPreference.java
@@ +83,5 @@
>       * Set whether this object's UI should display this as the default item. To ensure proper ordering,
>       * this method should only be called after this Preference is added to the PreferenceCategory.
>       * @param isDefault Flag indicating if this represents the default list item.
>       */
>      public void setIsDefault(boolean isDefault) {

This method is generic enough that it wouldn't necessarily always be called from the Gecko thread, such as in PanelsPreferenceCategory, right?

I think a better fix would be to make sure we call this method on the UI thread where we're actually handling the gecko message here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/SearchPreferenceCategory.java#103

And then add a comment to the documentation for this method mentioning it should be called from the UI thread.

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Beta patch: move View changes to UI thread

Margaret, I think this is low risk enough to nom for beta uplift because it's moving two existing calls into a UI thread, but beta becomes release really soon - what do you think?

Comment 16

[Chenxia Liu [:liuche]]

Flaviu, here's a beta build with the fix - it will install as "Fennec liuche". Can you verify that it works, setting default works, STR verify the problem fixed? I think this is really low risk, but let's get some more testing on this.

http://people.mozilla.org/~liuche/bug-962047/beta.apk

Comment 17

[Chenxia Liu [:liuche]]

Clearing need-info to address review comments.

Comment 18

[:Margaret Leibovic]

Comment on attachment 8364664 [details] [diff] [review]
Aurora patch: move View changes to UI thread

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

This is fine for aurora/beta, since we only go through this code path for search engine preferences. However, we can do better for Nightly, so my r- there still stands :)

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Nightly patch: move setDefault to UI thread + documentation

Good point, I think that's definitely the better solution!

Comment 20

[:Margaret Leibovic]

Comment on attachment 8364697 [details] [diff] [review]
Nightly patch: move setDefault to UI thread + documentation

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

Nice.

Comment 21

[Chenxia Liu [:liuche]]

Fx-team: https://hg.mozilla.org/integration/fx-team/rev/9cf4757cb559

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8364664 [details] [diff] [review]
Aurora patch: move View changes to UI thread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Present in original search settings, exposed by bug 925427
User impact if declined: Users of 2.3 devices may get into a state where their search engine settings don't work
Testing completed (on m-c, etc.): tested on local builds
Risk to taking this patch (and alternatives if risky): very low, moving code into a different thread.
String or IDL/UUID changes made by this patch: none

Comment 23

[Chenxia Liu [:liuche]]

Flaviu: sorry for the flag toggling!

Can you tell me a little bit more about what happens when the search settings fail?

Specifically:
Does the problem go away (correct search engines show up, menu is enabled) if you leave settings and then re-enter them? If not, does a restart of Firefox fix the problem?

Also, would you take the build in comment #16 for a run, see how it works and if there are any problems with it?

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cf4757cb559

Comment 25

[Flaviu Cos]

(In reply to Chenxia Liu [:liuche] from comment #23)
> Flaviu: sorry for the flag toggling!
> 
> Can you tell me a little bit more about what happens when the search
> settings fail?

Please check the video: http://www.youtube.com/watch?v=Qjs_jANPiP0

Comment 26

[Chenxia Liu [:liuche]]

(In reply to flaviu.cos from comment #25)
> (In reply to Chenxia Liu [:liuche] from comment #23)
> > Flaviu: sorry for the flag toggling!
> > 
> > Can you tell me a little bit more about what happens when the search
> > settings fail?
> 
> Please check the video: http://www.youtube.com/watch?v=Qjs_jANPiP0

Flaviu, do you think you could do some testing of the build in comment #16? I will also try to find a device that I can repro on here.

Comment 27

[Flaviu Cos]

(In reply to Chenxia Liu [:liuche] from comment #26)
> Flaviu, do you think you could do some testing of the build in comment #16?
> I will also try to find a device that I can repro on here.
I can not reproduce the bug using the build from comment 16.
Tested on Motorola Atrix (Android 2.3.4).

Comment 28

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cff3be00c86d

Comment 29

[Flaviu Cos]

Verified as fixed in builds:
- Aurora 29 29.0a2 (2014-02-20);
- 28 beta 4;
Device: Motorola Atrix (Andorid 2.3.4).