Broadcast session history

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Let's broadcast session history, get rid of the duplicate tab state caches, fillTabCachesAsynchronously(), etc.
(Assignee)

Comment 1

4 years ago
Created attachment 8362581 [details] [diff] [review]
0001-Bug-960903-Remove-UserTypedValueChanged-event.patch

We needed this only to invalidate the TabStateCache but we won't need it anymore so I think it would be fine to remove it. MXR doesn't yield any add-on consumers either.
Attachment #8362581 - Flags: review?(dao)
(Assignee)

Comment 2

4 years ago
Created attachment 8362586 [details] [diff] [review]
0002-Bug-960903-Broadcast-session-history-data.patch

\o/

This is the last part of the broadcasting series and gets rid of all the superfluous code parts.

1) fillTabCachesAsynchronously() won't be needed anymore.
2) TabStateCache is a lot smaller and simpler now. No two caches anymore.
3) Collecting tab state is now as easy as merging information about the tab with all the data collected by frame scripts and persisted in the TabStateCache.
4) We don't need Messenger.jsm anymore.
5) We don't need any cache hit/miss telemetry for the TabStateCache anymore.
6) The awkward Utils.copy() method is gone.

Tests are passing with some small modifications. The only thing I need to work on is a test especially for session history and all the possible invalidations. Will post that in a third patch.
Attachment #8362586 - Flags: review?(dteller)

Updated

4 years ago
Attachment #8362581 - Flags: review?(dao) → review+
Comment on attachment 8362586 [details] [diff] [review]
0002-Bug-960903-Broadcast-session-history-data.patch

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

Wow, that looks simpler.
However, I don't understand everything yet.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +216,5 @@
> + */
> +let SessionHistoryListener = {
> +  init: function () {
> +    gFrameTree.addObserver(this);
> +    addEventListener("hashchange", this, true);

So, just to be sure I understand:
- gFrameTree.addObserver(this) ensures that we're informed whenever the iframe structure changes;
- "hashchange" ensures that we're informed whenever we browse inside an and change only the hash.

Is that correct? Does this include pushState?

@@ +225,5 @@
> +    setTimeout(() => this.collect(), 0);
> +  },
> +
> +  handleEvent: function () {
> +    this.collect();

Why do we setTimeout() with observe() and not with handleEvent()?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ -741,5 @@
>          let otherBrowser = aEvent.detail;
>          TabState.onBrowserContentsSwapped(browser, otherBrowser);
>          TabStateCache.onBrowserContentsSwapped(browser, otherBrowser);
>          break;
> -      case "UserTypedValueChanged":

Is that change related to the patch?

@@ +733,5 @@
>          break;
>        case "TabHide":
>          this.onTabHide(win, aEvent.originalTarget);
>          break;
>        case "TabPinned":

// Fall through

@@ -773,5 @@
>        case "TabPinned":
> -        // If possible, update cached data without having to invalidate it
> -        TabStateCache.updateField(aEvent.originalTarget, "pinned", true);
> -        this.saveStateDelayed(win);
> -        break;

Do we now broadcast TabPinned/TabUnpinned stuff?

@@ -1489,5 @@
>        this.restoreNextTab();
>      }
>  
> -    // If possible, update cached data without having to invalidate it
> -    TabStateCache.updateField(aTab, "hidden", false);

Do we now broadcast "hidden"?

@@ +1462,5 @@
>    onGatherTelemetry: function() {
>      // On the first gather-telemetry notification of the session,
>      // gather telemetry data.
>      Services.obs.removeObserver(this, "gather-telemetry");
> +    let stateString = SessionStore.getBrowserState();

\o/

@@ -1875,5 @@
> -      if (Object.keys(deleteFrom).length) {
> -        TabStateCache.updateField(aTab, "extData", deleteFrom);
> -      } else {
> -        TabStateCache.removeField(aTab, "extData");
> -      }

I don't follow these extData changes. Do we have a new API that lets us handle extData separately?

::: browser/components/sessionstore/src/TabState.jsm
@@ -130,5 @@
> -    // Data collected while docShells have been swapped should not go into
> -    // the TabStateCache. Collections will most probably time out but we want
> -    // to make sure.
> -    this.dropPendingCollections(browser);
> -    this.dropPendingCollections(otherBrowser);

So, can you explain why we don't need this mechanism anymore?
Attachment #8362586 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 4

4 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #3)
> Comment on attachment 8362586 [details] [diff] [review]
> ::: browser/components/sessionstore/content/content-sessionStore.js
> @@ +216,5 @@
> > + */
> > +let SessionHistoryListener = {
> > +  init: function () {
> > +    gFrameTree.addObserver(this);
> > +    addEventListener("hashchange", this, true);
> 
> So, just to be sure I understand:
> - gFrameTree.addObserver(this) ensures that we're informed whenever the
> iframe structure changes;

Yes, we are notified when a new load starts and again when that load has finished.

> - "hashchange" ensures that we're informed whenever we browse inside an and
> change only the hash.

Yes, we visibly only change the change scroll position and add a new history entry when clicking a #anchor link, that's why we listen for the event to update shistory data.

> Does this include pushState?

No, it doesn't. We never supported pushState directly. If the tab wasn't invalidated by something else we didn't collect shistory data after pushState() or replaceState() calls. There unfortunately is no easy way to know as these methods don't fire any events. When moving to differential updates for shistory we might want to extend nsISHistoryListener and be sure to cover those as well. We can also cover this earlier in a follow-up.

> @@ +225,5 @@
> > +    setTimeout(() => this.collect(), 0);
> > +  },
> > +
> > +  handleEvent: function () {
> > +    this.collect();
> 
> Why do we setTimeout() with observe() and not with handleEvent()?

The setTimeout() here is only because we listen for the "browser:purge-session-history" notification. That notification is sent and all observers are then expected to clean their data. We can't ensure to be called after browser.xml's observer that clears session history and the MessageQueue could flush in the same tick. handleEvent() is for onhashchange that is fired after the shistory entry has been added.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ -741,5 @@
> >          let otherBrowser = aEvent.detail;
> >          TabState.onBrowserContentsSwapped(browser, otherBrowser);
> >          TabStateCache.onBrowserContentsSwapped(browser, otherBrowser);
> >          break;
> > -      case "UserTypedValueChanged":
> 
> Is that change related to the patch?

Yes, we don't need it anymore as we're recollecting "cheap tab data" every time.

> @@ +733,5 @@
> >          break;
> >        case "TabHide":
> >          this.onTabHide(win, aEvent.originalTarget);
> >          break;
> >        case "TabPinned":
> 
> // Fall through

Yes, do you really want me to denote that? That seems like a superflous comment. If TabPinned would do anything besides just being a label I would agree, but here...

> @@ -773,5 @@
> >        case "TabPinned":
> > -        // If possible, update cached data without having to invalidate it
> > -        TabStateCache.updateField(aEvent.originalTarget, "pinned", true);
> > -        this.saveStateDelayed(win);
> > -        break;
> 
> Do we now broadcast TabPinned/TabUnpinned stuff?

No, cheap tab data is collected every time when querying the tab state.

> @@ -1489,5 @@
> >        this.restoreNextTab();
> >      }
> >  
> > -    // If possible, update cached data without having to invalidate it
> > -    TabStateCache.updateField(aTab, "hidden", false);
> 
> Do we now broadcast "hidden"?

Same here.

> @@ -1875,5 @@
> > -      if (Object.keys(deleteFrom).length) {
> > -        TabStateCache.updateField(aTab, "extData", deleteFrom);
> > -      } else {
> > -        TabStateCache.removeField(aTab, "extData");
> > -      }
> 
> I don't follow these extData changes. Do we have a new API that lets us
> handle extData separately?

Same thing about cheap data.

> ::: browser/components/sessionstore/src/TabState.jsm
> @@ -130,5 @@
> > -    // Data collected while docShells have been swapped should not go into
> > -    // the TabStateCache. Collections will most probably time out but we want
> > -    // to make sure.
> > -    this.dropPendingCollections(browser);
> > -    this.dropPendingCollections(otherBrowser);
> 
> So, can you explain why we don't need this mechanism anymore?

We don't have background collections anymore. Tab data is broadcasted by the frame scripts themselves and will eventually be recorded in the TabStateCache. The notion of an explicit async collection is gone. This was only a guard against sync collections happening while we kicked off a background collection that sent data using async messages. SHistory is the last part that currently still uses it and that's gone now with the patch.
(Assignee)

Comment 5

4 years ago
Comment on attachment 8362586 [details] [diff] [review]
0002-Bug-960903-Broadcast-session-history-data.patch

Re-requesting review as I claim there is nothing to change for that patch with regard to your comments :)
Attachment #8362586 - Flags: review?(dteller)
(In reply to Tim Taubert [:ttaubert] from comment #4)
> > @@ +225,5 @@
> > > +    setTimeout(() => this.collect(), 0);
> > > +  },
> > > +
> > > +  handleEvent: function () {
> > > +    this.collect();
> > 
> > Why do we setTimeout() with observe() and not with handleEvent()?
> 
> The setTimeout() here is only because we listen for the
> "browser:purge-session-history" notification. That notification is sent and
> all observers are then expected to clean their data. We can't ensure to be
> called after browser.xml's observer that clears session history and the
> MessageQueue could flush in the same tick. handleEvent() is for onhashchange
> that is fired after the shistory entry has been added.

Nit: Can you please document the setTimeout(), in that case?

> > @@ +733,5 @@
> > >          break;
> > >        case "TabHide":
> > >          this.onTabHide(win, aEvent.originalTarget);
> > >          break;
> > >        case "TabPinned":
> > 
> > // Fall through
> 
> Yes, do you really want me to denote that? That seems like a superflous
> comment. If TabPinned would do anything besides just being a label I would
> agree, but here...

Ok, fair enough.

> No, cheap tab data is collected every time when querying the tab state.

Can you document all of this in the sessionstore wiki?
> 
> > ::: browser/components/sessionstore/src/TabState.jsm
> > @@ -130,5 @@
> > > -    // Data collected while docShells have been swapped should not go into
> > > -    // the TabStateCache. Collections will most probably time out but we want
> > > -    // to make sure.
> > > -    this.dropPendingCollections(browser);
> > > -    this.dropPendingCollections(otherBrowser);
> > 
> > So, can you explain why we don't need this mechanism anymore?
> 
> We don't have background collections anymore. Tab data is broadcasted by the
> frame scripts themselves and will eventually be recorded in the
> TabStateCache. The notion of an explicit async collection is gone. This was
> only a guard against sync collections happening while we kicked off a
> background collection that sent data using async messages. SHistory is the
> last part that currently still uses it and that's gone now with the patch.

Ok, fair enough.
Comment on attachment 8362586 [details] [diff] [review]
0002-Bug-960903-Broadcast-session-history-data.patch

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

r+, with the trivial docnit mentioned in my previous comment.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +216,5 @@
> + */
> +let SessionHistoryListener = {
> +  init: function () {
> +    gFrameTree.addObserver(this);
> +    addEventListener("hashchange", this, true);

So, just to be sure I understand:
- gFrameTree.addObserver(this) ensures that we're informed whenever the iframe structure changes;
- "hashchange" ensures that we're informed whenever we browse inside an and change only the hash.

Is that correct? Does this include pushState?

@@ +225,5 @@
> +    setTimeout(() => this.collect(), 0);
> +  },
> +
> +  handleEvent: function () {
> +    this.collect();

Why do we setTimeout() with observe() and not with handleEvent()?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ -741,5 @@
>          let otherBrowser = aEvent.detail;
>          TabState.onBrowserContentsSwapped(browser, otherBrowser);
>          TabStateCache.onBrowserContentsSwapped(browser, otherBrowser);
>          break;
> -      case "UserTypedValueChanged":

Is that change related to the patch?

@@ +733,5 @@
>          break;
>        case "TabHide":
>          this.onTabHide(win, aEvent.originalTarget);
>          break;
>        case "TabPinned":

// Fall through

@@ -773,5 @@
>        case "TabPinned":
> -        // If possible, update cached data without having to invalidate it
> -        TabStateCache.updateField(aEvent.originalTarget, "pinned", true);
> -        this.saveStateDelayed(win);
> -        break;

Do we now broadcast TabPinned/TabUnpinned stuff?

@@ -1489,5 @@
>        this.restoreNextTab();
>      }
>  
> -    // If possible, update cached data without having to invalidate it
> -    TabStateCache.updateField(aTab, "hidden", false);

Do we now broadcast "hidden"?

@@ +1462,5 @@
>    onGatherTelemetry: function() {
>      // On the first gather-telemetry notification of the session,
>      // gather telemetry data.
>      Services.obs.removeObserver(this, "gather-telemetry");
> +    let stateString = SessionStore.getBrowserState();

\o/

@@ -1875,5 @@
> -      if (Object.keys(deleteFrom).length) {
> -        TabStateCache.updateField(aTab, "extData", deleteFrom);
> -      } else {
> -        TabStateCache.removeField(aTab, "extData");
> -      }

I don't follow these extData changes. Do we have a new API that lets us handle extData separately?

::: browser/components/sessionstore/src/TabState.jsm
@@ -130,5 @@
> -    // Data collected while docShells have been swapped should not go into
> -    // the TabStateCache. Collections will most probably time out but we want
> -    // to make sure.
> -    this.dropPendingCollections(browser);
> -    this.dropPendingCollections(otherBrowser);

So, can you explain why we don't need this mechanism anymore?
Attachment #8362586 - Flags: review?(dteller)
Attachment #8362586 - Flags: review+
Attachment #8362586 - Flags: feedback+
(Assignee)

Comment 8

4 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #6)
> > The setTimeout() here is only because we listen for the
> > "browser:purge-session-history" notification. That notification is sent and
> > all observers are then expected to clean their data. We can't ensure to be
> > called after browser.xml's observer that clears session history and the
> > MessageQueue could flush in the same tick. handleEvent() is for onhashchange
> > that is fired after the shistory entry has been added.
> 
> Nit: Can you please document the setTimeout(), in that case?

Sure.

> > No, cheap tab data is collected every time when querying the tab state.
> 
> Can you document all of this in the sessionstore wiki?

Yes, all of that really needs to be written down.
(Assignee)

Comment 9

4 years ago
Created attachment 8363609 [details] [diff] [review]
0003-Bug-960903-Listen-for-subframe-loads-and-clear-obser.patch

Some fixes I discovered while writing tests.
Attachment #8363609 - Flags: review?(dteller)
(Assignee)

Comment 10

4 years ago
Created attachment 8363610 [details] [diff] [review]
0004-Bug-960903-Tests-for-broadcasting-shistory.patch

Tests.
Attachment #8363610 - Flags: review?(dteller)
Comment on attachment 8363609 [details] [diff] [review]
0003-Bug-960903-Listen-for-subframe-loads-and-clear-obser.patch

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

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +238,5 @@
> +  handleEvent: function (event) {
> +    // We are only interested in "load" events from subframes.
> +    if (event.type != "load" || event.target != content.document) {
> +      this.collect();
> +    }

By event.type != "load", do you mean event.type == "hashchange"?
Attachment #8363609 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 12

4 years ago
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #11)
> > +  handleEvent: function (event) {
> > +    // We are only interested in "load" events from subframes.
> > +    if (event.type != "load" || event.target != content.document) {
> > +      this.collect();
> > +    }
> 
> By event.type != "load", do you mean event.type == "hashchange"?

Yes, if you like that better :)
Comment on attachment 8363610 [details] [diff] [review]
0004-Bug-960903-Tests-for-broadcasting-shistory.patch

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

Looks good.
Attachment #8363610 - Flags: review?(dteller) → review+
(In reply to Tim Taubert [:ttaubert] from comment #12)
> (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> Yes, if you like that better :)

I do :)
(Assignee)

Comment 15

4 years ago
Created attachment 8364640 [details] [diff] [review]
0003-Bug-960903-Listen-for-subframe-loads-and-clear-obser.patch, v2

Changed to event.type == "hashchange".
Attachment #8363609 - Attachment is obsolete: true
Attachment #8364640 - Flags: review?(dteller)
Comment on attachment 8364640 [details] [diff] [review]
0003-Bug-960903-Listen-for-subframe-loads-and-clear-obser.patch, v2

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

Here we go
Attachment #8364640 - Flags: review?(dteller) → review+
Blocks: 916496
(Assignee)

Updated

4 years ago
Depends on: 965218
(Assignee)

Updated

4 years ago
Depends on: 967028

Updated

4 years ago
Depends on: 970965
Blocks: 700923

Updated

4 years ago
Depends on: 1034296
You need to log in before you can comment on or make changes to this bug.