Last Comment Bug 697732 - Back/forward from javascript do not update Java-side state
: Back/forward from javascript do not update Java-side state
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P3 normal (vote)
: ---
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on:
Blocks: 695165
  Show dependency treegraph
 
Reported: 2011-10-27 09:09 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-01-09 11:04 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
(1/2) Minor cleanup in Tab.java (2.95 KB, patch)
2011-10-28 06:51 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review+
Details | Diff | Splinter Review
(2/2) Keep java state in sync with gecko (14.15 KB, patch)
2011-10-28 06:51 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review
(2/2) Keep java state in sync with gecko (v2) (13.94 KB, patch)
2011-10-31 08:09 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review+
Details | Diff | Splinter Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2011-10-27 09:09:33 PDT
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) 2011-10-27 10:07:48 PDT
These are broken too?

http://www.w3.org/TR/html5/history.html#the-history-interface
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-27 10:55:27 PDT
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:kats@mozilla.com) 2011-10-28 06:51:08 PDT
Created attachment 570246 [details] [diff] [review]
(1/2) Minor cleanup in Tab.java

Same patch as was r+'d by Sriram in bug 695165
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-28 06:51:37 PDT
Created attachment 570247 [details] [diff] [review]
(2/2) Keep java state in sync with gecko
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-31 07:36:36 PDT
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:kats@mozilla.com) 2011-10-31 08:09:57 PDT
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.
Comment 8 Catalin Suciu [:csuciu] 2011-11-23 05:22:11 PST
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

Note You need to log in before you can comment on or make changes to this bug.