Closed Bug 1337940 Opened 7 years ago Closed 7 years ago

URL not properly restored on Reddit after navigating to the next page

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox54 verified)

VERIFIED FIXED
Firefox 54
Tracking Status
firefox54 --- verified

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(3 files)

This is about the session store part of this bug report.

+++ This bug was initially created as a clone of Bug #1337264 +++

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160606113944

Steps to reproduce:

1. Navigate to https://m.reddit.com/
2. Scroll to bottom; tap "NEXT" button at bottom right to navigate to the next page of content
3. Tap location bar to notice the URL has updated to reflect the new page (eg, https://www.reddit.com/?count=25&page=1&after=t3_5sf6ns)
4. Kill Firefox from Android task manager
5. Restart Firefox 


Actual results:

* After navigating to the second page, the location bar display is not updated properly, showing only https://www.reddit.com/ until I tap it, when it displays the actual URL of the current page
* After Firefox is restarted, the front page that I navigated away from is restored


Expected results:

* The location should have updated to display the correct URL without having to tap to select the location
* The page I was on when Firefox was killed should have been restored

I suspect the problem is related to a javascript page transition on the Reddit site, but I have not investigated this, so I could be wrong.
Blocks: 1337325
Comment on attachment 8836410 [details]
Bug 1337940 - Part 1 - Capture session store tab data on history listener notifications.

https://reviewboard.mozilla.org/r/111834/#review113414
Attachment #8836410 - Flags: review?(ahunt) → review+
Blocks: 1339206
Comment on attachment 8836864 [details]
Bug 1337940 - Part 2 - Make session store form data test work again.

https://reviewboard.mozilla.org/r/112162/#review115650
Attachment #8836864 - Flags: review?(ahunt) → review+
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/0172497c1024
Part 1 - Capture session store tab data on history listener notifications. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/dd8c7da15822
Part 2 - Make session store form data test work again. r=ahunt
That test seems to be causing a lot of location and presumably also history changes. Previously, the session store simply ignored those, but now it tracks them as well, so presumably the added overhead of capturing the tab state each time is enough to make this test time out on a debug build. I'll try and see if maybe folding bug 1339206 into this or anything else could reduce the impact here or whether we have to bite the bullet and increase the timeout.
Flags: needinfo?(jh+bugzilla)
So once again it turn out that the emulator is slooow and a debug build even sloooower. test_parse_rule consists of loads of sub-tests (more than 100 I think) and each new sub-test means a navigation and a new session history entry. We already start the test with something like 17 history entries and quickly run into the limit set by browser.sessionhistory.max_entries at 50 entries. At that point, collecting the history takes almost 1 s per call, so you can imagine the overhead this adds. Since these navigations don't change the page title, the session store previously didn't collect any history at all here.

Bug 1341810 might help, so we wouldn't have to collect the full tab history all the time, although probably not all that much - once we run into the max_entries limit, we're also starting to get OnHistoryPurge notifications, where we can't avoid a full collect because we don't know precisely what has been purged. Besides, it'll probably be a while before that bug gets implemented.

So in the meantime I'll try and see whether I can get lowering browser.sessionhistory.max_entries for this test only to have any immediate effect and if not it's probably going to be requestLongerTimeout().
Attachment #8840764 - Flags: review?(matt.woodrow) → review?(dbaron)
Comment on attachment 8840764 [details]
Bug 1337940 - Part 3 - Restrict the amount of session history we keep around during test_parse_rule.

https://reviewboard.mozilla.org/r/115042/#review117012

::: layout/style/test/test_parse_rule.html:256
(Diff revision 1)
> +SpecialPowers.setIntPref("browser.sessionhistory.max_entries", 4);
> +
> +SimpleTest.registerCleanupFunction(function() {
> +  SpecialPowers.clearUserPref("browser.sessionhistory.max_entries");
> +});

The concept here seems fine to me, but I believe it's considered preferable to use SpecialPowers.pushPrefEnv rather than using setIntPrefand clearUserPref directly (particularly since prefs take a little time to take effect in e10s, if I recall correctly).

So unless there's a reason you think this advice is wrong, could you restructure this to use pushPrefEnv instead?

See:
http://searchfox.org/mozilla-central/rev/90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/testing/specialpowers/content/specialpowersAPI.js#998
Attachment #8840764 - Flags: review?(dbaron) → review-
Comment on attachment 8840764 [details]
Bug 1337940 - Part 3 - Restrict the amount of session history we keep around during test_parse_rule.

https://reviewboard.mozilla.org/r/115042/#review117198

r=dbaron.  Thanks.
Attachment #8840764 - Flags: review?(dbaron) → review+
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/4285ead0e57a
Part 1 - Capture session store tab data on history listener notifications. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/6fa30faf3791
Part 2 - Make session store form data test work again. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/3e542f18ed62
Part 3 - Restrict the amount of session history we keep around during test_parse_rule. r=dbaron
Verified as fixed with the latest Nightly 54.0a1 build (2017-03-02).
Device: HTC 10 (Android 6.0.1).
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.