Closed Bug 962047 Opened 6 years ago Closed 6 years ago

Default search engines are not displayed after turning on search suggestions

Categories

(Firefox for Android :: General, defect)

27 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox26 --- wontfix
firefox27 + wontfix
firefox28 + verified
firefox29 --- verified
fennec 28+ ---

People

(Reporter: cos_flaviu, Assigned: liuche)

References

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

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)
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
29 is unaffected?
tracking-fennec: --- → ?
And just to confirm this is reproducible on release(Firefox 26) ?
(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.
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).
Regression from bug 925427.
Blocks: 925427
Flags: needinfo?(margaret.leibovic)
Keywords: regression
(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.
Flags: needinfo?(margaret.leibovic) → needinfo?(liuche)
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.
Flags: needinfo?(liuche) → needinfo?(flaviu.cos)
Can not reproduce the bug using the build from comment 8. 
Tested on Motorola Atrix (Android 2.3.4).
Flags: needinfo?(flaviu.cos)
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).
Assignee: nobody → liuche
tracking-fennec: ? → 28+
Attached patch Move View changes to UI thread (obsolete) — Splinter Review
Attachment #8364065 - Attachment is obsolete: true
Attachment #8364644 - Flags: review?(margaret.leibovic)
Any ideas for the "world's simplest test" that would catch a regression like this? #worlds #simplest
Attachment #8364664 - Flags: review?(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.
Attachment #8364644 - Flags: review?(margaret.leibovic) → review-
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?
Flags: needinfo?(margaret.leibovic)
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
Flags: needinfo?(flaviu.cos)
Clearing need-info to address review comments.
Flags: needinfo?(flaviu.cos)
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 :)
Attachment #8364664 - Flags: review?(margaret.leibovic) → review+
Attachment #8364669 - Flags: review+
Flags: needinfo?(margaret.leibovic)
Good point, I think that's definitely the better solution!
Attachment #8364644 - Attachment is obsolete: true
Attachment #8364697 - Flags: review?(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.
Attachment #8364697 - Flags: review?(margaret.leibovic) → review+
Target Milestone: --- → Firefox 29
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
Attachment #8364664 - Flags: approval-mozilla-aurora?
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?
https://hg.mozilla.org/mozilla-central/rev/9cf4757cb559
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(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
(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.
Flags: needinfo?(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).
Flags: needinfo?(flaviu.cos)
Attachment #8364664 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in builds:
- Aurora 29 29.0a2 (2014-02-20);
- 28 beta 4;
Device: Motorola Atrix (Andorid 2.3.4).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.