Closed Bug 677565 Opened 8 years ago Closed 8 years ago

Session restore sometimes loses tabs' URLs

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 8

People

(Reporter: mounir, Assigned: justin.lebar+bug)

References

Details

Attachments

(2 files)

Attached image screenshot
Sometimes, when lauching Firefox Mobile, the tabs seem to be restored but they are actually empty documents with no URL's. It happens also that some tabs are correctly restored but not all of them.
I thought it was related to the app being killed before being able to save everything but it also happened when updating Nightly... Even if, we should be able to save the tabs URL's before being ask to shutdown, shouldn't we?
Yeah, I got the same thing after updating current Nightly.
Note that about: urls are never restored.
Summary: Session restore sometimes loose tabs URL's → Session restore sometimes lose tabs URL's
Summary: Session restore sometimes lose tabs URL's → Session restore sometimes loses tabs URL's
How are you shutting down? Yes, we should be saving the full session before shutdown.

Martijn - is this repeatable?
Yes, I can reproduce.
I downloaded yesterday's trunk build, I had three urls open. After updating to today, the three tabs opened up again, but nothing loaded in them, they stayed blank.

One new js error in the error console that I haven't seen before is:
shEntry.setUniqueDocIdentifier is not a function
chrome://browser/content/bindings/browser.js
282

That might have something to do with it?

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/browser.js#282
I just experienced a sudden OS reboot. After reopening Fennec, it loaded the restored urls just fine in the corresponding tabs. So perhaps only happening after a software update?
Definitely, it doesn't happen *only* after a software update. It happens/happened quite often on my phone: I tend to have a lot of tabs opened and sometimes some of them change to a blank page after a session restore (usually, after being killed by the system).
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #3)
> Yes, I can reproduce.
> I downloaded yesterday's trunk build, I had three urls open. After updating
> to today, the three tabs opened up again, but nothing loaded in them, they
> stayed blank.
> 
> One new js error in the error console that I haven't seen before is:
> shEntry.setUniqueDocIdentifier is not a function
> chrome://browser/content/bindings/browser.js
> 282
> 
> That might have something to do with it?
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/
> browser.js#282

Looks like bug 646641 broke Fennec.
Blocks: 646641
Are there any tests for this?  I can try to fix it, but this code (at least on desktop) is very fragile.
> if (aEntry.stateData)
>   shEntry.stateData = aEntry.stateData;

This is also wrong -- the equivalent code in nsSessionStore.js is

> if (aEntry.stateData != null) {
>   entry.structuredCloneState = aEntry.stateData.getDataAsBase64();
>   entry.structuredCloneVersion = aEntry.stateData.formatVersion;
> }
Attached patch Patch v1Splinter Review
Summary: Session restore sometimes loses tabs URL's → Session restore sometimes loses tabs' URLs
I don't think bug 646641 is to blame for all of Fennec's problems.  At revision f182f03aaee9, before bug 646641 landed, SR still doesn't restore a tab to [1].

This makes the patch I wrote very hard to test.

[1] http://people.mozilla.org/~jlebar/test/test_pushstate_sr.html
It appears that there may be other things wrong here.  For instance, when I navigate to [1] from comment 10 and click the "push some state" button, a new history entry does not appear to be serialized.
At f182f03aaee9 (before bug 646641), SR will restore

  http://people.mozilla.org/~jlebar/test/2011-04/

but not

  http://people.mozilla.org/~jlebar/test/2011-04/kerning/

STR: open page in Fennec on desktop, close Fennec by clicking 'x' at corner of window, reopen Fennec.

Expected results: Page shows up in "tabs from last session."

Actual results: For second URL, no pages in "tabs from last session."
Comment on attachment 552103 [details] [diff] [review]
Patch v1

I'm having a lot of trouble testing this myself.  Basically any page I put up on people.mozilla.org isn't restored in session restore, and that's using a revision which doesn't include the changes from bug 646641.

In my experience on desktop, this code is very fragile and has a lot of edge cases, since it interacts with docshell in strange ways.  It's very hard to get right when there are tests, and without any tests, it'd probably be impossible.  I wish the code hadn't been duplicated, so correctness on the desktop could suggest correctness on mobile, but I guess that ship has sailed.

Having said that, bug 646641 definitely should have updated the mobile code.  This patch should improve things, although again, it's hard for me to test.  Locally, the patch doesn't seem to completely break SR, but that's about all I can say.
Attachment #552103 - Attachment description: WIP v1 → Patch v1
Attachment #552103 - Flags: review?(mark.finkle)
Comment on attachment 552103 [details] [diff] [review]
Patch v1


>+    if (aEntry.structuredCloneState && aEntry.structuredCloneVersion) {
>+      shEntry.stateData =
>+        Cc["@mozilla.org/docshell/structured-clone-container;1"].
>+        createInstance(Ci.nsIStructuredCloneContainer);
>+
>+      shEntry.stateData.initFromBase64(aEntry.structuredCloneState,
>+                                       aEntry.structuredCloneVersion);

nit: no need to wrap here

>+      let matchingEntry = aDocIdentMap[aEntry.docIdentifier];
>+      if (!matchingEntry) {
>+        aDocIdentMap[aEntry.docIdentifier] = shEntry;
>+      }
>+      else {
>+        shEntry.adoptBFCacheEntry(matchingEntry);
>+      }

nit: } else {

r+, but some tiny nits. Thanks!
Attachment #552103 - Flags: review?(mark.finkle) → review+
Thanks for the quick review, Mark!

Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/dd0630c44b2d
Whiteboard: [inbound]
Merged:
http://hg.mozilla.org/mozilla-central/rev/dd0630c44b2d
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 8
I've tried the following scenario:

1. On device BUILD ID: Mozilla/5.0 (Android;Linux armv7l; rv:9.0a1)Gecko/20110926
Firefox/9.0a1 Fennec /9.0a1 is installed.
2. Open some tabs. (5 tabs)
3. Update to latest Nightly.(Mozilla/5.0 (Android;Linux armv7l; rv:9.0a1) Gecko/20110927 Firefox/9.0a1 Fennec /9.0a1)
4. Verify tabs are properly restored and loaded.

Tabs are properly restored and loaded.

I've tried for several times this scenario and no tabs were lost. 

Verifying bug.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.