Closed Bug 921942 Opened 11 years ago Closed 11 years ago

Scroll position isn't restored in background tabs after session restore

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Version:
25 Branch
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 29

People

(Reporter: a.samirh78, Assigned: ttaubert)

References

Details

(Keywords: regression)

Attachments

(2 files, 9 obsolete files)

0002-Bug-921942-Add-FrameTree-and-scroll-position-broadca.patch
26.04 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
0001-Bug-921942-Broadcast-scroll-positions-r-yoric.patch (v7)
23.12 KB, patch
Yoric
: review+
Details | Diff | Splinter Review 
With Firefox set to restore the current windows and tabs:
- Open any web page A, and scroll down the page a bit
- Open another web page B in another tab, also scroll down a bit
- Make sure tab B is the current/active one then restart Firefox
- Notice that the scroll position on tab B has been restored correctly, whereas the scroll position on tab A (background tab) isn't restored

This is with Firefox 25 beta 3. The same issue happens with a brand new profile.
Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0

Reproducing on Firefox 25 beta 6 (build ID: 20131007213254) using the steps from comment 0.
I have used the crash me add-on to trigger a crash.

Marking this issue as new.
Status: UNCONFIRMED → NEW
Ever confirmed: true
There's something definitely off about restoring the scroll position, FWIW I still see the same problem in the released 25.0.
here is the regression window i'm getting for this issue: 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb48c7d58b8b&tochange=73b69c146ca6

there are a few "session restore"-related changes in there...
Flags: needinfo?(ttaubert)
I'm quite sure that bug 867143 is the cause here. We probably always queried scroll data back in the days without a tab state cache. IMO we should wait for bug 930967 and implement it properly so that we don't re-collect everything just because the user scrolled the page.

If we think it's a big issue (I don't) we could also look into a quick fix for now that sends an async message whenever the 'scroll' event is received and try to surgically update the cached tab state, if any.

David, any opinion on this?
Blocks: 867143
Flags: needinfo?(ttaubert) → needinfo?(dteller)
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
Full agreement with Tim: let's wait for bug 930967 and do this correctly once it has landed (or as part of that bug).
Flags: needinfo?(dteller)
Hello. Bug 930967 has been fixed. Any news about this one?
So, we would need to register in content-sessionStore.js to be informed of "scroll" events, and to call TextAndScrollData.updateFrame.
Yeah, we really need to make sure to not block for too long but pushing a lazy message onto the queue should be fine. The problem is that we store the scroll position for every frame and thus have to currently invalidate the whole session history, as David said. We can't event just push a new message onto the queue because session history isn't broadcasted, yet.

I don't think we can justify re-collecting the whole session history for a tab every time a user scrolls and we will need to come up with something better. With shistory broadcasted and improved to only send diffs for the current shistory entry (we're not quite there yet or working on it) we could at least just update the current shistory entry.

An idea might be to only ever store scroll positions for the current entry and its children, we could then try to collect scroll positions decoupled from session history and save a little space at the same time.
We'll run into similar problems with other data that we want to update and maintain separately from session history entries.

I just went for a more radical approach: the scroll position is now broadcasted nicely but only for the top-most frame. This should cover most of the pages and we don't need to save and query scroll positions for every child frame out there.

Left to do: write a test and rename TextAndScrollData.jsm.
Attachment #8339987 - Flags: feedback?(dteller)
I'll try and review this later today.

Random thought: by listening to "scroll" on the <xul:browser> element, we should be informed of scroll changes on all frames. I believe that the event contains the necessary information to find out which frame has scrolled. I hope that, with a few DOM operations, we should be able to deduce to which (sub)entry in |children| this maps (e.g. "frame [0,0,0,1] has just scrolled to position {x, y}"), which should let us broadcast just the scroll changes.
Listening in the xul:browser won't work in e10s but we can have the same results in the frame script as the scroll event should just bubble up. I thought about doing this and we can certainly think about this more. I just couldn't come up with a case where I thought it's important to restore the scroll position in subframes, after all this isn't really about rescuing data but more about convenience.

The problem with data stored per frame is that we not only have to send updates when the data changes but also when a frame is removed, re-ordered or a new one added.
> I just couldn't come up with a case where I thought it's important to restore the scroll position in subframes, after all this isn't really about rescuing data but more about convenience.

Well, some websites (still) put everything meaningful in a frameset, so the only meaningful scrolling information is in a frame. I assume that some do the same with iframes.

> The problem with data stored per frame is that we not only have to send updates when the data changes but also when a frame is removed, re-ordered or a new one added.

Good point.
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #12)
> > I just couldn't come up with a case where I thought it's important to restore the scroll position in subframes, after all this isn't really about rescuing data but more about convenience.
> 
> Well, some websites (still) put everything meaningful in a frameset, so the
> only meaningful scrolling information is in a frame. I assume that some do
> the same with iframes.

Argh, yes. :(
Another idea: we register a "load" event handler in the frame script. As soon as "load" is fired (for the top frame) we enumerate all frames and their subframes recursively and store them somewhere (weakly referenced of course).

Now when receiving a scroll event all we care about is if that originates from one of the frames we collected at "load". If so we will send an update of the scroll position tree and just assume that the frame position is still the same.

That way we could collect scroll positions for subframes that we actually can restore. Everything after the load event is handled by us anyway. We could use the same approach to determine whether to store and collect form data for a frame. That also might help us with bug 936271?
Comment on attachment 8339987 [details] [diff] [review]
0001-Bug-921942-Update-scroll-position-when-scrolling-sto.patch

Better patch in the works.
Attachment #8339987 - Attachment is obsolete: true
Attachment #8339987 - Flags: feedback?(dteller)
Ok, here's another patch that does the following:

* Scroll positions and form data is now broadcasted.
* When the page loads, we collect a tree of reachable frames that is then used to determine whether we should re-collect data when an event is handled.
* We ignore any other frame that has been added after the load event, we're not able to restore anything for it anyway.
* Scrolling and/or changing form input fields doesn't invalidate the whole tab anymore.

There are a few follow-ups when we take this. Currently our MessageQueue doesn't handle diffs very well, so for now we have to send all scroll positions and all form data to the parent process.

There are a few tests that break because they are waiting for a SessionStore:input message or simply because they aren't waiting for anything. We should update and probably merge all those subtests to have a nice and up-to-date form data test. Same goes for scroll positions.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8340386 - Flags: feedback?(dteller)
New approach.

This patch splits scroll positions from shistory entries. This allows us to store scroll positions for frames that are created dynamically before the load event is dispatched, even if we're not going to create history entries for those in the future.

It introduces a FrameTree module that is used by the frame scripts to determine which frames we need to collect data for. This provides a nice framework to do the same for form data in a follow-up.

With both the scroll and form data moved out of tabData.entries[] we could just never store shentries that were dynamically added, yay.

I'm working on frame tree and scroll data tests and will post the patch later.
Attachment #8340386 - Attachment is obsolete: true
Attachment #8340386 - Flags: feedback?(dteller)
Attachment #8340892 - Flags: review?(dteller)
Comment on attachment 8340892 [details] [diff] [review]
0001-Bug-921942-Broadcast-scroll-positions.patch

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

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +206,5 @@
> + * frame we update the scroll position and will restore it when requested.
> + *
> + * Causes a SessionStore:update message to be sent that contains the current
> + * scroll positions as a tree of strings. If no frame is scrolled this will
> + * return null.

That last sentence might need some details.

@@ +220,5 @@
> +
> +  handleEvent: function (event) {
> +    if (gFrameTree.contains(event.target.defaultView)) {
> +      MessageQueue.push("scroll", this.collect);
> +    }

Could you add a comment explaining why the |contains| could return |false|?

@@ +228,5 @@
> +    MessageQueue.push("scroll", () => null);
> +  },
> +
> +  collect: function () {
> +    return gFrameTree.map(ScrollPosition.collect);

I like it.

::: browser/components/sessionstore/src/FrameTree.jsm
@@ +15,5 @@
> +
> +/**
> + * Recursively merges all properties of a given object |from| to a given
> + * object |to|. The first argument passed in will be modified and returned.
> + */

You should document the fact that we overwrite any non-object key.

@@ +17,5 @@
> + * Recursively merges all properties of a given object |from| to a given
> + * object |to|. The first argument passed in will be modified and returned.
> + */
> +function merge(to, from) {
> +  if (!to || typeof(from) !== "object") {

|| !from
otherwise you won't handle the case in which |from == null|, I believe.

@@ +30,5 @@
> +}
> +
> +/**
> + * A FrameTree represents all frames that were reachable when the document
> + * was loaded. We will use this information to ignore frames when collecting

Nit: "we use"

@@ +67,5 @@
> +  _ifreq: null,
> +
> +  // A weakmap of frames mapping to lists of their children. This represents
> +  // the current frame tree as reachable when the document was loaded.
> +  _frames: null,

Could you move these values (and their doc) to where they live, i.e. in the constructor?

@@ +70,5 @@
> +  // the current frame tree as reachable when the document was loaded.
> +  _frames: null,
> +
> +  // The set of observers that will be notified when the frame tree is reset.
> +  _observers: null,

Nit: If it's a |Set|, could you write "Set" instead of "set"?

@@ +99,5 @@
> +    return this._frames.has(frame);
> +  },
> +
> +  /**
> +   * Recursively applies the stored frame tree to a given function |cb|. The

Nit: Actually, you are applying the function to the frame tree, aren't you?

@@ +111,5 @@
> +  map: function (cb) {
> +    let frames = this._frames;
> +
> +    function walk(frame) {
> +      let obj = merge({}, cb(frame) || {});

I don't understand why you need to merge anything here.

@@ +114,5 @@
> +    function walk(frame) {
> +      let obj = merge({}, cb(frame) || {});
> +
> +      if (frames.has(frame)) {
> +        let subframes = {};

Shouldn't this be an array?

@@ +155,5 @@
> +   * the user hitting 'ESC'. In any case we want to collect the currently
> +   * reachable frame tree and store that.
> +   */
> +  onStateChange: function (webProgress, request, stateFlags, status) {
> +    // Ignore state changes for subframes.

Ok, but please document why.

@@ +174,5 @@
> +    } else if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> +      // The document has finished loading and the "load" event will fire soon.
> +      // The load might have also been aborted. Either way we need to recollect
> +      // all reachable frames.
> +      this.collect(webProgress.DOMWindow);

Shouldn't we wait until the end of the load event?

::: browser/components/sessionstore/src/ScrollPosition.jsm
@@ +6,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["ScrollPosition"];
> +
> +const Ci = Components.interfaces;
> +

Side-note: We should start documenting where the code is meant to be executed.
e.g. "This is a child process module."

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2654,5 @@
>        tab.setAttribute("pending", "true");
>  
>        // Update the persistent tab state cache with |tabData| information.
>        TabStateCache.updatePersistent(browser, {
> +        scroll: tabData.scoll || null,

scroll

@@ +2965,1 @@
>      TextAndScrollData.restore(frameList);

Can you convince me that there is no conflict between the two methods that restore scroll position?

@@ +4173,5 @@
> +
> +/**
> + * Keeps track of data that needs to be restored after the tab's document
> + * has been loaded. This includes scroll positions, form data, and page style.
> + */

I like it.
Attachment #8340892 - Flags: review?(dteller) → feedback+
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #18)
> @@ +67,5 @@
> > +  _ifreq: null,
> > +
> > +  // A weakmap of frames mapping to lists of their children. This represents
> > +  // the current frame tree as reachable when the document was loaded.
> > +  _frames: null,
> 
> Could you move these values (and their doc) to where they live, i.e. in the
> constructor?

Done.

> @@ +111,5 @@
> > +  map: function (cb) {
> > +    let frames = this._frames;
> > +
> > +    function walk(frame) {
> > +      let obj = merge({}, cb(frame) || {});
> 
> I don't understand why you need to merge anything here.

Me neither, thanks for catching that. Turns out the whole merge() function is unused now, great :)

> @@ +114,5 @@
> > +    function walk(frame) {
> > +      let obj = merge({}, cb(frame) || {});
> > +
> > +      if (frames.has(frame)) {
> > +        let subframes = {};
> 
> Shouldn't this be an array?

No, that's explicitly an object so that serializing doesn't return empty keys. So it's basically a sparse array in the shape of an object.

> @@ +174,5 @@
> > +    } else if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> > +      // The document has finished loading and the "load" event will fire soon.
> > +      // The load might have also been aborted. Either way we need to recollect
> > +      // all reachable frames.
> > +      this.collect(webProgress.DOMWindow);
> 
> Shouldn't we wait until the end of the load event?

The code firing the (STATE_STOP | STATE_IS_DOCUMENT) listeners notes that this will cause load events to be dispatched. We should collect reachable frames before because we're currently listening for the load event as well to call restoreContent(). There is no guarantee that SessionStore or the web page itself handles the event first. I think it's the right thing to do to keep the old semantics here.

> ::: browser/components/sessionstore/src/ScrollPosition.jsm
> @@ +6,5 @@
> > +
> > +this.EXPORTED_SYMBOLS = ["ScrollPosition"];
> > +
> > +const Ci = Components.interfaces;
> > +
> 
> Side-note: We should start documenting where the code is meant to be
> executed.
> e.g. "This is a child process module."

Good idea.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +2654,5 @@
> >        tab.setAttribute("pending", "true");
> >  
> >        // Update the persistent tab state cache with |tabData| information.
> >        TabStateCache.updatePersistent(browser, {
> > +        scroll: tabData.scoll || null,
> 
> scroll

Good catch, thanks.

> @@ +2965,1 @@
> >      TextAndScrollData.restore(frameList);
> 
> Can you convince me that there is no conflict between the two methods that
> restore scroll position?

These two methods can't conflict other than for manipulated sessionstore.js files. TextAndScrollData.restore() restores scroll positions only for backwards compatibility. We won't put any more scroll positions in tabData.entries[] on the next save.
Attachment #8340892 - Attachment is obsolete: true
Attachment #8341723 - Flags: review?(dteller)
Added a small test to ensure we still support the old scroll position format (stored per shistory entry) for backwards compatibility.
Attachment #8341784 - Attachment is obsolete: true
Attachment #8341784 - Flags: review?(dteller)
Attachment #8341955 - Flags: review?(dteller)
Blocks: 947212
Comment on attachment 8341723 [details] [diff] [review]
0001-Bug-921942-Broadcast-scroll-positions.patch

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

A few nits and a few questions.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +220,5 @@
> +  },
> +
> +  handleEvent: function (event) {
> +    // Don't collect scroll data for frames created at or after the load event
> +    // as SessionStore can't restore scroll data for those.

I suspect that someone, some day, is going to hate us for this choice. It would be a good idea to start a wiki documenting Session Restore and things that we purposedly don't do.

@@ +222,5 @@
> +  handleEvent: function (event) {
> +    // Don't collect scroll data for frames created at or after the load event
> +    // as SessionStore can't restore scroll data for those.
> +    if (gFrameTree.contains(event.target.defaultView)) {
> +      MessageQueue.push("scroll", this.collect);

Nit: I'd rather write |() => this.collect()|. It's currently equivalent, but less likely to break in the future.

::: browser/components/sessionstore/src/FrameTree.jsm
@@ +81,5 @@
> +  },
> +
> +  /**
> +   * Recursively applies the given function |cb| to the stored frame tree.
> +   * The results returned by the callback will be merged and returned. Use

Not merged anymore.

@@ +85,5 @@
> +   * The results returned by the callback will be merged and returned. Use
> +   * this method to collect sessionstore data for all reachable frames stored
> +   * in the frame tree.
> +   *
> +   * @param cb (function)

If we are only interested in functions returning (non-empty) objects, please document this.

@@ +86,5 @@
> +   * this method to collect sessionstore data for all reachable frames stored
> +   * in the frame tree.
> +   *
> +   * @param cb (function)
> +   * @return object

Please document the special field |children| and the fact that we override it.

@@ +95,5 @@
> +    function walk(frame) {
> +      let obj = cb(frame) || {};
> +
> +      if (frames.has(frame)) {
> +        let subframes = {};

Nit: maybe |children| instead of |subframes|?
Also, despite your answer, I still don't understand why it's not an array.

@@ +100,5 @@
> +
> +        frames.get(frame).forEach((subframe, index) => {
> +          let result = walk(subframe, cb);
> +          if (result && Object.keys(result).length) {
> +            subframes[index] = result;

So we can't return, say, a number? What's the rationale?

@@ +156,5 @@
> +      }
> +    } else if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> +      // The document has finished loading and the "load" event will fire soon.
> +      // The load might have also been aborted. Either way we need to recollect
> +      // all reachable frames.

Something doesn't connect in my head:
- if I understand correctly, we want to ensure that we register all the frames that exist at the end of the "load" event, because these are the frames that we can restore;
- the only time we call |collect| is here, and this is before the "load" event.

What am I missing?

::: browser/components/sessionstore/src/ScrollPosition.jsm
@@ +8,5 @@
> +
> +const Ci = Components.interfaces;
> +
> +/**
> + * This is a child process module. It provides methods to collect and restore

Nit: I would put "This is a child process module" on a separate paragraph, after the "It provides ..."

@@ +12,5 @@
> + * This is a child process module. It provides methods to collect and restore
> + * scroll positions for single frames and frame trees.
> + */
> +this.ScrollPosition = Object.freeze({
> +  collect: function (frame) {

Nit: can you document 1/ the type of the argument (i.e. any frame at any position in the hierarchy); 2/ the format of objects returned by this method?

@@ +25,5 @@
> +
> +    return null;
> +  },
> +
> +  restore: function (frame, value) {

Same here. You can reference |collect()|, of course.

@@ +33,5 @@
> +      frame.scrollTo(match[1], match[2]);
> +    }
> +  },
> +
> +  restoreTree: function (root, data) {

Same here.

@@ +34,5 @@
> +    }
> +  },
> +
> +  restoreTree: function (root, data) {
> +    if (data.hasOwnProperty("scroll")) {

Why not "if ('scroll' in data)"?

@@ +45,5 @@
> +
> +    let frames = root.frames;
> +    for (let index of Object.keys(data.children)) {
> +      if (index < frames.length) {
> +        this.restoreTree(frames[index], data.children[index]);

Could you document the behavior if the structure of |root| doesn'tt match that of |data|?

::: browser/components/sessionstore/src/TextAndScrollData.jsm
@@ +77,5 @@
>        if ((content.document.designMode || "") == "on" && content.document.body) {
>          entry.innerHTML = content.document.body.innerHTML;
>        }
>      }
>  

So, do I understand correctly that this module is meant to be renamed to just TextData at some point in the future?
Attachment #8341723 - Flags: feedback+
Comment on attachment 8341955 [details] [diff] [review]
0002-Bug-921942-Add-FrameTree-and-scroll-position-broadca.patch

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

r+ if I misunderstood and we want to ignore frames created during the load event

::: browser/components/sessionstore/test/browser_483330.js
@@ -1,2 @@
> -function test() {
> -  /** Test for Bug 483330 **/

Out of curiosity, why do we retire this test?

::: browser/components/sessionstore/test/browser_frametree.js
@@ +32,5 @@
> +  yield sendMessage(browser, "ss-test:createDynamicFrames", {url: URL});
> +  browser.loadURI(URL_FRAMESET);
> +  yield promiseResetAndLoad(browser);
> +  yield checkFrameTree(browser, FRAME_TREE_FRAMESET,
> +    "dynamic frames created on or after the load event are ignored");

I thought we had concluded that we only wanted to ignore frames created after the load event?

@@ +99,5 @@
> +  return sendMessage(browser, "ss-test:mapFrameTree").then(tree => {
> +    is(JSON.stringify(tree), JSON.stringify(expected), msg);
> +  });
> +}
> +

Nit: Can you document these two functions?

::: browser/components/sessionstore/test/browser_scrollPositions.js
@@ +16,5 @@
> +  let browser = tab.linkedBrowser;
> +  yield promiseBrowserLoaded(browser);
> +
> +  // Scroll down a little.
> +  yield sendMessage(browser, "ss-test:setScrollPosition", {x: 1100, y: 1200});

Nit: could you turn these magic numbers into named constants and perhaps randomize them a little to ensure that we have few chances of false positives?

@@ +40,5 @@
> +  checkScroll(tab2, null, "force-reload resets scroll positions");
> +
> +  // Scroll back to the top and check that the position has been reset.
> +  yield sendMessage(browser, "ss-test:setScrollPosition", {x: 0, y: 0});
> +  checkScroll(tab, null, "no scroll stored");

Nit: maybe a small comment to explain why this works.

::: browser/components/sessionstore/test/content.js
@@ +61,5 @@
>  });
> +
> +addMessageListener("ss-test:getScrollPosition", function (msg) {
> +  let frame = content;
> +  if (msg.data.hasOwnProperty("frame")) {

Why not |(frame in msg.data)|?

@@ +80,5 @@
> +});
> +
> +addMessageListener("ss-test:createDynamicFrames", function (msg) {
> +  function createIFrame(rows) {
> +    let frames = content.document.getElementById("frames");

Why hardcode id "frames"?
Attachment #8341955 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] <currently on training, will be back on Friday 6th> from comment #22)
> > +  handleEvent: function (event) {
> > +    // Don't collect scroll data for frames created at or after the load event
> > +    // as SessionStore can't restore scroll data for those.
> 
> I suspect that someone, some day, is going to hate us for this choice. It
> would be a good idea to start a wiki documenting Session Restore and things
> that we purposedly don't do.

I agree but this isn't really something we chose to do. We only want to stay compatible with what was chosen a few years ago and so far not too many people (no one?) seemed to complain. Nevertheless documenting all this would be great indeed.

> > +    function walk(frame) {
> > +      let obj = cb(frame) || {};
> > +
> > +      if (frames.has(frame)) {
> > +        let subframes = {};
> 
> Nit: maybe |children| instead of |subframes|?
> Also, despite your answer, I still don't understand why it's not an array.

Ok, imagine that we have a root frame with five subframes (e.g. a page with five iframes). Now only the third child has changed its scroll position which means we only want to store scroll information for that one. Using an array it would look like this:

{children: [null, null, {scroll: "100,200"}, null, null]}

With only one child of ten having scroll data, there would be even more overhead. Now using an object we can do it like this:

{children: {2: {scroll: "100,200"}}}

That was the idea behind it. If you think that win is negligible and we should rather use an array here I would be okay with that, too. At least when switching to differential updates in the future (where we only collect data for the frames that changed) this should be a good format.

> > +        frames.get(frame).forEach((subframe, index) => {
> > +          let result = walk(subframe, cb);
> > +          if (result && Object.keys(result).length) {
> > +            subframes[index] = result;
> 
> So we can't return, say, a number? What's the rationale?

The rationale here is that if every frame may have subframes, we can't just store a single value in the tree returned by map(). We need a "children" property that may contain data for possible subframes and every other property may then hold actual information about the frame itself.

> > +    } else if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
> > +      // The document has finished loading and the "load" event will fire soon.
> > +      // The load might have also been aborted. Either way we need to recollect
> > +      // all reachable frames.
> 
> Something doesn't connect in my head:
> - if I understand correctly, we want to ensure that we register all the
> frames that exist at the end of the "load" event, because these are the
> frames that we can restore;
> - the only time we call |collect| is here, and this is before the "load"
> event.
> 
> What am I missing?

We're only interested in everything created *before* the load event has fired. SessionStore is only one of the possible load listeners and we can never ensure to be called before or after all of the content page has done its work. That *might* destroy some cases where it currently works but this actually removes some non-determinism where sometimes it might fail just because SessionStore is called before the page's load listeners that create the frame.

If we wanted to include all frames created before and when "load" is dispatched we should use a "readystatechange" listener to collect frame data and also to restore document data. That might make more sense but would change the current semantics slightly - although only in a way that we would collect more data instead of less.

> > +  restoreTree: function (root, data) {
> > +    if (data.hasOwnProperty("scroll")) {
> 
> Why not "if ('scroll' in data)"?

Personal preference, no hard reasons.

> ::: browser/components/sessionstore/src/TextAndScrollData.jsm
> @@ +77,5 @@
> >        if ((content.document.designMode || "") == "on" && content.document.body) {
> >          entry.innerHTML = content.document.body.innerHTML;
> >        }
> >      }
> >  
> 
> So, do I understand correctly that this module is meant to be renamed to
> just TextData at some point in the future?

Yes, I meant to do that in bug 947212.
Attachment #8341723 - Attachment is obsolete: true
Attachment #8344555 - Flags: review?(dteller)
(In reply to David Rajchenbach Teller [:Yoric] <currently on training, will be back on Friday 6th> from comment #23)
> r+ if I misunderstood and we want to ignore frames created during the load
> event

Yes, that is the current plan.

> > -function test() {
> > -  /** Test for Bug 483330 **/
> 
> Out of curiosity, why do we retire this test?

That's a very simple test that tests collection and restoration of scroll positions. This is now all covered by browser_scrollPositions.js.

> > +  yield sendMessage(browser, "ss-test:createDynamicFrames", {url: URL});
> > +  browser.loadURI(URL_FRAMESET);
> > +  yield promiseResetAndLoad(browser);
> > +  yield checkFrameTree(browser, FRAME_TREE_FRAMESET,
> > +    "dynamic frames created on or after the load event are ignored");
> 
> I thought we had concluded that we only wanted to ignore frames created
> after the load event?

See my previous comment wrt. the readystatechange event.

> > +addMessageListener("ss-test:getScrollPosition", function (msg) {
> > +  let frame = content;
> > +  if (msg.data.hasOwnProperty("frame")) {
> 
> Why not |(frame in msg.data)|?

Personal preference, no hard reasons.

> @@ +80,5 @@
> > +});
> > +
> > +addMessageListener("ss-test:createDynamicFrames", function (msg) {
> > +  function createIFrame(rows) {
> > +    let frames = content.document.getElementById("frames");
> 
> Why hardcode id "frames"?

Laziness :) Fixed that.
Attachment #8341955 - Attachment is obsolete: true
Attachment #8344563 - Flags: review+
(In reply to Tim Taubert [:ttaubert] from comment #24)
> > I suspect that someone, some day, is going to hate us for this choice. It
> > would be a good idea to start a wiki documenting Session Restore and things
> > that we purposedly don't do.
> 
> I agree but this isn't really something we chose to do. We only want to stay
> compatible with what was chosen a few years ago and so far not too many
> people (no one?) seemed to complain. Nevertheless documenting all this would
> be great indeed.

I created the page here:
https://wiki.mozilla.org/Firefox/session_restore

Your turn :)

> Ok, imagine that we have a root frame with five subframes (e.g. a page with
> five iframes). Now only the third child has changed its scroll position
> which means we only want to store scroll information for that one. Using an
> array it would look like this:
> 
> {children: [null, null, {scroll: "100,200"}, null, null]}
> 
> With only one child of ten having scroll data, there would be even more
> overhead. Now using an object we can do it like this:
> 
> {children: {2: {scroll: "100,200"}}}
> 
> That was the idea behind it. If you think that win is negligible and we
> should rather use an array here I would be okay with that, too. At least
> when switching to differential updates in the future (where we only collect
> data for the frames that changed) this should be a good format.

Ok, now that makes sense. Could you document this?

> > > +        frames.get(frame).forEach((subframe, index) => {
> > > +          let result = walk(subframe, cb);
> > > +          if (result && Object.keys(result).length) {
> > > +            subframes[index] = result;
> > 
> > So we can't return, say, a number? What's the rationale?
> 
> The rationale here is that if every frame may have subframes, we can't just
> store a single value in the tree returned by map(). We need a "children"
> property that may contain data for possible subframes and every other
> property may then hold actual information about the frame itself.

I'm not a huge fan of that, but I don't want us to waste time arguing on this, so do whatever you feel is best. Just don't forget to document this requirement.

> > Something doesn't connect in my head:
> > - if I understand correctly, we want to ensure that we register all the
> > frames that exist at the end of the "load" event, because these are the
> > frames that we can restore;
> > - the only time we call |collect| is here, and this is before the "load"
> > event.
> > 
> > What am I missing?
> 
> We're only interested in everything created *before* the load event has
> fired. SessionStore is only one of the possible load listeners and we can
> never ensure to be called before or after all of the content page has done
> its work. That *might* destroy some cases where it currently works but this
> actually removes some non-determinism where sometimes it might fail just
> because SessionStore is called before the page's load listeners that create
> the frame.
> 
> If we wanted to include all frames created before and when "load" is
> dispatched we should use a "readystatechange" listener to collect frame data
> and also to restore document data. That might make more sense but would
> change the current semantics slightly - although only in a way that we would
> collect more data instead of less.

I have run more experiments:
- inserted with a |defer='true'| script => remembers form data but not frame history;
- inserted during |DOMContentLoaded| => remembers form data but not frame history;
- inserted during |load| => remembers nothing;
- inserted later => remembers nothing.

If we don't want to break compatibility, we should make efforts to wait until all listeners of DOMContentLoaded have returned. How difficult is this?

> > Why not "if ('scroll' in data)"?
> 
> Personal preference, no hard reasons.

I prefer |in|, but I won't insist.
Comment on attachment 8344555 [details] [diff] [review]
0001-Bug-921942-Broadcast-scroll-positions.patch

Cancelling r? until we have finished the discussion on loading.
Attachment #8344555 - Flags: review?(dteller)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #26)
> > > > +        frames.get(frame).forEach((subframe, index) => {
> > > > +          let result = walk(subframe, cb);
> > > > +          if (result && Object.keys(result).length) {
> > > > +            subframes[index] = result;
> > > 
> > > So we can't return, say, a number? What's the rationale?
> > 
> > The rationale here is that if every frame may have subframes, we can't just
> > store a single value in the tree returned by map(). We need a "children"
> > property that may contain data for possible subframes and every other
> > property may then hold actual information about the frame itself.
> 
> I'm not a huge fan of that, but I don't want us to waste time arguing on
> this, so do whatever you feel is best. Just don't forget to document this
> requirement.

Do you have a better idea? How else could we have a hierarchic structure of frames that holds one or multiple values per frame?

> I have run more experiments:
> - inserted with a |defer='true'| script => remembers form data but not frame
> history;
> - inserted during |DOMContentLoaded| => remembers form data but not frame
> history;
> - inserted during |load| => remembers nothing;
> - inserted later => remembers nothing.
> 
> If we don't want to break compatibility, we should make efforts to wait
> until all listeners of DOMContentLoaded have returned. How difficult is this?

So how about waiting for document.readyState=complete (i.e. the readystatechange event)? That is fired after all load event listeners have been processed. If a load listener adds an iframe that immediately starts loading this will defer the active document's load event and thus also the readystatechange event.
Flags: needinfo?(dteller)
(In reply to Tim Taubert [:ttaubert] from comment #28)
> > I'm not a huge fan of that, but I don't want us to waste time arguing on
> > this, so do whatever you feel is best. Just don't forget to document this
> > requirement.
> 
> Do you have a better idea? How else could we have a hierarchic structure of
> frames that holds one or multiple values per frame?

Not sure.


Side-note, while I'm thinking about it:
> Ok, imagine that we have a root frame with five subframes (e.g. a page with
> five iframes). Now only the third child has changed its scroll position
> which means we only want to store scroll information for that one. Using an
> array it would look like this:
> 
> {children: [null, null, {scroll: "100,200"}, null, null]}
> 
> With only one child of ten having scroll data, there would be even more
> overhead. Now using an object we can do it like this:
> 
> {children: {2: {scroll: "100,200"}}}
> 
> That was the idea behind it. If you think that win is negligible and we
> should rather use an array here I would be okay with that, too. At least
> when switching to differential updates in the future (where we only collect
> data for the frames that changed) this should be a good format.

I'm almost sure that using arrays to represent sparse arrays is going to be faster than encoding them with objects. Serializing/deserializing arrays is much faster than general objects, plus I'm almost sure that they take less memory.

> > I have run more experiments:
> > - inserted with a |defer='true'| script => remembers form data but not frame
> > history;
> > - inserted during |DOMContentLoaded| => remembers form data but not frame
> > history;
> > - inserted during |load| => remembers nothing;
> > - inserted later => remembers nothing.
> > 
> > If we don't want to break compatibility, we should make efforts to wait
> > until all listeners of DOMContentLoaded have returned. How difficult is this?
> 
> So how about waiting for document.readyState=complete (i.e. the
> readystatechange event)? That is fired after all load event listeners have
> been processed. If a load listener adds an iframe that immediately starts
> loading this will defer the active document's load event and thus also the
> readystatechange event.

That sounds good. When you say "load event listeners", is that "onload" or "onDOMContentLoaded"?
Flags: needinfo?(dteller)
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #29)
> I'm almost sure that using arrays to represent sparse arrays is going to be
> faster than encoding them with objects. Serializing/deserializing arrays is
> much faster than general objects, plus I'm almost sure that they take less
> memory.

Hmm. Ok, let's go with arrays then.

> > So how about waiting for document.readyState=complete (i.e. the
> > readystatechange event)? That is fired after all load event listeners have
> > been processed. If a load listener adds an iframe that immediately starts
> > loading this will defer the active document's load event and thus also the
> > readystatechange event.
> 
> That sounds good. When you say "load event listeners", is that "onload" or
> "onDOMContentLoaded"?

"onload". DOMContentLoaded is dispatched when we're done parsing but doesn't wait for subframes to load.
(In reply to Tim Taubert [:ttaubert] from comment #30)
> > That sounds good. When you say "load event listeners", is that "onload" or
> > "onDOMContentLoaded"?
> 
> "onload". DOMContentLoaded is dispatched when we're done parsing but doesn't
> wait for subframes to load.

Sounds good to me. Can you think of any drawback?
No, I can't think of any. This should enable us to have a well-defined (and easy to talk about) set of frames that we collect data for. Additionally we won't save any less data than we currently do.
Changed from STATE_STOP to a readystatechange listener. Switched to using a sparse array for subframe data. Changed the frame tree format slightly to handle destroyed frames better.
Attachment #8344555 - Attachment is obsolete: true
Attachment #8347185 - Flags: review?(dteller)
Test suite adapted to new subframe structure and added two small tests for dead frames.
Attachment #8344563 - Attachment is obsolete: true
Attachment #8347194 - Flags: review+
Comment on attachment 8347185 [details] [diff] [review]
0001-Bug-921942-Broadcast-scroll-positions.patch

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

By the way, could you give version numbers to your patches? This would simplify interdiff.

::: browser/components/sessionstore/src/FrameTree.jsm
@@ +39,5 @@
> + */
> +function FrameTreeInternal({addEventListener, docShell}) {
> +  // A weakmap of frames mapping to their initial indices in their parents'
> +  // child lists. This represents the current frame tree as reachable when the
> +  // wdocument was loaded.

I believe that this is the most difficult part of this module, so this would deserve more documentation, including an example.

@@ +163,5 @@
> +  handleEvent: function ({target}) {
> +    let doc = this.content.document;
> +
> +    // The document and its resources have finished loading. The active
> +    // document's load event can't be any longer delayed has been fired.

Nit: "can't be delayed any longer."
Actually, I believe that "The active document's load event has been fired and has returned." should be sufficient, shouldn't it?

@@ +187,5 @@
> +      return;
> +    }
> +
> +    // Ignore notifications about our document having finished loading.
> +    if (!(stateFlags & Ci.nsIWebProgressListener.STATE_START)) {

Are we really interested in STATE_START? Not in STATE_STOP?
Attachment #8347185 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #35)
> By the way, could you give version numbers to your patches? This would
> simplify interdiff.

Yeah sorry. This occurred to me as well.

> > +    // Ignore notifications about our document having finished loading.
> > +    if (!(stateFlags & Ci.nsIWebProgressListener.STATE_START)) {
> 
> Are we really interested in STATE_START? Not in STATE_STOP?

Yes, that is the part the listens for STATE_START and resets the frame tree as soon as we start loading a new document. The "stop loading" part is handled by readystatechange now.
Ah, right.
Can you just detail this slightly in the doc?
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #37)
> Ah, right.
> Can you just detail this slightly in the doc?

Yes, I will write all of this down.
I ran it on try a few times and saw intermittent failures that were caused by faulty tests and invalid assumptions. Anything created at the load event doesn't delay it, I must have read the docs wrong. [1]

(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #26)
> I have run more experiments:
> - inserted with a |defer='true'| script => remembers form data but not frame
> history;
> - inserted during |DOMContentLoaded| => remembers form data but not frame
> history;
> - inserted during |load| => remembers nothing;
> - inserted later => remembers nothing.
> 
> If we don't want to break compatibility, we should make efforts to wait
> until all listeners of DOMContentLoaded have returned. How difficult is this?

I reverted the patch and removed the "readystatechange" listener. As that is immediately fired before "load" we can just use the STATE_STOP notification. By doing this we will not break compatibility because we will remember all frames created before "load", e.g. created at DOMContentLoaded.

It has equally easy semantics, every frame that was created and started loading before "load" was fired will be included. Every frame inserted and loading before "load" will also delay the load event so that's a good hook I think.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#delay-the-load-event
Attachment #8347185 - Attachment is obsolete: true
Attachment #8347595 - Flags: review?(dteller)
Do I understand correctly that DOMContentLoaded delays the load event?
Flags: needinfo?(ttaubert)
Not DOMContentLoaded specifically but everything done before "load" is fired. Which includes frames created and starting to load at DOMContentLoaded.
Flags: needinfo?(ttaubert)
Comment on attachment 8347595 [details] [diff] [review]
0001-Bug-921942-Broadcast-scroll-positions-r-yoric.patch (v7)

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

::: browser/components/sessionstore/src/FrameTree.jsm
@@ +166,5 @@
> +   * @see nsIWebProgressListener.onStateChange
> +   *
> +   * We want to be notified about new documents that start loading to clear
> +   * the current frame tree and about completed document loads to recollect
> +   * reachable frames.

Nit: 
«
We want to be notified about:
- new documents that start loading to clear the current frame tree;
- completed document loads to recollect reachable frames.
»

I believe that this is more readable.

@@ +182,5 @@
> +      this._frames.clear();
> +
> +      // Notify observers that the frame tree has been reset.
> +      for (let obs of this._observers) {
> +        obs.onFrameTreeReset();

I wonder: does this slow down the loading/display/execution of the web page? Can this be setTimeout()-ed?
Attachment #8347595 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #42)
> @@ +182,5 @@
> > +      this._frames.clear();
> > +
> > +      // Notify observers that the frame tree has been reset.
> > +      for (let obs of this._observers) {
> > +        obs.onFrameTreeReset();
> 
> I wonder: does this slow down the loading/display/execution of the web page?
> Can this be setTimeout()-ed?

Only if observers would do heavy stuff when being notified. All the ScrollPositionListener does (and further components will do) is push a "null" message onto the message queue.
https://hg.mozilla.org/mozilla-central/rev/cf11083b4786
https://hg.mozilla.org/mozilla-central/rev/b42216b98522
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Depends on: 952401
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: