677565 - Session restore sometimes loses tabs' URLs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: 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?

Comment 1

[Martijn Wargers (dead)]

Yeah, I got the same thing after updating current Nightly.
Note that about: urls are never restored.

Comment 2

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

How are you shutting down? Yes, we should be saving the full session before shutdown.

Martijn - is this repeatable?

Comment 3

[Martijn Wargers (dead)]

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

Comment 4

[Martijn Wargers (dead)]

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?

Comment 5

[Mounir Lamouri (:mounir)]

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

Comment 6

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

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

Comment 7

[Justin Lebar (not reading bugmail)]

Are there any tests for this?  I can try to fix it, but this code (at least on desktop) is very fragile.

Comment 8

[Justin Lebar (not reading bugmail)]

> 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;
> }

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 10

[Justin Lebar (not reading bugmail)]

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

Comment 11

[Justin Lebar (not reading bugmail)]

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.

Comment 12

[Justin Lebar (not reading bugmail)]

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 13

[Justin Lebar (not reading bugmail)]

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.

Comment 14

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

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!

Comment 15

[Justin Lebar (not reading bugmail)]

Thanks for the quick review, Mark!

Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/dd0630c44b2d

Comment 16

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/dd0630c44b2d

Comment 17

[Carla Nadastean]

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.