846013 - about:blank subframe entries in session restore can cause perf problems

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Attachment: patch

This was fixed in desktop a while ago (bug 705597), and seems like a worthwhile perf win, even if sessionstore.js never gets as big as it could on desktop.

Comment 1

[Brian Nicholson (:bnicholson)]

Comment on attachment 719179 [details] [diff] [review]
patch

Review of attachment 719179 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/components/SessionStore.js
@@ -668,5 @@
>          let child = aEntry.GetChildAt(i);
> -        if (child)
> -          entry.children.push(this._serializeHistoryEntry(child));
> -        else // to maintain the correct frame order, insert a dummy entry
> -          entry.children.push({ url: "about:blank" });

It's not clear what this bit here was for, so we should watch out for session restore regressions.

Comment 2

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #1)
> Comment on attachment 719179 [details] [diff] [review]
> patch
> 
> Review of attachment 719179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/components/SessionStore.js
> @@ -668,5 @@
> >          let child = aEntry.GetChildAt(i);
> > -        if (child)
> > -          entry.children.push(this._serializeHistoryEntry(child));
> > -        else // to maintain the correct frame order, insert a dummy entry
> > -          entry.children.push({ url: "about:blank" });
> 
> It's not clear what this bit here was for, so we should watch out for
> session restore regressions.

Reading the comments in bug 705597, it looks like this is a historical artifact that's no longer needed. Cc'ing ttaubert in case he remembers what the implications of removing this might be.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Starting with bug 705597 comment #33 nobody really seemed to know why we were inserting about:blank entries "to maintain the correct frame order" in the first place. I removed that part and I can't recall any problems we had with it.

Wait... I just read bug 705597 comment #43 and I recall the .childCount attribute being wrong when dynamic history entries (e.g. iframes) have been removed. That lead to GetEntryAtIndex() failing.

... and I fixed that in bug 707862. So that patch by Margaret should be good :)

Comment 4

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd374908d03

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fcd374908d03