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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

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

[Doug Turner (:dougt)]

These are broken too?

http://www.w3.org/TR/html5/history.html#the-history-interface

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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).

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/2) Minor cleanup in Tab.java

Same patch as was r+'d by Sriram in bug 695165

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/2) Keep java state in sync with gecko

Comment 5

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

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: (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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/projects/birch/rev/d0e53c1bb8e3
https://hg.mozilla.org/projects/birch/rev/ce66d1dd8247

Comment 8

[Catalin Suciu [:csuciu]]

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