921762 - Collecting text and scroll data separately from history can lead to inconsistencies

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just discovered this problem during normal browsing today. When we collect session history, we store data for iframes in a list of children. Then when we collect text-and-scroll data, we iterate over the list of iframes and assume that it matches up with the list of children that we collected earlier.

However, in between collecting history and collecting text-and-scroll data, we return to the event loop, which allows code from the page to run. nytimes.com has a habit of creating and destroying iframes constantly. If it changes the set of iframes in between collecting history and collecting text-and-scroll data, then the history data that we collect won't match up with the text-and-scroll data.

I noticed this because the patches in bug 909048 cause this failure to be more verbose (i.e., there's a JS error). The current code manages to get away without throwing any errors, but it still returns bad data. What do you guys think we should do here? I can think of three things:

1. Collect both kinds of data atomically. I don't know how expensive it is to collect them, so I don't know how much it matters.

2. Try to detect when the set of frames has changed, and recollect the data atomically in that case. I'm not sure how difficult the detection would be though.

3. Ignore the problem as we do now. I can make the patches in bug 909048 fail silently. This doesn't really appeal to me though.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Tim is more up to date than me on the latest workings of Session Restore, but I believe that there is no problem with 1., as long as we do not recollect history when it's not needed.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

This is going to be fixed by bug 909048, right?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Fixed by bug 909048.