Back/forward from javascript do not update Java-side state

VERIFIED FIXED

Status

()

Firefox for Android
General
P3
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
All
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Pages can do javascript:back() or javascript:forward(), which should update the back/forward state in the Java code to keep everything in sync. Currently there are not many user-visible issues that this results in, but one such issue can be reproduced as follows:

1. Start Fennec
2. Go to any page (e.g. google.com)
3. Go to http://people.mozilla.org/~kgupta/bug/695165.html
4. Click on the "Back" link. This should take you back to google
5. Quit Fennec using the menu item
6. Start up Fennec again

At step 6, it shows the screenshot for google, but is actually loading the test page from people.mozilla.org page in the background, so when gecko is up, it replaces the google screenshot with the test page. This is a result of the Java-side history stack getting out of sync with the gecko history.

Comment 1

6 years ago
These are broken too?

http://www.w3.org/TR/html5/history.html#the-history-interface
Assignee: nobody → kgupta
Priority: -- → P3
Somewhat, yes. pushState updates the URL bar correctly because it sends an onLocationChange event up, but things get out of sync, so hitting back could cause the browser to exit rather than going back further in the history (java history stack runs out too quickly).
Created attachment 570246 [details] [diff] [review]
(1/2) Minor cleanup in Tab.java

Same patch as was r+'d by Sriram in bug 695165
Attachment #570246 - Flags: review+
Created attachment 570247 [details] [diff] [review]
(2/2) Keep java state in sync with gecko
Attachment #570247 - Flags: review?(sriram)
Comment on attachment 570247 [details] [diff] [review]
(2/2) Keep java state in sync with gecko


>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>+            } else if (event.startsWith("SessionHistory")) {
>+                Tab tab = Tabs.getInstance().getTab(message.getInt("tabID"));
>+                if (tab != null) {
>+                    event = event.substring("SessionHistory:".length());
>+                    tab.handleSessionHistoryMessage(event, message);
>+                }

Why not register a message listener for Tabs and do the redirect there instead of here? I guess you still need to do the "find the tab with tabID" dance, but at lteast it seems more appropriate out of GeckoApp. Think about it.
>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>     let flags = Ci.nsIWebProgress.NOTIFY_STATE_ALL |
>                 Ci.nsIWebProgress.NOTIFY_LOCATION |
>                 Ci.nsIWebProgress.NOTIFY_PROGRESS;
>     this.browser.addProgressListener(this, flags);
>+    this.browser.sessionHistory.addSHistoryListener(this);
>+

Just a note that this.browser.sessionHistory" is lazy and might not actually exist early in the lifetime of the browser. You might be fine in this patch, but I know desktop and mobile use crappy "while (sessionHistory is null) { wait and try again }" logic in some places. Just an FYI.

>+  OnHistoryNewEntry: function(aUri) {
>+  OnHistoryGoBack: function(aUri) {
>+  OnHistoryGoForward: function(aUri) {
>+  OnHistoryPurge: function(aNumEntries) {

Instead of sending -1 for every index except "Goto", you might be able to send "this.browser.sessionHistory.index" - not that it is used in Java, but it would atleast be the right index.

r+, but think about the suggested changes
Attachment #570247 - Flags: review?(sriram) → review+
Created attachment 570705 [details] [diff] [review]
(2/2) Keep java state in sync with gecko (v2)

I moved the event handling into Tabs.java as it does make more sense there, and also stopped sending the index and uri if they are not used. I could have sent browser.sessionHistory.index but decided against because as per bug 698094 I want to lean towards reducing the sync state rather than increasing it, and it's also more efficient this way.
Attachment #570247 - Attachment is obsolete: true
Attachment #570705 - Flags: review+
https://hg.mozilla.org/projects/birch/rev/d0e53c1bb8e3
https://hg.mozilla.org/projects/birch/rev/ce66d1dd8247
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Verified fixed on the latest fennec native build:
Mozilla/5.0 (Android;Linux armv7l;rv:11.0a1)Gecko/20111122 Firefox/11.0a1 Fennec/11.0a1
Samsung GalaxyS, Android 2.2
Status: RESOLVED → VERIFIED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.