Closed Bug 930967 Opened 6 years ago Closed 6 years ago

Let tabs broadcast data when it changes instead of collecting it through the main process

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 9 obsolete files)

42.32 KB, patch
billm
: review+
smacleod
: review+
Details | Diff | Splinter Review
48.84 KB, patch
smacleod
: review+
Details | Diff | Splinter Review
The main idea here is that tabs should broadcast their data instead of the main process having to go and collect whenever it detects an invalidation. Especially in an e10s world the content script is a lot better at detecting changes. We should not have to send messages that trigger invalidations and then again messages that kick off data collection.
Sorry for all the TODOs in the patch, they are only a placeholder for me to remind me to add comments there. I think the patch requires some explanations:

1) I started with DOMSessionStorage because that seems easy for now. Finally we of course want to broadcast all tab data like that. We will then have listeners and observers in the content script that know exactly when we need to re-collect data.

2) Whenever data changes, a message is put in the queue. At most every second (or whatever we agree on) there is one message sending fresh data to the chrome process. Data is thus sent in batches to act as a damper should there be lots of changes.

3) PermaTabStateCache is an ugly name but it works for now :) It's basically another tab state cache that we will never clear. We only read and update it. When all data collection has been migrated to broadcasting we can name it TabStateCache and remove the old one.

4) Currently, DOMSessionStorage data changes are handled very efficiently instead of re-collecting all data every time as we do now. We could of course just send diffs instead of the whole data object every time. We could do this in a followup.

5) The content script has become quite huge. We should think about splitting it into JSMs again or using #includes.

6) We're using the sync handler to flush in situations where we need data to be returned instantly. That is: when a tab is closed, when a window is closed, on quit-application-requested, and when duplicating a tab. In those cases the frame script will send a sync message to let us update our permanent cache before continuing.

7) Another difficult case is handling the re-use of tabs through setTabState, setWindowState, etc. I solved this by sending 'update IDs' with the update messages that are increased every time a tab is re-used. So even if there's an update message on its way and we call setTabState, we will ignore it because the message will have an old ID.

8) Privacy level handling can change when the user changes prefs or pins/unpins a tab. Thus we always send all data to the main process and let it handle filtering sensitive data as needed.

The patch seems to work well and the tests (including the ones I attached) are succeeding. Do you see any major problems with this approach? Looking forward to some feedback!
Attachment #822272 - Flags: feedback?(wmccloskey)
Attachment #822272 - Flags: feedback?(smacleod)
Attachment #822272 - Flags: feedback?(dteller)
Here are the tests. They *should* run in e10s once we get the restoring part working.
Next version of the patch that also broadcast docShell capability data.

Another thing I solved was the problem that data that has been sent asynchronously (off the timer) just before flushing (e.g. due to closing a tab) would be lost.

We now send a monotonically increasing id with every update message. When we re-use a tab through setTabState, flush() is called that will then issue a new update id and send a message. All messages with an ID lower than the last received update ID will just be ignored.

To prevent data loss, chrome passes the last update ID it has received to SyncHandler.flush(). The frame script can then merge all data that has been sent with ids bigger then the given one and send that to chrome synchronously.
Attachment #822272 - Attachment is obsolete: true
Attachment #822272 - Flags: feedback?(wmccloskey)
Attachment #822272 - Flags: feedback?(smacleod)
Attachment #822272 - Flags: feedback?(dteller)
Attachment #822747 - Flags: feedback?(wmccloskey)
Attachment #822747 - Flags: feedback?(smacleod)
Attachment #822747 - Flags: feedback?(dteller)
Attached patch Add broadcasting for data, v3 (obsolete) — Splinter Review
I worked some more on this and I think this approach is stable and I like it. I cleaned it up and added documentation everywhere. I wrote a ton of tests to ensure it's working as intended and we cover all the race conditions I could come up with.

The one thing I didn't touch is broadcasting session history data. As the last thing collected via fillTabCachesAsynchronously() we can remove a huge bunch of code then. This seems best to do in a follow-up.
Attachment #822273 - Attachment is obsolete: true
Attachment #822747 - Attachment is obsolete: true
Attachment #822747 - Flags: feedback?(wmccloskey)
Attachment #822747 - Flags: feedback?(smacleod)
Attachment #822747 - Flags: feedback?(dteller)
Attachment #822988 - Flags: review?(wmccloskey)
Attachment #822988 - Flags: review?(dteller)
Attachment #822988 - Flags: feedback?(smacleod)
A bunch of tests.
Attachment #822991 - Flags: review?(wmccloskey)
Attachment #822991 - Flags: review?(dteller)
Attachment #822991 - Flags: feedback?(smacleod)
Attachment #822991 - Attachment description: Add tests for broadcasting sessionstore data, v2 → Add tests for broadcasting sessionstore data, v3
Depends on: 931706
Comment on attachment 822988 [details] [diff] [review]
Add broadcasting for data, v3

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

Looks good, but I believe that we can actually get rid of all the sync calls I have seen so far.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +179,5 @@
>  
> +/**
> + * Listens for changes to the page style. Whenever a different page style is
> + * selected or author styles are enabled/disabled we send a message with the
> + * currently applied style to the chrome process.

In future versions, it would be great if you could document with each listener the kind of messages it can send.

@@ +207,5 @@
> +  /**
> +   * This field is used to compare the last docShell capabilities to the ones
> +   * that have just been collected. If nothing changed we won't send a message.
> +   */
> +  _lastCapabilities: "",

Nit: "latest"?

@@ +217,5 @@
> +    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_LOCATION);
> +  },
> +
> +  /**
> +   * onLocationChange() will be called as soon as we start loading a page after

Nit: the future tense sounds weird to me.

@@ +302,5 @@
> +   * This allows us to easily find all updates with an $ID being
> +   * |this._lastFlushID < $ID < this._id| and then merge and re-send those
> +   * synchronously.
> +   */
> +  _lastFlushID: 0,

Nit: latest

@@ +340,5 @@
> +   *
> +   * @param key (string)
> +   *        A unique identifier specific to the type of data this is passed.
> +   * @param value (object)
> +   *        The value that is pushed onto the queue and will be sent to chrome.

Shouldn't we rather pass lazy values (as closures)?

Pro: we won't collect data that we don't send.
Con: might cause a stamped when the queue gets flushed.

@@ +370,5 @@
> +
> +    let sync = options && options.sync;
> +    let sendMessage = sync ? sendSyncMessage : sendAsyncMessage;
> +
> +    try {

Telemetry might be useful here.

@@ +375,5 @@
> +      sendMessage("SessionStore:update", {id: this._id, data: data});
> +    } catch (e) {
> +      // Sending may fail if the tab is currently being destroyed. Just in case
> +      // the failure is temporary and caused by something else, we want to
> +      // return here, and not clear and mark the current data as sent.

Can this happen?
Do we want to log it?

@@ +397,5 @@
> +   * @param id (int)
> +   *        A unique id that represents the last message received by the chrome
> +   *        process before flushing. We will use this to determine data that
> +   *        would be lost when data has been sent asynchronously shortly
> +   *        before flushing synchronously.

That sentence is not very clear.

@@ +419,5 @@
> +    this._lastFlushID = this._id;
> +
> +    // It's important to always send data, even if there is none. The update
> +    // message will be received by chrome that can then update its last
> +    // received update ID to ignore further messages.

Telemetry might be useful here, too.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +960,5 @@
> +
> +    // Collect window data only when *not* closed during shutdown.
> +    if (this._loadState == STATE_RUNNING) {
> +      // Flush all data queued in the content script before the window is gone.
> +      TabState.flushWindow(aWindow);

Rather than doing this synchronously, could we:
- hide the window;
- perform the collection;
- close the window once collection is complete?

@@ +1014,5 @@
>      // get a current snapshot of all windows
>      this._forEachBrowserWindow(function(aWindow) {
> +      // Flush all data queued in the content script to not lose it when
> +      // shutting down.
> +      TabState.flushWindow(aWindow);

This is full sync. Wouldn't be better to call AsyncShutdown instead and let processes respond asynchronously?

@@ +1273,5 @@
>        return;
>      }
>  
> +    // Flush all data queued in the content script before the tab is gone.
> +    TabState.flush(aTab.linkedBrowser);

What prevents us from doing this one asynchronously, by waiting until the collection is complete before we actually close the tab?

@@ +1520,5 @@
>        throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
>  
> +    // Flush all data queued in the content script because we will need that
> +    // state to properly duplicate the given tab.
> +    TabState.flush(aTab.linkedBrowser);

I seem to remember that tabs have a "not ready yet" state (i.e. while they don't have a linked browser). Would it be possible to do tab duplication asynchronously by collecting data asynchronously/opening the new tab in a "not ready yet" state/filling in the new tab once data has been received?

@@ +2560,5 @@
>  
> +      // Flush all data from the content script synchronously. This is done so
> +      // that all async messages that are still on their way to chrome will
> +      // be ignored and don't override any tab data set by restoreHistory().
> +      TabState.flush(tab.linkedBrowser);

I'm sure that there is a way to make this happen asynchronously. I can think of complicated ways (have both an "id" and a "generation", and in this operation, reset the id and increase the generation), but there must be simpler ways, too.

::: browser/components/sessionstore/src/TabState.jsm
@@ +97,5 @@
> +  flush: function (browser) {
> +    if (this._syncHandlers.has(browser)) {
> +      let lastID = this._lastMessageID.get(browser);
> +      this._syncHandlers.get(browser).flush(lastID);
> +    }

I don't like synchronous stuff, even if I appreciate we need it in corner cases.

Are there cases in which we could do the following

::: browser/components/sessionstore/src/TabStateCache.jsm
@@ +99,5 @@
>      return TabStateCacheInternal.removeField(aKey, aField);
>    },
>  
>    /**
> +   * Update the permanent cache for a given |browser|.

It's not clear from the documentation what the "permanent cache" is.
Attachment #822988 - Flags: review?(dteller) → feedback+
Comment on attachment 822991 [details] [diff] [review]
Add tests for broadcasting sessionstore data, v3

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

I have looked at a few of the changes and they look good. I'll look more in depth after we have agreed on the main patch.
Attachment #822991 - Flags: review?(dteller) → feedback+
Comment on attachment 822988 [details] [diff] [review]
Add broadcasting for data, v3

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

This looks great. I'm worried that getting rid of the sync calls will be harder than it looks. I looked into the issue fairly carefully a while back and there's a lot of Gecko grossness that you have to deal with. I think tab closing is probably the lowest hanging fruit. It looked like the easiest one, and it's also the one most likely to matter. However, I think it's fine for a follow-up bug.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +213,5 @@
> +  init: function () {
> +    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                              .getInterface(Ci.nsIWebProgress);
> +
> +    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_LOCATION);

The last time I added a new progress listener, I regressed Tp5 (https://bugzilla.mozilla.org/show_bug.cgi?id=899222#c19). Would you mind running this through Talos to make sure it's okay? onLocationChange fires less often than onStateChange (which I was using) so it's probably fine, but it would be good to make sure.

@@ +291,5 @@
> + */
> +let MessageQueue = {
> +  /**
> +   * A unique, monotonically increasing ID used for outgoing messages. This is
> +   * important make it possible to reuse tabs and allow sync flushes before

Missing a word.

@@ +311,5 @@
> +   * between those IDs haven't been received by chrome, yet. As chrome flushes
> +   * due to some operation that would make us lose data we will merge all those
> +   * unconfirmed updates and send them together with the currently queued data.
> +   */
> +  _updates: [],

If flushes are rare, could this array get very large? An alternate idea would be to store:

1. A mapping from keys to the last time that key was updated, maybe called _lastUpdate.
2. An object similar to _data, but cleared only on flush operations (call it _data2 for now).

The push code would just set _lastUpdate[key] = id.

The flush algorithm would include all the keys that were last updated after the input ID. That could easily be computed based on _lastUpdate and _data2.

@@ +314,5 @@
> +   */
> +  _updates: [],
> +
> +  /**
> +   * An object holding all currently queue data. The keys are identifier that

"queued ... identifiers"

@@ +334,5 @@
> +  _timeout: null,
> +
> +  /**
> +   * Pushes a given |value| onto the queue. The given |key| represents the type
> +   * of data that is stored and can override data has been queued before but

"data that has been queued"

@@ +402,5 @@
> +   */
> +  flush: function (id) {
> +    // Copies all properties and their values from |src| to |dst|.
> +    function merge(dst, src) {
> +      for (let [k, v] in Iterator(src)) {

I asked around and I'm told that we should not be continuing to use Iterator() (jorendorff suggested I say this in all caps, but I won't :-). Object.keys is the replacement. Alternatively we could use a Map here.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2700,5 @@
>      if (tabData.storage && browser.docShell instanceof Ci.nsIDocShell)
>        SessionStorage.deserialize(browser.docShell, tabData.storage);
>  
> +    // Update the permanent tab state cache with |tabData| information.
> +    TabStateCache.updatePermanent(browser, {

I feel like the right time to do this is when we set __SS_data in restoreHistoryPrecursor. That's the point when collection is supposed to get the new data rather than the old (based on how _collectBaseTabData works). Is there any reason this code can't go there?

::: browser/components/sessionstore/src/TabState.jsm
@@ +67,5 @@
>    _syncHandlers: new WeakMap(),
>  
> +  // A map (xul:browser -> int) that maps a browser to the
> +  // last message ID we received for it.
> +  _lastMessageID: new WeakMap(),

I expect you'll need to do something in onSwapDocshells for this.

::: browser/components/sessionstore/src/TabStateCache.jsm
@@ +148,5 @@
>  });
>  
>  let TabStateCacheInternal = {
>    _data: new WeakMap(),
> +  _permanentData: new WeakMap(),

Seems like we also need to deal with docshell swapping here?

@@ +253,5 @@
> +   */
> +  updatePermanent: function (browser, newData) {
> +    let data = this._permanentData.get(browser) || {};
> +
> +    for (let [key, value] in Iterator(newData)) {

Iterator() again.

@@ +289,5 @@
> +
> +    let data = this._permanentData.get(browser);
> +    let includePrivateData = options && options.includePrivateData;
> +
> +    for (let [key, value] in Iterator(data)) {

Same.

@@ +298,5 @@
> +        let isPinned = tab.pinned;
> +
> +        // If we're not allowed to include private data, let's filter out hosts
> +        // based on the given tab's pinned state and the privacy level.
> +        for (let [host, data] in Iterator(data.storage)) {

Same.
Attachment #822988 - Flags: review?(wmccloskey) → review+
Comment on attachment 822991 [details] [diff] [review]
Add tests for broadcasting sessionstore data, v3

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

These look very nice. I have a few high-level comments:

- The way the SyncHandler is accessed is going to be a problem for bug 673569, which I'd like to get moving again. I think we should fix this before landing.
- When waiting for setTabState or duplicateTab to complete, it's better to wait on SSTabRestored rather than the load event. That ensures that we're completely finished restoring the tab. I have a patch in my queue that converts all the tests to work this way.
- While reading the patches, I was assuming that getTabState did a flush() call before collecting. But now I see that it doesn't. It seems like people using it might expect it to flush. Maybe we should change it so that it does? I expect that this will be a lot more important when session history is converted to the broadcast approach. The main downside is that the tests won't be really testing async behavior if we do this. Maybe we should start adding async APIs now so that we can use them in tests?

::: browser/components/sessionstore/test/browser_broadcast.js
@@ +46,5 @@
> +  let browser = tab.linkedBrowser;
> +
> +  yield modifySessionStorage(browser, {foo: "baz"});
> +  let tab2 = ss.duplicateTab(window, tab);
> +  yield promiseBrowserLoaded(tab2.linkedBrowser);

The load event happens in the middle of restoring the new tab. Maybe we should have one test that checks right after duplicateTab happens (which should read out of browser.__SS_data) and another test that happens after SSTabRestored (which should really use the restored data, I think)?

@@ +68,5 @@
> +
> +  yield modifySessionStorage(browser, {foo: "baz"});
> +  yield closeWindow(win);
> +
> +  let [{tabs: [, {storage}]}] = JSON.parse(ss.getClosedWindowData());

You keep teaching me new JS language features :-). Personally I think it would be clearer to say |let [{tabs: [_, {storage}]}] = ...|, but maybe that's just my ML background.

@@ +95,5 @@
> +  // asynchronous messages.
> +  handler.flushAsync();
> +
> +  ss.setTabState(tab, state);
> +  yield promiseBrowserLoaded(browser);

I think it would be better to wait for SSTabRestored here.

@@ +177,5 @@
> +function waitForStorageEvent(browser) {
> +  return waitForMessage(browser, "ss-test:MozStorageChanged");
> +}
> +
> +function waitForUpdateMessage(browser) {

This looks unused.

@@ +192,5 @@
> +  browser.messageManager.sendAsyncMessage("ss-test:modifySessionStorage", data);
> +  return waitForStorageEvent(browser);
> +}
> +
> +function notifyObservers(browser, topic) {

This doesn't seem to be used.

::: browser/components/sessionstore/test/browser_capabilities.js
@@ +31,5 @@
>    browser.reload();
> +  yield promiseBrowserLoaded(browser);
> +
> +  // Flush to make sure chrome received all data.
> +  handler.flush();

Why do we need these flush calls? It seems like we should automatically be sending the new capabilities in the onLocationChange handler of DocShellCapabilitiesListener. Is that not called for reload()?

@@ +42,5 @@
>    is(disallow.size, 2, "two capabilities disallowed");
>  
>    // Reuse the tab to restore a new, clean state into it.
>    ss.setTabState(tab, JSON.stringify({ entries: [{url: "about:robots"}] }));
> +  yield promiseBrowserLoaded(browser);

It's almost always better to wait on SSTabRestored after doing setTabState.

@@ +54,5 @@
>    ok(flags.every(f => docShell[f]), "all flags set to true");
>  
>    // Restore the state with disallowed features.
>    ss.setTabState(tab, JSON.stringify(disallowedState));
> +  yield promiseBrowserLoaded(browser);

Same.

@@ +79,3 @@
>  }
>  
> +function waitForUpdateMessage(browser) {

This looks to be unused.

::: browser/components/sessionstore/test/browser_pageStyle.js
@@ +15,5 @@
> +  yield promiseBrowserLoaded(browser);
> +  let sheets = yield getStyleSheets(browser);
> +
> +  // Enable all style sheets one by one.
> +  for (let [title] of sheets) {

Can you please say |let [title, disabled] of sheets|? I just think it makes it clearer for people reading/changing the code in the future.

@@ +20,5 @@
> +    yield enableStyleSheetsForSet(browser, title);
> +
> +    let tab2 = gBrowser.duplicateTab(tab);
> +    let browser2 = tab2.linkedBrowser;
> +    yield promiseBrowserLoaded(browser2);

Again, I'd prefer waiting on SSTabRestored.

@@ +24,5 @@
> +    yield promiseBrowserLoaded(browser2);
> +
> +    let sheets = yield getStyleSheets(browser2);
> +    let enabled = sheets.map(([title, disabled]) => !disabled && title)
> +                        .filter(title => title);

It seems clearer to me to do the filter first and then the map...
  sheets.filter(([title, disabled]) => !disabled).map(([title, disabled]) => title).
I can see not wanting to have to pattern match on the array twice, but the |!disabled && title| bit is slightly confusing.

Also, the map isn't really needed. You just need to switch |enabled[0] == title| to |enabled[0][0] == title|.

@@ +40,5 @@
> +  yield setAuthorStyleDisabled(browser, true);
> +
> +  let tab2 = gBrowser.duplicateTab(tab);
> +  let browser2 = tab2.linkedBrowser;
> +  yield promiseBrowserLoaded(browser2);

SSTabRestored.

::: browser/components/sessionstore/test/browser_sessionStorage.js
@@ +27,5 @@
> +    "sessionStorage data for mochi.test has been serialized correctly");
> +
> +  // Ensure that modifying sessionStore values works.
> +  yield modifySessionStorage(browser, {foo: "baz"});
> +  handler.flush();

Why is the flush needed?

@@ +38,5 @@
> +
> +  // Test that duplicating a tab works.
> +  let tab2 = gBrowser.duplicateTab(tab);
> +  let browser2 = tab2.linkedBrowser;
> +  yield promiseBrowserLoaded(browser2);

SSTabRestored

::: browser/components/sessionstore/test/content.js
@@ +6,5 @@
> +  sendSyncMessage("ss-test:MozStorageChanged");
> +});
> +
> +addMessageListener("ss-test:modifySessionStorage", function (msg) {
> +  for (let [k, v] in Iterator(msg.data)) {

Iterator

@@ +16,5 @@
> +  Services.obs.notifyObservers(null, msg.data, "");
> +});
> +
> +addMessageListener("ss-test:getSyncHandler", function (msg) {
> +  sendSyncMessage("ss-test:getSyncHandler", {}, {handler: SyncHandler});

Err, this is not good. We shouldn't be depending on content-sessionStore.js being loaded ahead of us in the same global. I think we should try to get the object from TabState.jsm. And if it doesn't have it, we should set up a listener for the setupSyncHandler message. Or something like that.

Alternatively, could we just use TabState.flush() in the places where you're using the sync handler?

::: browser/components/sessionstore/test/head.js
@@ +7,5 @@
>  
> +const FRAME_SCRIPT = "chrome://mochitests/content/browser/browser/components/" +
> +                     "sessionstore/test/content.js";
> +
> +let mm = window.messageManager;

It seems more robust to use the global message manager. Otherwise, any new windows that the test creates won't get the content script, right? (It looks like head.js is only loaded into the main window, afaics.)

@@ +464,5 @@
>  function next() {
>    TestRunner.next();
>  }
> +
> +function waitForMessage(browser, name) {

We already have waitForContentMessage, which is similar except that it takes a timeout and doesn't use promises. Maybe we should combine them, or give them names that more clearly distinguish them. waitForContentMessage is only used in a few places, so it should be easy to refactor it.
Attachment #822991 - Flags: review?(wmccloskey)
Depends on: 938084, 938093, 938071
Comment on attachment 822988 [details] [diff] [review]
Add broadcasting for data, v3

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

This approach looks really good to me. I couldn't find anything Yoric or Bill hadn't already covered :D

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +311,5 @@
> +   * between those IDs haven't been received by chrome, yet. As chrome flushes
> +   * due to some operation that would make us lose data we will merge all those
> +   * unconfirmed updates and send them together with the currently queued data.
> +   */
> +  _updates: [],

This seems much nicer to me. How about _queuedData, and _latestData (for _data and _data2 respectively)?
Attachment #822988 - Flags: feedback?(smacleod) → feedback+
(In reply to Steven MacLeod [:smacleod] from comment #10)
> This seems much nicer to me. How about _queuedData, and _latestData (for
> _data and _data2 respectively)?

Is supposed to be in reply to Bill's comment about rare flushes and the updates array. I thought splinter would have quoted that here as well.
Comment on attachment 822991 [details] [diff] [review]
Add tests for broadcasting sessionstore data, v3

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

Overall looks good. I'll do a thorough pass like Yoric once things are updated.

::: browser/components/sessionstore/test/browser_916390_form_data_loss.js
@@ +27,5 @@
>    let state = JSON.parse(ss.getBrowserState());
>    let {formdata} = state.windows[0].tabs[1].entries[0];
>    is(formdata.id.txt, "m", "txt's value is correct");
>  
> +  // Change the number of session history entries invalidate the cache.

Missing a word
Attachment #822991 - Flags: feedback?(smacleod) → feedback+
Attached patch Add broadcasting for data, v4 (obsolete) — Splinter Review
Attachment #822988 - Attachment is obsolete: true
Attachment #832189 - Flags: review?(wmccloskey)
Attachment #832189 - Flags: review?(smacleod)
Attachment #832189 - Flags: review?(dteller)
Attachment #822991 - Attachment is obsolete: true
Attachment #832191 - Flags: review?(wmccloskey)
Attachment #832191 - Flags: review?(smacleod)
Attachment #832191 - Flags: review?(dteller)
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #6)
> > +/**
> > + * Listens for changes to the page style. Whenever a different page style is
> > + * selected or author styles are enabled/disabled we send a message with the
> > + * currently applied style to the chrome process.
> 
> In future versions, it would be great if you could document with each
> listener the kind of messages it can send.

All those listeners do is send SessionStore:update messages and use the MessageQueue object for that.

> @@ +340,5 @@
> > +   *
> > +   * @param key (string)
> > +   *        A unique identifier specific to the type of data this is passed.
> > +   * @param value (object)
> > +   *        The value that is pushed onto the queue and will be sent to chrome.
> 
> Shouldn't we rather pass lazy values (as closures)?
> 
> Pro: we won't collect data that we don't send.
> Con: might cause a stamped when the queue gets flushed.

I actually like the fact that we collect data when it changes, in smaller chunks. With lots of changes to the same type of data (e.g. DOMSessionStorage) this would of course be slower as reading once, but in the future we also want to be a little more clever about only queueing the data that has changed - the specific sessionStorage key that has changed or the history entry that was added.

> @@ +370,5 @@
> > +
> > +    let sync = options && options.sync;
> > +    let sendMessage = sync ? sendSyncMessage : sendAsyncMessage;
> > +
> > +    try {
> 
> Telemetry might be useful here.

Sorry, I don't understand what exactly you think we should measure here? The time it takes to send a message?

> @@ +375,5 @@
> > +      sendMessage("SessionStore:update", {id: this._id, data: data});
> > +    } catch (e) {
> > +      // Sending may fail if the tab is currently being destroyed. Just in case
> > +      // the failure is temporary and caused by something else, we want to
> > +      // return here, and not clear and mark the current data as sent.
> 
> Can this happen?
> Do we want to log it?

I don't know how send(A)SyncMessage() could fail but I thought it would be nice to handle it. This should probably only be caused by closed tabs so I don't think there's much value in logging it.

> @@ +419,5 @@
> > +    this._lastFlushID = this._id;
> > +
> > +    // It's important to always send data, even if there is none. The update
> > +    // message will be received by chrome that can then update its last
> > +    // received update ID to ignore further messages.
> 
> Telemetry might be useful here, too.

Hmm, really? We're flushing a lot, when closing a tab, duplicating a tab, closing a window, etc. What exactly do you think we should measure here?

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +960,5 @@
> > +
> > +    // Collect window data only when *not* closed during shutdown.
> > +    if (this._loadState == STATE_RUNNING) {
> > +      // Flush all data queued in the content script before the window is gone.
> > +      TabState.flushWindow(aWindow);
> 
> Rather than doing this synchronously, could we:
> - hide the window;
> - perform the collection;
> - close the window once collection is complete?

I'm not sure how we could prevent a window from closing if something calls win.close()? I'm all for doing that but it seems like this would need new APIs we don't have yet?

> @@ +1014,5 @@
> >      // get a current snapshot of all windows
> >      this._forEachBrowserWindow(function(aWindow) {
> > +      // Flush all data queued in the content script to not lose it when
> > +      // shutting down.
> > +      TabState.flushWindow(aWindow);
> 
> This is full sync. Wouldn't be better to call AsyncShutdown instead and let
> processes respond asynchronously?

The problem is that I don't know whether it is okay to collect data on quit-granted. I tried to maintain the current behavior as much as possible even if that means that there are a couple of synchronous fallbacks we have to keep. I think it would be best to adress those one by one in follow-ups, no?

> @@ +1273,5 @@
> >        return;
> >      }
> >  
> > +    // Flush all data queued in the content script before the tab is gone.
> > +    TabState.flush(aTab.linkedBrowser);
> 
> What prevents us from doing this one asynchronously, by waiting until the
> collection is complete before we actually close the tab?

We currently also don't have support for waiting on anything when gBrowser.removeTab() is called.

> @@ +1520,5 @@
> >        throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
> >  
> > +    // Flush all data queued in the content script because we will need that
> > +    // state to properly duplicate the given tab.
> > +    TabState.flush(aTab.linkedBrowser);
> 
> I seem to remember that tabs have a "not ready yet" state (i.e. while they
> don't have a linked browser). Would it be possible to do tab duplication
> asynchronously by collecting data asynchronously/opening the new tab in a
> "not ready yet" state/filling in the new tab once data has been received?

I think that tabs have a linkedBrowser right after they are created [looking at addTab()]. Also ss.duplicateTab() is an API with quite a few consumers so it would be great to not break it for now. That would also be better to tackle in a follow-up.

> @@ +2560,5 @@
> >  
> > +      // Flush all data from the content script synchronously. This is done so
> > +      // that all async messages that are still on their way to chrome will
> > +      // be ignored and don't override any tab data set by restoreHistory().
> > +      TabState.flush(tab.linkedBrowser);
> 
> I'm sure that there is a way to make this happen asynchronously. I can think
> of complicated ways (have both an "id" and a "generation", and in this
> operation, reset the id and increase the generation), but there must be
> simpler ways, too.

This would be a lot easier when the restoring part of SessionRestore works using async messages as well. For now we just go and set the state synchronously and we therefore need a sync message.
(In reply to Bill McCloskey (:billm) from comment #8)
> @@ +213,5 @@
> > +  init: function () {
> > +    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                              .getInterface(Ci.nsIWebProgress);
> > +
> > +    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_LOCATION);
> 
> The last time I added a new progress listener, I regressed Tp5
> (https://bugzilla.mozilla.org/show_bug.cgi?id=899222#c19). Would you mind
> running this through Talos to make sure it's okay? onLocationChange fires
> less often than onStateChange (which I was using) so it's probably fine, but
> it would be good to make sure.

We actually already have another onLocationChange listener in the frame script that sends SessionStore:loadStart messages. But sure, I can do that.

> @@ +311,5 @@
> > +   * between those IDs haven't been received by chrome, yet. As chrome flushes
> > +   * due to some operation that would make us lose data we will merge all those
> > +   * unconfirmed updates and send them together with the currently queued data.
> > +   */
> > +  _updates: [],
> 
> If flushes are rare, could this array get very large? An alternate idea
> would be to store:
> 
> 1. A mapping from keys to the last time that key was updated, maybe called
> _lastUpdate.
> 2. An object similar to _data, but cleared only on flush operations (call it
> _data2 for now).
> 
> The push code would just set _lastUpdate[key] = id.
> 
> The flush algorithm would include all the keys that were last updated after
> the input ID. That could easily be computed based on _lastUpdate and _data2.

Yes, that's a very good point. Your solution is actually a lot easier but might make differential updates a little harder to implement later. But we're not there yet anyway and we can revisit this code later.

> @@ +402,5 @@
> > +   */
> > +  flush: function (id) {
> > +    // Copies all properties and their values from |src| to |dst|.
> > +    function merge(dst, src) {
> > +      for (let [k, v] in Iterator(src)) {
> 
> I asked around and I'm told that we should not be continuing to use
> Iterator() (jorendorff suggested I say this in all caps, but I won't :-).
> Object.keys is the replacement. Alternatively we could use a Map here.

Hah okay, thanks for the hint. I'll remove all Iterator() usages.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +2700,5 @@
> >      if (tabData.storage && browser.docShell instanceof Ci.nsIDocShell)
> >        SessionStorage.deserialize(browser.docShell, tabData.storage);
> >  
> > +    // Update the permanent tab state cache with |tabData| information.
> > +    TabStateCache.updatePermanent(browser, {
> 
> I feel like the right time to do this is when we set __SS_data in
> restoreHistoryPrecursor. That's the point when collection is supposed to get
> the new data rather than the old (based on how _collectBaseTabData works).
> Is there any reason this code can't go there?

Yeah it probably makes more sense to move it there.

> ::: browser/components/sessionstore/src/TabState.jsm
> @@ +67,5 @@
> >    _syncHandlers: new WeakMap(),
> >  
> > +  // A map (xul:browser -> int) that maps a browser to the
> > +  // last message ID we received for it.
> > +  _lastMessageID: new WeakMap(),
> 
> I expect you'll need to do something in onSwapDocShells for this.

Yes, thanks for the hint.

> ::: browser/components/sessionstore/src/TabStateCache.jsm
> @@ +148,5 @@
> >  });
> >  
> >  let TabStateCacheInternal = {
> >    _data: new WeakMap(),
> > +  _permanentData: new WeakMap(),
> 
> Seems like we also need to deal with docshell swapping here?

Done.
(In reply to Steven MacLeod [:smacleod] from comment #10)
> ::: browser/components/sessionstore/content/content-sessionStore.js
> @@ +311,5 @@
> > +   * between those IDs haven't been received by chrome, yet. As chrome flushes
> > +   * due to some operation that would make us lose data we will merge all those
> > +   * unconfirmed updates and send them together with the currently queued data.
> > +   */
> > +  _updates: [],
> 
> This seems much nicer to me. How about _queuedData, and _latestData (for
> _data and _data2 respectively)?

I went with _data for all the data is queued and that is also always the latest data. The second map is named _lastUpdated.
(In reply to Bill McCloskey (:billm) from comment #9)
> Comment on attachment 822991 [details] [diff] [review]
> Add tests for broadcasting sessionstore data, v3

Oh, I somehow missed your feedback here, sorry. Will need to address that.
Attachment #832191 - Attachment is obsolete: true
Attachment #832191 - Flags: review?(wmccloskey)
Attachment #832191 - Flags: review?(smacleod)
Attachment #832191 - Flags: review?(dteller)
Attachment #832249 - Flags: review?(wmccloskey)
Attachment #832249 - Flags: review?(smacleod)
Attachment #832249 - Flags: review?(dteller)
(In reply to Bill McCloskey (:billm) from comment #9)
> - The way the SyncHandler is accessed is going to be a problem for bug
> 673569, which I'd like to get moving again. I think we should fix this
> before landing.

Yes, sorry about that :) Fixed.

> - When waiting for setTabState or duplicateTab to complete, it's better to
> wait on SSTabRestored rather than the load event. That ensures that we're
> completely finished restoring the tab. I have a patch in my queue that
> converts all the tests to work this way.

Yeah, good point. Will do.

> - While reading the patches, I was assuming that getTabState did a flush()
> call before collecting. But now I see that it doesn't. It seems like people
> using it might expect it to flush. Maybe we should change it so that it
> does? I expect that this will be a lot more important when session history
> is converted to the broadcast approach. The main downside is that the tests
> won't be really testing async behavior if we do this. Maybe we should start
> adding async APIs now so that we can use them in tests?

Yeah I didn't do this on purpose because I didn't want to let other code easily make us fall back to sync messages. It is true that with converting history to broadcasting we will potentially break lots of tests but flushing whenever someone calls getTabState(), getWindowState() or getBrowserState() seems bad. SessionStore after all was designed to be a crash recovery and as such it's not strictly necessary to always return the most up-to-date data. That is, in theory. In practice there might be and probably are consumers that expect the current state.

Even with async APIs, there's nothing that ensures a client does ever receive the most up-to-date state as it can change while waiting for the async message to arrive. The only thing async APIs can ensure is to retrieve the current state at the time the state was requested.

Would changing the semantics of all the state retrieval methods be such a bad thing? Would we really break that many clients? We could try and see whether there are any problems when landing this on Nightly. We can always insert a couple of sync flushes if this change turns out to be problematic.

> ::: browser/components/sessionstore/test/browser_broadcast.js
> @@ +68,5 @@
> > +
> > +  yield modifySessionStorage(browser, {foo: "baz"});
> > +  yield closeWindow(win);
> > +
> > +  let [{tabs: [, {storage}]}] = JSON.parse(ss.getClosedWindowData());
> 
> You keep teaching me new JS language features :-). Personally I think it
> would be clearer to say |let [{tabs: [_, {storage}]}] = ...|, but maybe
> that's just my ML background.

I also tend to like the ML syntax a little more although it technically creates a variable named "_" which isn't really a big deal.

> ::: browser/components/sessionstore/test/browser_capabilities.js
> @@ +31,5 @@
> >    browser.reload();
> > +  yield promiseBrowserLoaded(browser);
> > +
> > +  // Flush to make sure chrome received all data.
> > +  handler.flush();
> 
> Why do we need these flush calls? It seems like we should automatically be
> sending the new capabilities in the onLocationChange handler of
> DocShellCapabilitiesListener. Is that not called for reload()?

We do send them but there's no guarantee that these have arrived yet at this point. Especially with the timeout used in the frame script.

> ::: browser/components/sessionstore/test/browser_pageStyle.js
> @@ +24,5 @@
> > +    yield promiseBrowserLoaded(browser2);
> > +
> > +    let sheets = yield getStyleSheets(browser2);
> > +    let enabled = sheets.map(([title, disabled]) => !disabled && title)
> > +                        .filter(title => title);
> 
> It seems clearer to me to do the filter first and then the map...
>   sheets.filter(([title, disabled]) => !disabled).map(([title, disabled]) =>
> title).
> I can see not wanting to have to pattern match on the array twice, but the
> |!disabled && title| bit is slightly confusing.
> 
> Also, the map isn't really needed. You just need to switch |enabled[0] ==
> title| to |enabled[0][0] == title|.

Thanks for helping to clean that up. I think it was late when I wrote that...

> ::: browser/components/sessionstore/test/browser_sessionStorage.js
> @@ +27,5 @@
> > +    "sessionStorage data for mochi.test has been serialized correctly");
> > +
> > +  // Ensure that modifying sessionStore values works.
> > +  yield modifySessionStorage(browser, {foo: "baz"});
> > +  handler.flush();
> 
> Why is the flush needed?

Because getTabState() doesn't flush...

> ::: browser/components/sessionstore/test/content.js
> @@ +16,5 @@
> > +  Services.obs.notifyObservers(null, msg.data, "");
> > +});
> > +
> > +addMessageListener("ss-test:getSyncHandler", function (msg) {
> > +  sendSyncMessage("ss-test:getSyncHandler", {}, {handler: SyncHandler});
> 
> Err, this is not good. We shouldn't be depending on content-sessionStore.js
> being loaded ahead of us in the same global. I think we should try to get
> the object from TabState.jsm. And if it doesn't have it, we should set up a
> listener for the setupSyncHandler message. Or something like that.

I'm looking forward to each frame script having its own global :) Listening for "setupSyncHandler" and maintaining a WeakMap in head.js sounds fine.

> ::: browser/components/sessionstore/test/head.js
> @@ +464,5 @@
> >  function next() {
> >    TestRunner.next();
> >  }
> > +
> > +function waitForMessage(browser, name) {
> 
> We already have waitForContentMessage, which is similar except that it takes
> a timeout and doesn't use promises. Maybe we should combine them, or give
> them names that more clearly distinguish them. waitForContentMessage is only
> used in a few places, so it should be easy to refactor it.

Good idea, done.
(In reply to Tim Taubert [:ttaubert] from comment #15)
> (In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment
> #6)
> > > +/**
> > > + * Listens for changes to the page style. Whenever a different page style is
> > > + * selected or author styles are enabled/disabled we send a message with the
> > > + * currently applied style to the chrome process.
> > 
> > In future versions, it would be great if you could document with each
> > listener the kind of messages it can send.
> 
> All those listeners do is send SessionStore:update messages and use the
> MessageQueue object for that.

Sure, but it would be interesting (at some point) to know what's in the |data| field of these SessionStore:update messages.

> > @@ +370,5 @@
> > > +
> > > +    let sync = options && options.sync;
> > > +    let sendMessage = sync ? sendSyncMessage : sendAsyncMessage;
> > > +
> > > +    try {
> > 
> > Telemetry might be useful here.
> 
> Sorry, I don't understand what exactly you think we should measure here? The
> time it takes to send a message?

Actually, I was thinking of how often we send sync vs. async messages.
Time to send messages might be useful, too, your call.

> > Telemetry might be useful here, too.
> 
> Hmm, really? We're flushing a lot, when closing a tab, duplicating a tab,
> closing a window, etc. What exactly do you think we should measure here?

If I recall correctly, I was interested in knowing how often we flush – in the hope that we could progressively decrease the amount of flushing.

> 
> > ::: browser/components/sessionstore/src/SessionStore.jsm
> > @@ +960,5 @@
> > > +
> > > +    // Collect window data only when *not* closed during shutdown.
> > > +    if (this._loadState == STATE_RUNNING) {
> > > +      // Flush all data queued in the content script before the window is gone.
> > > +      TabState.flushWindow(aWindow);
> > 
> > Rather than doing this synchronously, could we:
> > - hide the window;
> > - perform the collection;
> > - close the window once collection is complete?
> 
> I'm not sure how we could prevent a window from closing if something calls
> win.close()? I'm all for doing that but it seems like this would need new
> APIs we don't have yet?

Could you file a bug?

> > @@ +1014,5 @@
> > >      // get a current snapshot of all windows
> > >      this._forEachBrowserWindow(function(aWindow) {
> > > +      // Flush all data queued in the content script to not lose it when
> > > +      // shutting down.
> > > +      TabState.flushWindow(aWindow);
> > 
> > This is full sync. Wouldn't be better to call AsyncShutdown instead and let
> > processes respond asynchronously?
> 
> The problem is that I don't know whether it is okay to collect data on
> quit-granted. I tried to maintain the current behavior as much as possible
> even if that means that there are a couple of synchronous fallbacks we have
> to keep. I think it would be best to adress those one by one in follow-ups,
> no?

Sounds good. Could you file it?

> > @@ +1273,5 @@
> > >        return;
> > >      }
> > >  
> > > +    // Flush all data queued in the content script before the tab is gone.
> > > +    TabState.flush(aTab.linkedBrowser);
> > 
> > What prevents us from doing this one asynchronously, by waiting until the
> > collection is complete before we actually close the tab?
> 
> We currently also don't have support for waiting on anything when
> gBrowser.removeTab() is called.

As above, let's file it :)

> > @@ +1520,5 @@
> > >        throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
> > >  
> > > +    // Flush all data queued in the content script because we will need that
> > > +    // state to properly duplicate the given tab.
> > > +    TabState.flush(aTab.linkedBrowser);
> > 
> > I seem to remember that tabs have a "not ready yet" state (i.e. while they
> > don't have a linked browser). Would it be possible to do tab duplication
> > asynchronously by collecting data asynchronously/opening the new tab in a
> > "not ready yet" state/filling in the new tab once data has been received?
> 
> I think that tabs have a linkedBrowser right after they are created [looking
> at addTab()]. Also ss.duplicateTab() is an API with quite a few consumers so
> it would be great to not break it for now. That would also be better to
> tackle in a follow-up.

Sounds like a plan. Can you file it?

> > @@ +2560,5 @@
> > >  
> > > +      // Flush all data from the content script synchronously. This is done so
> > > +      // that all async messages that are still on their way to chrome will
> > > +      // be ignored and don't override any tab data set by restoreHistory().
> > > +      TabState.flush(tab.linkedBrowser);
> > 
> > I'm sure that there is a way to make this happen asynchronously. I can think
> > of complicated ways (have both an "id" and a "generation", and in this
> > operation, reset the id and increase the generation), but there must be
> > simpler ways, too.
> 
> This would be a lot easier when the restoring part of SessionRestore works
> using async messages as well. For now we just go and set the state
> synchronously and we therefore need a sync message.

ok
Comment on attachment 832189 [details] [diff] [review]
Add broadcasting for data, v4

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

Do I understand correctly that session cookies, history, forms, etc. are meant for followup patches?

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +37,5 @@
> +  try {
> +    return event.storageArea == content.sessionStorage;
> +  } catch (ex) {
> +    // This page does not have a DOMSessionStorage
> +    // (this is typically the case for about: pages)

Could you find out which exception is launched and catch only this kind of exception?

@@ +192,5 @@
> +  },
> +
> +  observe: function (subject, topic) {
> +    if (subject.defaultView && subject.defaultView.top == content) {
> +      MessageQueue.push("pageStyle", PageStyle.collect(docShell) || null);

I realize that we want to avoid the stampedes that could happen if we made everything lazy. However, I believe that we should push closures here and use a DeferredTask in MessageQueue.push instead of just waiting BATCH_DELAY_MS with already-collected data. This would both save us from recollecting data too often.

@@ +225,5 @@
> +   * we are certain that there's nothing blocking the load (e.g. a content
> +   * policy added by AdBlock or the like).
> +   */
> +  onLocationChange: function() {
> +    let caps = DocShellCapabilities.collect(docShell).join(",");

I realize that the order of capabilities won't change, so this join operation is sufficient. Could you add a small comment to that effect?

@@ +259,5 @@
> +  },
> +
> +  handleEvent: function (event) {
> +    // Ignore events triggered by localStorage or globalStorage changes.
> +    if (isSessionStorageEvent(event)) {

You seem to use that utility function only once. Is it really useful to make it a toplevel function?

@@ +375,5 @@
> +    } catch (e) {
> +      // Sending may fail if the tab is currently being destroyed. Just in case
> +      // the failure is temporary and caused by something else, we want to
> +      // return here, and not clear and mark the current data as sent.
> +      return;

I fear that we could end up infinite-looping here.
If we don't know of anything that could cause this, I'd rather not add this catch-all case.

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +18,5 @@
>  this.SessionStorage = {
>    /**
>     * Updates all sessionStorage "super cookies"
>     * @param aDocShell
>     *        That tab's docshell (containing the sessionStorage)

Please document @return

@@ +42,5 @@
>  let DomStorage = {
>    /**
>     * Reads all session storage data from the given docShell.
>     * @param aDocShell
>     *        A tab's docshell (containing the sessionStorage)

Here, too.
Also, is this too late to rename that method |collect|?

@@ +58,2 @@
>  
> +      let origin = principal.jarPrefix + principal.origin;

Could you take the opportunity to document |origin|? This variable is rather unclear to me.

@@ -62,4 @@
>  
> -      // Check if we're allowed to store sessionStorage data.
> -      let isHttps = principal.URI && principal.URI.schemeIs("https");
> -      if (aFullData || PrivacyLevel.canSave({isHttps: isHttps, isPinned: isPinned})) {

Remind me, what allows us to remove the PrivacyLevel check? Have we moved this to the TabStateCache?

::: browser/components/sessionstore/src/TabState.jsm
@@ +72,5 @@
>    // See SyncHandler in content-sessionStore.js.
>    _syncHandlers: new WeakMap(),
>  
> +  // A map (xul:browser -> int) that maps a browser to the
> +  // last message ID we received for it.

Nit: Could you mention what kind of message you're talking about?
i.e. we're probably going to store something similar for messages between workers in bug 886447, so it would be nice to make the comments and the field names unambiguous.

@@ +130,5 @@
>      this.dropPendingCollections(otherBrowser);
>  
> +    // Swap data stored per-browser.
> +    let swap = map => Utils.swapMapEntries(map, browser, otherBrowser);
> +    [this._syncHandlers, this._latestMessageID].forEach(swap);

Nit: you probably don't need to define a named function just for a single use.

::: browser/components/sessionstore/src/TabStateCache.jsm
@@ +266,5 @@
>     *        The first of the two browsers that swapped docShells.
>     * @param {xul:browser} otherBrowser
>     *        The second of the two browsers that swapped docShells.
>     */
>    onSwapDocShells: function(browser, otherBrowser) {

Is it too late to rename this method? It actually doesn't involve docshells, just swapping data between two browsers, isn't it?

@@ +302,5 @@
> +    this._permanentData.set(browser, data);
> +  },
> +
> +  /**
> +   * Copy tab data for the given |tab| from the permanent cache to |tabData|.

I don't really like putting that much intelligence in the TabStateCache. I believe that the cache should not know about "storage", or pinned tabs, in particular.

::: browser/components/sessionstore/src/Utils.jsm
@@ +35,5 @@
>      let prevChar = host[index - 1];
>      return (index == (host.length - domain.length)) &&
>             (prevChar == "." || prevChar == "/");
> +  },
> +

Any documentation?
Attachment #832189 - Flags: review?(dteller) → feedback+
Comment on attachment 832249 [details] [diff] [review]
Add tests for broadcasting sessionstore data, v4

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

I have a few questions pending, but otherwise, r=me.

::: browser/components/sessionstore/test/browser_broadcast.js
@@ +10,5 @@
> +add_task(function flush_on_tabclose() {
> +  let tab = yield createTabWithStorageData(["http://example.com"]);
> +  let browser = tab.linkedBrowser;
> +
> +  yield modifySessionStorage(browser, {foo: "baz"});

Nit: Could you use distinct values for |foo| in each test?

@@ +27,5 @@
> +  let tab = yield createTabWithStorageData(["http://example.com"]);
> +  let browser = tab.linkedBrowser;
> +
> +  yield modifySessionStorage(browser, {foo: "baz"});
> +  sendQuitApplicationRequested();

Nit: Could you add a comment explaining why this probably doesn't interfere with other tests?

@@ +96,5 @@
> +  // Flush all data contained in the content script but send it using
> +  // asynchronous messages.
> +  SyncHandlers.get(browser).flushAsync();
> +
> +  ss.setTabState(tab, state);

Doesn't this suggest that setTabState should flush automatically?

@@ +123,5 @@
> +
> +  // Flush all data contained in the content script but send it using
> +  // asynchronous messages.
> +  SyncHandlers.get(browser).flushAsync();
> +  gBrowser.removeTab(tab);

So what happens if we call removeTab but not flushAsync()?

::: browser/components/sessionstore/test/browser_sessionStorage.js
@@ +19,5 @@
> +  // Flush to make sure chrome received all data.
> +  SyncHandlers.get(browser).flush();
> +
> +  let {storage} = JSON.parse(ss.getTabState(tab));
> +  is(storage["http://example.com"].foo, "bar",

Could you use different strings for the various tests?

::: browser/components/sessionstore/test/content.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Could you document this file?

@@ +27,5 @@
> +  sendSyncMessage("ss-test:enableStyleSheetsForSet");
> +});
> +
> +addMessageListener("ss-test:enableSubDocumentStyleSheetsForSet", function (msg) {
> +  let iframe = content.document.getElementById("iframe");

That looks very specific. Wouldn't it be better to pass the |id| as part of |msg.data|?

::: browser/components/sessionstore/test/head.js
@@ +12,5 @@
> +           .getService(Ci.nsIMessageListenerManager);
> +mm.loadFrameScript(FRAME_SCRIPT, true);
> +mm.addMessageListener("SessionStore:setupSyncHandler", onSetupSyncHandler);
> +
> +let SyncHandlers = new WeakMap();

Could you document this?
Attachment #832249 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #23)
> ::: browser/components/sessionstore/test/browser_broadcast.js
> @@ +10,5 @@
> > +add_task(function flush_on_tabclose() {
> > +  let tab = yield createTabWithStorageData(["http://example.com"]);
> > +  let browser = tab.linkedBrowser;
> > +
> > +  yield modifySessionStorage(browser, {foo: "baz"});
> 
> Nit: Could you use distinct values for |foo| in each test?

Will do.

> @@ +27,5 @@
> > +  let tab = yield createTabWithStorageData(["http://example.com"]);
> > +  let browser = tab.linkedBrowser;
> > +
> > +  yield modifySessionStorage(browser, {foo: "baz"});
> > +  sendQuitApplicationRequested();
> 
> Nit: Could you add a comment explaining why this probably doesn't interfere
> with other tests?

Sure.

> @@ +96,5 @@
> > +  // Flush all data contained in the content script but send it using
> > +  // asynchronous messages.
> > +  SyncHandlers.get(browser).flushAsync();
> > +
> > +  ss.setTabState(tab, state);
> 
> Doesn't this suggest that setTabState should flush automatically?

It does flush automatically. The test here causes all code queued in the frame script to be sent asynchronously and would thus get lost if our algorithm using message ids would be broken. That's what the test here ensures.

> @@ +123,5 @@
> > +
> > +  // Flush all data contained in the content script but send it using
> > +  // asynchronous messages.
> > +  SyncHandlers.get(browser).flushAsync();
> > +  gBrowser.removeTab(tab);
> 
> So what happens if we call removeTab but not flushAsync()?

Everything just works fine because removeTab() flushes automatically, please see my previous annotation.

> ::: browser/components/sessionstore/test/browser_sessionStorage.js
> @@ +19,5 @@
> > +  // Flush to make sure chrome received all data.
> > +  SyncHandlers.get(browser).flush();
> > +
> > +  let {storage} = JSON.parse(ss.getTabState(tab));
> > +  is(storage["http://example.com"].foo, "bar",
> 
> Could you use different strings for the various tests?

Will do.

> ::: browser/components/sessionstore/test/content.js
> @@ +1,4 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> 
> Could you document this file?

Sure.

> @@ +27,5 @@
> > +  sendSyncMessage("ss-test:enableStyleSheetsForSet");
> > +});
> > +
> > +addMessageListener("ss-test:enableSubDocumentStyleSheetsForSet", function (msg) {
> > +  let iframe = content.document.getElementById("iframe");
> 
> That looks very specific. Wouldn't it be better to pass the |id| as part of
> |msg.data|?

Yup, will do.

> ::: browser/components/sessionstore/test/head.js
> @@ +12,5 @@
> > +           .getService(Ci.nsIMessageListenerManager);
> > +mm.loadFrameScript(FRAME_SCRIPT, true);
> > +mm.addMessageListener("SessionStore:setupSyncHandler", onSetupSyncHandler);
> > +
> > +let SyncHandlers = new WeakMap();
> 
> Could you document this?

Sure.
Blocks: 939742
Blocks: 939759
Blocks: 939761
Blocks: 939764
Attached patch Add broadcasting for data, v5 (obsolete) — Splinter Review
Attachment #832189 - Attachment is obsolete: true
Attachment #832189 - Flags: review?(wmccloskey)
Attachment #832189 - Flags: review?(smacleod)
Attachment #8333830 - Flags: review?(wmccloskey)
Attachment #8333830 - Flags: review?(smacleod)
Attachment #8333830 - Flags: review?(dteller)
Carrying over r+ from David.
Attachment #832249 - Attachment is obsolete: true
Attachment #832249 - Flags: review?(wmccloskey)
Attachment #832249 - Flags: review?(smacleod)
Attachment #8333832 - Flags: review?(wmccloskey)
Attachment #8333832 - Flags: review?(smacleod)
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #21)
> (In reply to Tim Taubert [:ttaubert] from comment #15)
> > All those listeners do is send SessionStore:update messages and use the
> > MessageQueue object for that.
> 
> Sure, but it would be interesting (at some point) to know what's in the
> |data| field of these SessionStore:update messages.

Done.

> > > Telemetry might be useful here.
> > 
> > Sorry, I don't understand what exactly you think we should measure here? The
> > time it takes to send a message?
> 
> Actually, I was thinking of how often we send sync vs. async messages.
> Time to send messages might be useful, too, your call.

Should this be a ratio like we did for the TabStateCache? Can this be tackled in a follow-up?

> > > Telemetry might be useful here, too.
> > 
> > Hmm, really? We're flushing a lot, when closing a tab, duplicating a tab,
> > closing a window, etc. What exactly do you think we should measure here?
> 
> If I recall correctly, I was interested in knowing how often we flush – in
> the hope that we could progressively decrease the amount of flushing.

Should this also be a ratio? Does a counter make sense here? Can this be tackled in a follow-up?

> > I'm not sure how we could prevent a window from closing if something calls
> > win.close()? I'm all for doing that but it seems like this would need new
> > APIs we don't have yet?
> 
> Could you file a bug?

Filed bug 939742.

> > The problem is that I don't know whether it is okay to collect data on
> > quit-granted. I tried to maintain the current behavior as much as possible
> > even if that means that there are a couple of synchronous fallbacks we have
> > to keep. I think it would be best to adress those one by one in follow-ups,
> > no?
> 
> Sounds good. Could you file it?

Filed bug 939759.

> > We currently also don't have support for waiting on anything when
> > gBrowser.removeTab() is called.
> 
> As above, let's file it :)

Filed bug 939761.

> > I think that tabs have a linkedBrowser right after they are created [looking
> > at addTab()]. Also ss.duplicateTab() is an API with quite a few consumers so
> > it would be great to not break it for now. That would also be better to
> > tackle in a follow-up.
> 
> Sounds like a plan. Can you file it?

Filed bug 939764.
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #22)
> Do I understand correctly that session cookies, history, forms, etc. are
> meant for followup patches?

Yes, exactly. History, forms and cookie hosts are collected together. Cookies will be collected by the chrome process.

> ::: browser/components/sessionstore/content/content-sessionStore.js
> @@ +37,5 @@
> > +  try {
> > +    return event.storageArea == content.sessionStorage;
> > +  } catch (ex) {
> > +    // This page does not have a DOMSessionStorage
> > +    // (this is typically the case for about: pages)
> 
> Could you find out which exception is launched and catch only this kind of
> exception?

Done.

> @@ +192,5 @@
> > +  },
> > +
> > +  observe: function (subject, topic) {
> > +    if (subject.defaultView && subject.defaultView.top == content) {
> > +      MessageQueue.push("pageStyle", PageStyle.collect(docShell) || null);
> 
> I realize that we want to avoid the stampedes that could happen if we made
> everything lazy. However, I believe that we should push closures here and
> use a DeferredTask in MessageQueue.push instead of just waiting
> BATCH_DELAY_MS with already-collected data. This would both save us from
> recollecting data too often.

If we use DeferredTask sending a message could be delayed indefinitely by a constant stream of events, right? Looking at the source it looks like DeferredTask.start() cancels any active timer and restarts it.

I converted the code to use lazy closures now.

> @@ +225,5 @@
> > +   * we are certain that there's nothing blocking the load (e.g. a content
> > +   * policy added by AdBlock or the like).
> > +   */
> > +  onLocationChange: function() {
> > +    let caps = DocShellCapabilities.collect(docShell).join(",");
> 
> I realize that the order of capabilities won't change, so this join
> operation is sufficient. Could you add a small comment to that effect?

Good point, will do.

> @@ +259,5 @@
> > +  },
> > +
> > +  handleEvent: function (event) {
> > +    // Ignore events triggered by localStorage or globalStorage changes.
> > +    if (isSessionStorageEvent(event)) {
> 
> You seem to use that utility function only once. Is it really useful to make
> it a toplevel function?

It's really not necessary but I found it a little more readable with a good function name and the awkward try-catch stuff captured in a single function.

> @@ +375,5 @@
> > +    } catch (e) {
> > +      // Sending may fail if the tab is currently being destroyed. Just in case
> > +      // the failure is temporary and caused by something else, we want to
> > +      // return here, and not clear and mark the current data as sent.
> > +      return;
> 
> I fear that we could end up infinite-looping here.
> If we don't know of anything that could cause this, I'd rather not add this
> catch-all case.

That sounds reasonable, I'll remove the return here.

> ::: browser/components/sessionstore/src/SessionStorage.jsm
> @@ +18,5 @@
> >  this.SessionStorage = {
> >    /**
> >     * Updates all sessionStorage "super cookies"
> >     * @param aDocShell
> >     *        That tab's docshell (containing the sessionStorage)
> 
> Please document @return

Done.

> @@ +42,5 @@
> >  let DomStorage = {
> >    /**
> >     * Reads all session storage data from the given docShell.
> >     * @param aDocShell
> >     *        A tab's docshell (containing the sessionStorage)
> 
> Here, too.
> Also, is this too late to rename that method |collect|?

I don't think it's too late. I agree, let's get our naming straight for all the data collection modules.

> @@ +58,2 @@
> >  
> > +      let origin = principal.jarPrefix + principal.origin;
> 
> Could you take the opportunity to document |origin|? This variable is rather
> unclear to me.

Done.

> @@ -62,4 @@
> >  
> > -      // Check if we're allowed to store sessionStorage data.
> > -      let isHttps = principal.URI && principal.URI.schemeIs("https");
> > -      if (aFullData || PrivacyLevel.canSave({isHttps: isHttps, isPinned: isPinned})) {
> 
> Remind me, what allows us to remove the PrivacyLevel check? Have we moved
> this to the TabStateCache?

Yes, exactly. We need all the data in the cache so that we can appropriately duplicate tabs and thus the privacy level filter has now moved to the TabStateCache.

> ::: browser/components/sessionstore/src/TabState.jsm
> @@ +72,5 @@
> >    // See SyncHandler in content-sessionStore.js.
> >    _syncHandlers: new WeakMap(),
> >  
> > +  // A map (xul:browser -> int) that maps a browser to the
> > +  // last message ID we received for it.
> 
> Nit: Could you mention what kind of message you're talking about?
> i.e. we're probably going to store something similar for messages between
> workers in bug 886447, so it would be nice to make the comments and the
> field names unambiguous.

Done.

> ::: browser/components/sessionstore/src/TabStateCache.jsm
> @@ +266,5 @@
> >     *        The first of the two browsers that swapped docShells.
> >     * @param {xul:browser} otherBrowser
> >     *        The second of the two browsers that swapped docShells.
> >     */
> >    onSwapDocShells: function(browser, otherBrowser) {
> 
> Is it too late to rename this method? It actually doesn't involve docshells,
> just swapping data between two browsers, isn't it?

I think the name is appropriate, isn't it? It handles the 'SwapDocShells' event that is fired when two browser swap their docShells.

> @@ +302,5 @@
> > +    this._permanentData.set(browser, data);
> > +  },
> > +
> > +  /**
> > +   * Copy tab data for the given |tab| from the permanent cache to |tabData|.
> 
> I don't really like putting that much intelligence in the TabStateCache. I
> believe that the cache should not know about "storage", or pinned tabs, in
> particular.

Yeah, I agree. I rewrote this to make TabStateCache expose a |getPermanent()| method that TabState uses to retrieve and apply permanently cached data.

> ::: browser/components/sessionstore/src/Utils.jsm
> @@ +35,5 @@
> >      let prevChar = host[index - 1];
> >      return (index == (host.length - domain.length)) &&
> >             (prevChar == "." || prevChar == "/");
> > +  },
> > +
> 
> Any documentation?

Hmpf, I didn't write that stuff, it's distributed all over the code base. Let me copy a comment... :)
Depends on: 939784
Comment on attachment 8333830 [details] [diff] [review]
Add broadcasting for data, v5

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

Looks good except for a few possible issues.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +59,5 @@
> + */
> +function isSessionStorageEvent(event) {
> +  try {
> +    return event.storageArea == content.sessionStorage;
> +  } catch (ex if ex.result == Cr.NS_ERROR_NOT_AVAILABLE) {

I think this should be (ex if ex instanceof Ci.nsIException && ex.result == Cr.NS_ERROR_NOT_AVAILABLE). It's too bad there isn't a helper for this sort of check.

@@ +263,5 @@
> +      let caps = DocShellCapabilities.collect(docShell).join(",");
> +
> +      // Send new data only when the capability list changes.
> +      if (caps == this._latestCapabilities) {
> +        throw new AbortDataCollection();

Is the following possible?
0. Assume that the current capabilities are "a,b".
1. onLocationChanged is called, we set a new "disallow" function in _data. Call it disallow1.
2. We call send() based on a timer. We call disallow1 and it sets _latestCapabilities to "x,y". We send those to the parent.
3. Another onLocationChange happens and we create disallow2. It replaces disallow1 in _data.
4. Now the parent calls flush(). We call disallow2 and it throws AbortDataCollection. Now send() doesn't send anything for "disallow".

It seems that the caller of flush would end up with stale values for the capabilities in this case.

Calling DocShellCapabilities.collect is very cheap, so I don't think we should be lazy here. Just collect the data eagerly and then throw push a closure that returns that data.

@@ +430,5 @@
> +    }
> +
> +    // If all data collection routines bailed out there might be no data left.
> +    if (Object.keys(data).length === 0) {
> +      return;

In the comment for flush() down below, you say that we should always send something to keep the ID up-to-date. This code seems to work differently though. I think the comment is right and we should remove this check?

::: browser/components/sessionstore/src/TabState.jsm
@@ +132,5 @@
>      this.dropPendingCollections(otherBrowser);
>  
> +    // Swap data stored per-browser.
> +    [this._syncHandlers, this._latestMessageID]
> +      .forEach(map => Utils.swapMapEntries(map, browser, otherBrowser));

I thought of a better way to do this. Every time we create a <browser> element, we could do something like |browser.permanentKey = {}| (where permanentKey is an XBL-defined field). swapDocShells would be responsible for swapping the permanentKey objects. Then anyone wanting to use weakmaps of <browser> elements could just use browser.permanentKey for lookups. Perhaps we could do this as a follow-up though.
Attachment #8333830 - Flags: review?(wmccloskey) → review+
Comment on attachment 8333832 [details] [diff] [review]
Add tests for broadcasting sessionstore data, v5

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

I just gave this a quick once-over. It seems good.
Attachment #8333832 - Flags: review?(wmccloskey) → review+
(In reply to Tim Taubert [:ttaubert] from comment #28)
> If we use DeferredTask sending a message could be delayed indefinitely by a
> constant stream of events, right? Looking at the source it looks like
> DeferredTask.start() cancels any active timer and restarts it.

Ah, right, good point.

> I converted the code to use lazy closures now.

Thanks.


> > ::: browser/components/sessionstore/src/TabStateCache.jsm
> > @@ +266,5 @@
> > >     *        The first of the two browsers that swapped docShells.
> > >     * @param {xul:browser} otherBrowser
> > >     *        The second of the two browsers that swapped docShells.
> > >     */
> > >    onSwapDocShells: function(browser, otherBrowser) {
> > 
> > Is it too late to rename this method? It actually doesn't involve docshells,
> > just swapping data between two browsers, isn't it?
> 
> I think the name is appropriate, isn't it? It handles the 'SwapDocShells'
> event that is fired when two browser swap their docShells.

Well, it's just that TabStateCache doesn't know (or do) anything about docshells.
I won't insist, though.

> Yeah, I agree. I rewrote this to make TabStateCache expose a
> |getPermanent()| method that TabState uses to retrieve and apply permanently
> cached data.

Thanks.

> > Any documentation?
> 
> Hmpf, I didn't write that stuff, it's distributed all over the code base.
> Let me copy a comment... :)

:)
Comment on attachment 8333830 [details] [diff] [review]
Add broadcasting for data, v5

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

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +30,5 @@
>    "resource:///modules/sessionstore/TextAndScrollData.jsm");
>  
>  /**
> + * Exception that can be thrown by data collection routines to signal that
> + * there is not data to be collected and they should just be skipped.

I personally add @constructor annotations to constructors. Your call.

@@ +36,5 @@
> +function AbortDataCollection() {}
> +
> +/**
> + * Returns a lazy function that will evaluate the given function |fn| only once
> + * and cache its return value. The value will not be cached should |fn| throw.

So the function is re-evaluated at each call if it throws? Is that by design?

@@ +162,5 @@
>    },
>  
> +  /**
> +   * This function is used to make the tab process flush all data that
> +   * hasn't been sent to chrome, yet.

Nit: I believe that it would be clearer if you wrote "parent process" instead of "chrome" everywhere.

@@ +309,5 @@
> +    // that needs to purge data can do its work first.
> +    setTimeout(() => this.collect(), 0);
> +  },
> +
> +  collect: function () {

Do we actually call that method from anywhere besides |observe|? If not, I'd just inline it.

@@ +424,5 @@
> +      } catch (ex if ex instanceof AbortDataCollection) {
> +        // The data collection routine signalled that there is not data to be
> +        // sent to chrome. Remove it from the map until it re-inserts itself
> +        // again.
> +        this._data.delete(key);

I like the AbortDataCollection strategy.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2617,1 @@
>        if (tabData.extData) {

Hadn't you removed that if/else in a previous version of the patch?
Or am I getting confused in my interdiffs?

::: browser/components/sessionstore/src/TabStateCache.jsm
@@ +125,5 @@
> +
> +  /**
> +   * Updates permanently cached data for a given |browser|. This data is
> +   * permanent in the sense that we never clear it, it will always be
> +   * overwritten.

That doesn't sound "permanent" to me. Maybe "immutable"?
Attachment #8333830 - Flags: review?(dteller) → review+
Carrying over r+ from David and Bill.
Attachment #8333830 - Attachment is obsolete: true
Attachment #8333830 - Flags: review?(smacleod)
Attachment #8335935 - Flags: review?(smacleod)
(In reply to Bill McCloskey (:billm) from comment #29)
> It seems that the caller of flush would end up with stale values for the
> capabilities in this case.

Indeed it would.

> Calling DocShellCapabilities.collect is very cheap, so I don't think we
> should be lazy here. Just collect the data eagerly and then throw push a
> closure that returns that data.

Yes, I think the concept of throwing AbortDataCollection doesn't work out that way with lazy closures.

> @@ +430,5 @@
> > +    }
> > +
> > +    // If all data collection routines bailed out there might be no data left.
> > +    if (Object.keys(data).length === 0) {
> > +      return;
> 
> In the comment for flush() down below, you say that we should always send
> something to keep the ID up-to-date. This code seems to work differently
> though. I think the comment is right and we should remove this check?

Oops, good catch. Thanks.

> ::: browser/components/sessionstore/src/TabState.jsm
> @@ +132,5 @@
> >      this.dropPendingCollections(otherBrowser);
> >  
> > +    // Swap data stored per-browser.
> > +    [this._syncHandlers, this._latestMessageID]
> > +      .forEach(map => Utils.swapMapEntries(map, browser, otherBrowser));
> 
> I thought of a better way to do this. Every time we create a <browser>
> element, we could do something like |browser.permanentKey = {}| (where
> permanentKey is an XBL-defined field). swapDocShells would be responsible
> for swapping the permanentKey objects. Then anyone wanting to use weakmaps
> of <browser> elements could just use browser.permanentKey for lookups.
> Perhaps we could do this as a follow-up though.

That sounds like an interesting idea to me. Let's clean that up in a follow up.
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #31)
> > > >    onSwapDocShells: function(browser, otherBrowser) {
> > > 
> > > Is it too late to rename this method? It actually doesn't involve docshells,
> > > just swapping data between two browsers, isn't it?
> > 
> > I think the name is appropriate, isn't it? It handles the 'SwapDocShells'
> > event that is fired when two browser swap their docShells.
> 
> Well, it's just that TabStateCache doesn't know (or do) anything about
> docshells.
> I won't insist, though.

I renamed it to onBrowserContentsSwapped but this will hopefully completely go away with the follow-up I filed for Bill's suggestion.

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #32)
> @@ +162,5 @@
> >    },
> >  
> > +  /**
> > +   * This function is used to make the tab process flush all data that
> > +   * hasn't been sent to chrome, yet.
> 
> Nit: I believe that it would be clearer if you wrote "parent process"
> instead of "chrome" everywhere.

So how about 'chrome process'? ;) Yeah I can replace those usages.

> @@ +309,5 @@
> > +    // that needs to purge data can do its work first.
> > +    setTimeout(() => this.collect(), 0);
> > +  },
> > +
> > +  collect: function () {
> 
> Do we actually call that method from anywhere besides |observe|? If not, I'd
> just inline it.

handleEvent() calls it as well.

> @@ +424,5 @@
> > +      } catch (ex if ex instanceof AbortDataCollection) {
> > +        // The data collection routine signalled that there is not data to be
> > +        // sent to chrome. Remove it from the map until it re-inserts itself
> > +        // again.
> > +        this._data.delete(key);
> 
> I like the AbortDataCollection strategy.

Sorry, I removed that for now due to the race condition that Bill pointed out. Collecting docShell data is really cheap and the other code doesn't need it for now. We can always re-introduce it later.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +2617,1 @@
> >        if (tabData.extData) {
> 
> Hadn't you removed that if/else in a previous version of the patch?
> Or am I getting confused in my interdiffs?

Hmm. I honestly can't remember that I touched that. There was some code that Bill landed in the meantime that I think touched that code? I remember I had to rebase a little.

> ::: browser/components/sessionstore/src/TabStateCache.jsm
> @@ +125,5 @@
> > +
> > +  /**
> > +   * Updates permanently cached data for a given |browser|. This data is
> > +   * permanent in the sense that we never clear it, it will always be
> > +   * overwritten.
> 
> That doesn't sound "permanent" to me. Maybe "immutable"?

I decided to go with "persistent" and updated all fields and method names.
(In reply to Tim Taubert [:ttaubert] from comment #34)
> > I thought of a better way to do this. Every time we create a <browser>
> > element, we could do something like |browser.permanentKey = {}| (where
> > permanentKey is an XBL-defined field). swapDocShells would be responsible
> > for swapping the permanentKey objects. Then anyone wanting to use weakmaps
> > of <browser> elements could just use browser.permanentKey for lookups.
> > Perhaps we could do this as a follow-up though.
> 
> That sounds like an interesting idea to me. Let's clean that up in a follow
> up.

(In reply to Tim Taubert [:ttaubert] from comment #35)
> I renamed it to onBrowserContentsSwapped but this will hopefully completely
> go away with the follow-up I filed for Bill's suggestion.

Filed bug 941540.
Try looks great apart from that this patch seems to make bug 824021 and bug 919060 happen more often. Fortunately, because I was working on those anyway \o/.

https://tbpl.mozilla.org/?tree=Try&rev=efab6c3fb93f
Depends on: 824021, 919060
Comment on attachment 8335935 [details] [diff] [review]
Add broadcasting for data, v6

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

Just a couple nits about "permanent" vs "persistent", looks great!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2635,5 @@
>        browser.__SS_restoreState = TAB_STATE_NEEDS_RESTORE;
>        browser.setAttribute("pending", "true");
>        tab.setAttribute("pending", "true");
>  
> +      // Update the permanent tab state cache with |tabData| information.

Update the "persistent"

::: browser/components/sessionstore/src/TabState.jsm
@@ +177,5 @@
>          tabData.index = history.index;
>        }
>  
> +      // Copy data from the permanent cache.
> +      this._copyFromPermanentCache(tab, tabData);

"Persistent" cache?

@@ +300,5 @@
>        tabData.index = history.index;
>      }
>  
> +    // Copy data from the permanent cache.
> +    this._copyFromPermanentCache(tab, tabData, options);

Persistent?

@@ +315,5 @@
> +   *        The tab data belonging to the given |tab|.
> +   * @param options (object)
> +   *        {includePrivateData: true} to always include private data
> +   */
> +  _copyFromPermanentCache: function (tab, tabData, options = {}) {

Persistent?
Attachment #8335935 - Flags: review?(smacleod) → review+
Comment on attachment 8333832 [details] [diff] [review]
Add tests for broadcasting sessionstore data, v5

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

Looks great :D. Just had a few nits about comments.

::: browser/components/sessionstore/test/browser_broadcast.js
@@ +44,5 @@
> +  gBrowser.removeTab(tab);
> +});
> +
> +/**
> + * This tests ensures we won't lose tab data queued in the content script when

This "test" ensures ...

@@ +86,5 @@
> +
> +/**
> + * This test ensures that tab data that has been sent asynchronously just
> + * before reusing a tab (via e.g. setTabState) does not overwrite the new data.
> + * The stale data must be ignored by issuing a synchronous flush before reusing

Isn't "by issuing a synchronous flush before reusing a tab" just an implementation detail? Should you really say this in the test? How about just "The stale data must be ignored."

::: browser/components/sessionstore/test/browser_pageStyle.js
@@ +6,5 @@
> +const URL = getRootDirectory(gTestPath) + "browser_pageStyle_sample.html";
> +const URL_NESTED = getRootDirectory(gTestPath) + "browser_pageStyle_sample_nested.html";
> +
> +/**
> + * This tests ensure that page style information is correctly persisted.

This "test" ...

@@ +48,5 @@
> +  gBrowser.removeTab(tab2);
> +});
> +
> +/**
> + * This tests ensures that page style notification from nested documents are

This "test" ...
Attachment #8333832 - Flags: review?(smacleod) → review+
(In reply to Steven MacLeod [:smacleod] from comment #38)
> Just a couple nits about "permanent" vs "persistent", looks great!

Thanks for catching those. I should have taken a look at the other files m(
(In reply to Steven MacLeod [:smacleod] from comment #39)
> > + * This test ensures that tab data that has been sent asynchronously just
> > + * before reusing a tab (via e.g. setTabState) does not overwrite the new data.
> > + * The stale data must be ignored by issuing a synchronous flush before reusing
> 
> Isn't "by issuing a synchronous flush before reusing a tab" just an
> implementation detail? Should you really say this in the test? How about
> just "The stale data must be ignored."

Yeah, that's a valid point. We wouldn't want to update all test comments when the implementation changes.
Pushed small follow-up because I was accidentally overriding the 'timeout' argument for promiseContentMessage(). This lead to intermittent failures where setTimeout(0) was faster than the message itself :)

https://hg.mozilla.org/integration/fx-team/rev/89135eff3d54
Pushed another follow-up as it doesn't really help when the default timeout for promiseContentMessage() is equal to the delay we use in the frame script. I removed the timeout parameter because we really are waiting for a content message and giving a proper timeout is almost impossible on our slow test machines. The test can just time out waiting for the message.

https://hg.mozilla.org/integration/fx-team/rev/b26502022122
Depends on: 942513
Depends on: 950132
Depends on: 951675
Depends on: 961671
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.