Closed Bug 817586 Opened 9 years ago Closed 9 years ago

Top Sites pages cannot be tapped after going through History

Categories

(Firefox for Android Graveyard :: General, defect)

20 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox18 unaffected, firefox19 affected, firefox20+ wontfix, firefox21 affected, fennec21+)

VERIFIED FIXED
Firefox 22
Tracking Status
firefox18 --- unaffected
firefox19 --- affected
firefox20 + wontfix
firefox21 --- affected
fennec 21+ ---

People

(Reporter: xti, Assigned: wesj)

References

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

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)
Blocks: ptpbmode
Assignee: nobody → sriram
This happens on normal browsing mode too. This is probably a regression from ViewPager.
No longer blocks: ptpbmode
Summary: PBM - Top Sites pages cannot be tapped after going through History → Top Sites pages cannot be tapped after going through History
Sriram - any progress here?
tracking-fennec: --- → ?
I have a WIP for this.
Assignee: sriram → wjohnston
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.
tracking-fennec: ? → 19+
(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.
Attached patch Potential fix (obsolete) — Splinter Review
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?
Attachment #704046 - Flags: review?(lucasr.at.mozilla)
(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 can reproduce on Beta but the item in Top-Sites is selected on second tap, not first tab.
(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.
Mark - heard wesj is out (congrats to him :). Can you help answer Lucas's question in comment 7?
Assignee: wjohnston → mark.finkle
Sriram - Can you still make this happen? If so, does Wes' patch fix it for you?
Flags: needinfo?(sriram)
(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.
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.
Flags: needinfo?(sriram)
19 is unaffected; wfm
Comment on attachment 704046 [details] [diff] [review]
Potential fix

Removing review flag for now.
Attachment #704046 - Flags: review?(lucasr.at.mozilla)
(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.
Keywords: regression
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?
(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)
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
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?
Flags: needinfo?(mark.finkle)
Wes, can you get a try build with this patch for QA to test with?
Assignee: mark.finkle → wjohnston
Flags: needinfo?(mark.finkle)
Keywords: qawanted, verifyme
Flags: needinfo?(wjohnston)
Pushed wesj's patch to try https://tbpl.mozilla.org/?tree=Try&rev=c8342fa64613

Adrian/Andreea would you please test the resulting builds.
Flags: needinfo?(andreea.pod)
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
Flags: needinfo?(andreea.pod)
Wes any other ideas?
Keywords: qawanted, verifyme
QA Contact: adrian.tamas
ping?
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.
tracking-fennec: 19+ → 21+
Attached patch Potential Fix 2 (obsolete) — Splinter Review
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.
Attachment #704046 - Attachment is obsolete: true
Flags: needinfo?(wjohnston)
Attached patch Potential Fix 2 (obsolete) — Splinter Review
grr. qrefed.
Attachment #727769 - Attachment is obsolete: true
(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 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 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.
Attachment #727771 - Flags: review?(lucasr.at.mozilla)
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!
Attached patch Patch (obsolete) — Splinter Review
Attachment #727771 - Attachment is obsolete: true
Attachment #727771 - Flags: review?(lucasr.at.mozilla)
Attachment #727828 - Flags: review?(lucasr.at.mozilla)
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.
(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
(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 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.
Attachment #727828 - Flags: review?(lucasr.at.mozilla) → review-
Attached patch PatchSplinter Review
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.
Attachment #727828 - Attachment is obsolete: true
Attachment #728521 - Flags: review?(lucasr.at.mozilla)
Attachment #728521 - Flags: review?(lucasr.at.mozilla) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/943c43de7329
https://hg.mozilla.org/mozilla-central/rev/bff25b01fb0d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
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]'
Attached patch Patch v4Splinter Review
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.
Attachment #737583 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #737583 - Flags: review?(lucasr.at.mozilla) → review+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.