817586 - Top Sites pages cannot be tapped after going through History

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

Firefox 20.0a1 (2012-12-03)
Device: Galaxy S2
OS: Android 4.0.3

Steps to reproduce:
1. Open a Private Tab
2. Tap on URL Bar for Awesomescreen
3. Tap on History tab
4. Tap on Top Sites tab and open any page from the list

Expected result:
The selected page opens correctly in privacy mode.

Actual result:
The Top Sites list is scrollable but no page can be selected (opened)

Comment 1

[Sriram Ramasubramanian [:sriram]]

This happens on normal browsing mode too. This is probably a regression from ViewPager.

Comment 2

[Alex Keybl [:akeybl]]

Sriram - any progress here?

Comment 3

[Wesley Johnston (:wesj)]

I have a WIP for this.

Comment 4

[Wesley Johnston (:wesj)]

I think the issue is we try to cache both the linearlayout view and the listview. I think at some point the linearlayout is destroyed by some android smartness underneath us, but we're still holding the listview.

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> I have a WIP for this.

Any updates? We're coming up on Beta 3, so it'd be good to get resolution soon.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Potential fix

I suspect the problem is something like this. Somehow mView is being deleted but the mListView is not?

I can't reproduce the problem anymore to test.... I wonder if an SDK upgrade fixed something?

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #6)
> Created attachment 704046 [details] [diff] [review]
> Potential fix
> 
> I suspect the problem is something like this. Somehow mView is being deleted
> but the mListView is not?
> 
> I can't reproduce the problem anymore to test.... I wonder if an SDK upgrade
> fixed something?

I think I need more background in order to review this patch. Where is mView removed from?

Comment 8

[Aaron Train [:aaronmt]]

I can reproduce on Beta but the item in Top-Sites is selected on second tap, not first tab.

Comment 9

[Alex Keybl [:akeybl]]

(In reply to Aaron Train [:aaronmt] from comment #8)
> I can reproduce on Beta but the item in Top-Sites is selected on second tap,
> not first tab.

So the full STR for this FF19 issue is:

1. Open a New Tab
2. Tap on URL Bar for Awesomescreen
3. Tap on History tab
4. Tap on Top Sites tab and open any page from the list

Expected result:
The site is opened

Actual result:
The site is opened on second tap

Given that, this isn't a very critical regression. No longer tracking for FF19 since we're pretty close to release, but if the fix is low risk, we'd take an uplift.

Comment 10

[Alex Keybl [:akeybl]]

Mark - heard wesj is out (congrats to him :). Can you help answer Lucas's question in comment 7?

Comment 11

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

Sriram - Can you still make this happen? If so, does Wes' patch fix it for you?

Comment 12

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #7)
> (In reply to Wesley Johnston (:wesj) from comment #6)
> > Created attachment 704046 [details] [diff] [review]
> > Potential fix
> > 
> > I suspect the problem is something like this. Somehow mView is being deleted
> > but the mListView is not?
> > 
> > I can't reproduce the problem anymore to test.... I wonder if an SDK upgrade
> > fixed something?
> 
> I think I need more background in order to review this patch. Where is mView
> removed from?

I have no idea. This is a bit of a shot in the dark guess when I could reproduce and was trying to track this down. Since I can't reproduce this anymore, its hard to dig around and figure out what's going on.

But if mView is deleted somehow, we SHOULD do this regardless. Its a correctness patch.

Comment 13

[Wesley Johnston (:wesj)]

Checked again today. Nightly sometimes ignore my first tap on the AllPages item. I'm looking into that.

Beta "just works" for me. I'll remove the affected 19 flag, but I'd appreciate someone from QA verifying.

Comment 14

[Aaron Train [:aaronmt]]

19 is unaffected; wfm

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 704046 [details] [diff] [review]
Potential fix

Removing review flag for now.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Aaron Train [:aaronmt] from comment #14)
> 19 is unaffected; wfm

Is this still affecting 20?  We're moving 20 to beta channel next week so let's get this uplifted while it's still on Aurora and have our first FF 20 beta 1 not ship with this regression.

Comment 17

[Aaron Train [:aaronmt]]

I'm having trouble reproducing this at all anymore on mozilla-central and mozilla-aurora (both 02/11 builds). Has an unrelated fix corrected this in another bug?

Comment 18

[Andreea Pod]

(In reply to Aaron Train [:aaronmt] from comment #17)
> I'm having trouble reproducing this at all anymore on mozilla-central and
> mozilla-aurora (both 02/11 builds). Has an unrelated fix corrected this in
> another bug?

I'm still able to reproduce this issue on the latest Nightly and Aurora (2013-02-12)
using Samsung Galaxy S2 (Android 4.0.3) and Samsung Galaxy Tab 10.1 (Android 3.1)

Comment 19

[Adrian Tamas (:AdrianT)]

This is still an issue on Firefox Mobile 19 beta 6 on the Samsung Galaxy S2 (Android 4.0.3). The issue is easily reproducible on beta

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking status flag based on comment 19 - does this potential fix attached help?  Should we get QA a try build with it included to test on?

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Wes, can you get a try build with this patch for QA to test with?

Comment 22

[Kevin Brosnan [Ex-Mozilla]]

Pushed wesj's patch to try https://tbpl.mozilla.org/?tree=Try&rev=c8342fa64613

Adrian/Andreea would you please test the resulting builds.

Comment 23

[Adrian Tamas (:AdrianT)]

This is still an issue on Samsung Galaxy S2(Android 4.0.3) with the try build.The issue is still reproducible using the steps from comment 0

Comment 24

[Kevin Brosnan [Ex-Mozilla]]

Wes any other ideas?

Comment 25

[Aaron Train [:aaronmt]]

ping?

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this was shipped in FF19 and we're closing the window on our fourth week of beta today, this is going to have to remain unfixed in FF20 and we can look at uplift to FF21 if requested.

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Potential Fix 2

lucas jogged my brain about us needing to be better about managing our view's lifecycle here. This does that. Build at:

http://people.mozilla.com/~wjohnston/awesometaps.apk

if someone can test it. If it works, I'll have lucas review this and we can uplift.

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Potential Fix 2

grr. qrefed.

Comment 29

[Aaron Train [:aaronmt]]

(In reply to Wesley Johnston (:wesj) from comment #27)
> Created attachment 727769 [details] [diff] [review]
> Potential Fix 2
> 
> lucas jogged my brain about us needing to be better about managing our
> view's lifecycle here. This does that. Build at:
> 
> http://people.mozilla.com/~wjohnston/awesometaps.apk
> 
> if someone can test it. If it works, I'll have lucas review this and we can
> uplift.

That works for me; tested on an SII using str in comment #0.

Comment 30

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 727771 [details] [diff] [review]
Potential Fix 2

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

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +915,5 @@
>              }
>          }
>      }
> +
> +    // Implementors should override this to clean themselves up when their view is no longer cached

"@Override" annotation please.

::: mobile/android/base/awesomebar/AwesomeBarTab.java
@@ +139,5 @@
>  
>          return imm.hideSoftInputFromWindow(view.getWindowToken(), 0);
>      }
> +
> +    // Implementors should override this to clean themselves up when their view is no longer cached

destroy() is declared abstract in the same file. And implemented in the same file.
Probably "abstract" should be discarded.

Comment 31

[Wesley Johnston (:wesj)]

Comment on attachment 727771 [details] [diff] [review]
Potential Fix 2

I'm still not exactly sure what the problem is here. I originally cached these because I didn't want to have to redo db queries more than once if you were quickly flipping through sections.

Comment 32

[Wesley Johnston (:wesj)]

Whoops. Yeah, I changed this. Juggling inbound and central kills me. but didn't realize that we'd already created a destroy piece. Fixing. Thanks!

Comment 33

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 34

[Brian Nicholson (:bnicholson)]

Comment on attachment 727828 [details] [diff] [review]
Patch

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

::: mobile/android/base/awesomebar/BookmarksTab.java
@@ +90,5 @@
>      }
>  
>      @Override
> +    public void destroy(boolean isClosing) {
> +        super.destroy(isClosing);

I don't think this will compile will it? The base destroy() definition is protected, but your overridden method is public.

Comment 35

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #34)
> I don't think this will compile will it? The base destroy() definition is
> protected, but your overridden method is public.

I didn't do this on purpose, but yes it is valid to allow more access than the overridden method:

"The access specifier for an overriding method can allow more, but not less, access than the overridden method. For example, a protected instance method in the superclass can be made public, but not private, in the subclass."
http://docs.oracle.com/javase/tutorial/java/IandI/override.html

Comment 36

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #35)
> (In reply to Brian Nicholson (:bnicholson) from comment #34)
> > I don't think this will compile will it? The base destroy() definition is
> > protected, but your overridden method is public.
> 
> I didn't do this on purpose, but yes it is valid to allow more access than
> the overridden method:
> 
> "The access specifier for an overriding method can allow more, but not less,
> access than the overridden method. For example, a protected instance method
> in the superclass can be made public, but not private, in the subclass."
> http://docs.oracle.com/javase/tutorial/java/IandI/override.html

Interesting...I never knew that. Thanks for the info!

Comment 37

[Lucas Rocha (:lucasr)]

Comment on attachment 727828 [details] [diff] [review]
Patch

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

This destroy method with a boolean arguments looks a bit too hacky. I'd prefer a clearer semantic distinction between closing (a close()/pause() method equivalent to the current destroy(false)) and destroying (a destroy() method equivalent to the current destroy(true)).

Ideally, each page of awesomescreen should actually be a fragment. It would make it much simpler to understand and maintain the life cycle of each page. I haven't suggested that before because we were not using the Android compatibility library back then. But this should probably be done in a separate bug.

Comment 38

[Wesley Johnston (:wesj)]

Attachment: Patch

Fair enough. I still don't buy that fragments automagically make this easier, but it would be nice to use them here since it would make it trivial to transplant these panes onto about:home (or we could use this PagerAdapter on about:home instead!).

I used onDestroyView as a method name here to "start" that transition.

Comment 39

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #38)
> Created attachment 728521 [details] [diff] [review]
> Patch
> 
> Fair enough. I still don't buy that fragments automagically make this
> easier, but it would be nice to use them here since it would make it trivial
> to transplant these panes onto about:home (or we could use this PagerAdapter
> on about:home instead!).

I'm not expecting any magic coming out of using fragments really. I just want us to rely on the platform features/API whenever possible instead of coming up with fresh custom code to do the exact same thing.

The life cycle of pages in ViewPager is already implemented through a well-known API via fragments on Android.

Comment 40

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/943c43de7329
https://hg.mozilla.org/mozilla-central/rev/bff25b01fb0d

Comment 41

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57a3afad9252

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Something in this push was causing robocop failures and talos crashes. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf175669197

https://tbpl.mozilla.org/php/getParsedLog.php?id=21356944&tree=Mozilla-Inbound

0 INFO SimpleTest START
1 INFO TEST-START | testHistoryTab
2 INFO TEST-PASS | testHistoryTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_big_link.html should equal http://mochi.test:8888/tests/robocop/robocop_big_link.html
3 INFO TEST-PASS | testHistoryTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_blank_01.html should equal http://mochi.test:8888/tests/robocop/robocop_blank_01.html
waitForText timeout on Settings
4 INFO TEST-PASS | testHistoryTab | bookmark added successfully - true should equal true
5 INFO TEST-PASS | testHistoryTab | checking that history list exists and has 3 children - android.widget.ExpandableListView@48684fb0 should not equal null
6 INFO TEST-PASS | testHistoryTab | TextView is filled in - Big Link
7 INFO TEST-PASS | testHistoryTab | TextView is filled in - http://mochi.test:8888/tests/robocop/robocop_big_link.html
8 INFO TEST-PASS | testHistoryTab | Correct number of ImageViews visible - 1 should equal 1
9 INFO TEST-PASS | testHistoryTab | TextView is filled in - Browser Blank Page 01
10 INFO TEST-PASS | testHistoryTab | TextView is filled in - http://mochi.test:8888/tests/robocop/robocop_blank_01.html
11 INFO TEST-PASS | testHistoryTab | Correct number of ImageViews visible - 2 should equal 2
12 INFO TEST-PASS | testHistoryTab | First row has Today header - Today
13 INFO TEST-PASS | testHistoryTab | Correct number of ImageViews visible - 0 should equal 0
INFO | automation.py | Application ran for: 0:01:36.113976
INFO | zombiecheck | Reading PID log: /tmp/tmp1lAcDrpidlog
PROCESS-CRASH | java-exception | java.lang.NullPointerException	at org.mozilla.gecko.AllPagesTab.setSearchEngines(AllPagesTab.java:625)

https://tbpl.mozilla.org/php/getParsedLog.php?id=21357237&tree=Mozilla-Inbound

Failed tcheckerboard: 
		Stopped Tue, 02 Apr 2013 12:30:53
    talos_results.add(mytest.runTest(browser_config, test))
  File "/builds/tegra-158/talos-data/talos/ttest.py", line 397, in runTest
    test_results.add(browser_log_filename, counter_results=counter_results)
  File "/builds/tegra-158/talos-data/talos/results.py", line 128, in add
    browserLog = BrowserLogResults(filename=results, counter_results=counter_results, global_counters=self.global_counters)
  File "/builds/tegra-158/talos-data/talos/results.py", line 324, in __init__
    self.error(match.group(1))
  File "/builds/tegra-158/talos-data/talos/results.py", line 339, in error
    raise utils.talosError(message)
talosError: 'browser non-zero return code (1) [browser_output.txt]'

Comment 43

[Wesley Johnston (:wesj)]

Attachment: Patch v4

Sorry for the delay here lucas. The test failures are caused by two things.

1.) A race with the awesomebar animation that seems to be tickled here. I fixed that in bug 729463.

2.) Some background tasks returning (queries for the favicons) after we've destroyed the cursor and cursorAdapter and throwing/crashing. This JUST cleans up the views in onDestoryView() and leaves the cursors around. That seems better for performance if you're quickly shuffling between views anyway.

Comment 44

[Lucas Rocha (:lucasr)]

Comment on attachment 737583 [details] [diff] [review]
Patch v4

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

::: mobile/android/base/AwesomeBarTabs.java
@@ +296,5 @@
>          }
>      }
>  
>      public void destroy() {
> +        Log.i(LOGTAG, "onDestroy");

Remove this log?

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +342,5 @@
>              boolean changed = !mSearchTerm.equals(searchTerm);
>              mSearchTerm = searchTerm;
>  
>              if (changed)
> +                notifyDataSetChanged();

Unrelated?

Comment 45

[u421692]

I was able to reproduce this issue on Samsung Galaxy Tab(Android 4.0.4), Samsung Galaxy S2(Android 4.0.3), Acer Iconia A500(Android 3.2.1), Samsung Galaxy Note(Android 4.0.4). However I was not able to reproduce this issue on Samsung Galaxy R(Android 2.3.4) and Samsung Galaxy Tab 2(Android 4.1). We can say that is a issue on honeycomb/ice cream sandwich devices.

Comment 46

[Aaron Train [:aaronmt]]

(In reply to Pop Mihai from comment #45)
> I was able to reproduce this issue on Samsung Galaxy Tab(Android 4.0.4),
> Samsung Galaxy S2(Android 4.0.3), Acer Iconia A500(Android 3.2.1), Samsung
> Galaxy Note(Android 4.0.4). However I was not able to reproduce this issue
> on Samsung Galaxy R(Android 2.3.4) and Samsung Galaxy Tab 2(Android 4.1). We
> can say that is a issue on honeycomb/ice cream sandwich devices.

Please file a new bug.

Comment 47

[u421692]

After further investigation, I observed that this issue is only reproducible on the latest Release and Beta, but is not reproducible on the latest Nightly and Aurora. I beleive the patch was never uplifted to Firefox Mobile 22. Should this be uplifted to Beta?