1167923 - Use xul:tab instead of xul:browser as key in TabStateCache

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

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

Bug 906076 will make xul:browser instantiation lazy. To avoid forcing instantiation too early, we need to use xul:tab instead of xul:browser wherever possible, in particular with the TabStateCache.

So, this bug is about changing the TabStateCache to use xul:tab as keys.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

We only have a browser reference after the tab was removed to access data. I don't see how this is going to work with a tab reference as that has long gone away at that point.

Comment 2

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

Alessio's patch in bug 867118 managed to use a xul:tab instead of a xul:browser for the removal of SS_data. What's the difference here?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

The difference is that this surely won't work with bug 1109875, including possible follow-up changes. Highest priority atm is to get rid of CPOWs and make sessionstore async.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

What might work is to put the permanentKey on the <xul:tab> as well and pass either a tab or browser to TabStateCache routines where possible. We're using that key as "the key" internally anyway.

Comment 5

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

Can you mentor Allasso on that? This is blocking his work faster startup.

Comment 6

[u462496]

Attachment: Replace xul:browser with xul:tab as key in TabStateCache

I also replaced xul:tab for xul:browser as key in copyFromCache, as it was a trivial change and it made the changes for TabStateCache simpler.  If you think this should have been a separate bug, that's fine, I'll split them up.

Comment 7

[u462496]

Comment on attachment 8609854 [details] [diff] [review]
Replace xul:browser with xul:tab as key in TabStateCache

I didn't see the updated comments before uploading this patch.

Comment 8

[u462496]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> What might work is to put the permanentKey on the <xul:tab> as well and pass
> either a tab or browser to TabStateCache routines where possible. We're
> using that key as "the key" internally anyway.

Since a caller can be either xul:browser or xul:tab, should we use permanentKey as the argument to callers of TabStateCache, or get permanentKey reference inside of TabStateCache as we have been?

Comment 9

[u462496]

Attachment: bug_1167923_patch_2.diff

xul:tab and xul:browser share the same permanentKey, allowing changes in SessionStore.jsm to use tab for key in TabStateCache when available.  Please advise on this: I did not test for xul:tab or xul:browser in TabStateCache since we are using permanentKey, which seems would provide the sanity check.

Comment 10

[u462496]

Attachment: bug_1167923_patch_2.diff

updated, only using tab as key in restoreTab in SessionStore.jsm since at this time, for bug 906076 purposes it does not seem necessary in receiveMessage.

Comment 11

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

Comment on attachment 8609910 [details] [diff] [review]
bug_1167923_patch_2.diff

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

::: mozilla_central/browser/base/content/tabbrowser.xml
@@ +1592,5 @@
>          </body>
>        </method>
>  
>        <method name="_createPreloadBrowser">
> +        <parameter name="aPemanentKey"/>

I don't think you should set the permanent key in the preloaded browser. If my memory serves, you don't know in which tab it will end up, right?

@@ +1622,5 @@
>  
>              let remote = aParams && aParams.remote;
>              let uriIsAboutBlank = aParams && aParams.uriIsAboutBlank;
>              let isPreloadBrowser = aParams && aParams.isPreloadBrowser;
> +            let permanentKey = (aParams && aParams.permanentKey) || {};

For the sake of debugging, I'd rather have something a little easier to identify than a raw `{}`. Could we have something along the liens of `Object.freeze({permamentkey: null})`?

@@ +1726,5 @@
>              var t = document.createElementNS(NS_XUL, "tab");
>  
>              var uriIsAboutBlank = !aURI || aURI == "about:blank";
>  
> +            t.permanentKey = {};

Here, too.

@@ +1780,1 @@
>              }

Rather than setting the permanent key in `_createBrowser`, I believe that you should set it in this method, along the line in which `_tabForBrowser` is set.

::: mozilla_central/browser/components/sessionstore/TabStateCache.jsm
@@ +17,5 @@
>   * - caching private data in addition to public data is memory consuming.
>   */
>  this.TabStateCache = Object.freeze({
>    /**
> +   * Retrieves cached data for a given |mapKey|.

Why not call it `permanentKey`?

@@ +27,2 @@
>     */
> +  get: function (mapKey) {

If you make the permanent key a little bit more recognizable, you'll be able to add type-checking here.

Comment 12

[u462496]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11)
> I don't think you should set the permanent key in the preloaded browser. If
> my memory serves, you don't know in which tab it will end up, right? 

Is there ever a case where permanentKey is used on a preloaded browser?

If we don't set a permanentKey in _createBrowser, and a preloaded browser is created, xul:browser won't have a permanentKey.  Is that okay then, does it not need one until it is associated with a tab?

> ::: mozilla_central/browser/components/sessionstore/TabStateCache.jsm
> @@ +17,5 @@
> >   * - caching private data in addition to public data is memory consuming.
> >   */
> >  this.TabStateCache = Object.freeze({
> >    /**
> > +   * Retrieves cached data for a given |mapKey|.
> 
> Why not call it `permanentKey`?
> 
> @@ +27,2 @@
> >     */
> > +  get: function (mapKey) {
> 
> If you make the permanent key a little bit more recognizable, you'll be able
> to add type-checking here.

This seems confusing to me.  We are not passing permanentKey as an argument to the methods.  We are passing either xul:tab or xul:browser.  TabStateCache methods then pass permanentKey to the WeakMap methods.  We would end up with something like:

  get: function (permanentKey) {
    return this._data.get(permanentKey.permanentKey);
  },

Comment 13

[u462496]

> This seems confusing to me.  We are not passing permanentKey as an argument
> to the methods.  We are passing either xul:tab or xul:browser. 
> TabStateCache methods then pass permanentKey to the WeakMap methods.  We
> would end up with something like:
> 
>   get: function (permanentKey) {
>     return this._data.get(permanentKey.permanentKey);
>   },

I suppose another option would be to have the callers of TabStateCache use permanentKey as the argument, so we are only dealing with permanentKey in TabStateCache.

Comment 14

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

> I suppose another option would be to have the callers of TabStateCache use permanentKey as the argument, so we are only dealing with permanentKey in TabStateCache.

Sorry for being unclear, that's what I meant. Alternatively, if you prefer passing the tab or browser, as you do now, just call the argument `key` and document that it's either a xul:tab or a xul:browser.

Comment 15

[u462496]

David, did you see my first question in comment 12?

Comment 16

[u462496]

To clarify,

Is browser.permanentKey only used on browsers which are already associated with a tab?

If we don't set permanentKey in _createBrowser, and a preloaded browser is created, it will not have a permanentKey.  Is that okay then?

Comment 17

[u462496]

Attachment: Associate permanentKey with tab and browser, use tab or browser as arg for TabStateCache

Comment 18

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

I'm 99% sure that `permanentKey` is only used if the browser is in a tab.
If you want to be 100% sure, you can add a getter that throws if the browser is not in a tab, but I think that this is overkill.

Comment 19

[u462496]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #18)
> I'm 99% sure that `permanentKey` is only used if the browser is in a tab.
> If you want to be 100% sure, you can add a getter that throws if the browser
> is not in a tab, but I think that this is overkill.

I came to the same conclusion after examining the code, so I made that assumption on the patch I uploaded this morning.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #18)
> I'm 99% sure that `permanentKey` is only used if the browser is in a tab.
> If you want to be 100% sure, you can add a getter that throws if the browser
> is not in a tab, but I think that this is overkill.

The permanentKey is needed as soon as the frameLoader is created. We need it for the preloaded browser for example, the SyncHandler won't otherwise get registered. After the tab was closed and the <xul:tab> is gone the <browser> is still alive and sends messages, here we also need the permanentKey. Why do we need to change anything about the permanentKey itself? I thought this is bug is only about changing the TabStateCache API.

Comment 21

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

Well, the idea is to duplicate the `permanentKey` and have it both in the tab and in the browser, so as to be able to use either as keys for `TabStateCache`.

Do you have other ideas to be able to use a xul:tab instead of a xul:browser as keys for TabStateCache when we don't have a xul:browser at hand? Because that was the point of Allasso's patch in bug 867118.

Comment 22

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

Comment on attachment 8611193 [details] [diff] [review]
Associate permanentKey with tab and browser, use tab or browser as arg for TabStateCache

(clearing f? until that question is resolved)

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #21)
> Well, the idea is to duplicate the `permanentKey` and have it both in the
> tab and in the browser, so as to be able to use either as keys for
> `TabStateCache`.
> 
> Do you have other ideas to be able to use a xul:tab instead of a xul:browser
> as keys for TabStateCache when we don't have a xul:browser at hand? Because
> that was the point of Allasso's patch in bug 867118.

Not sure I understand the question. As soon as the browser is linked to a tab, the tab should simply get the same permanentKey as the browser. And as I said before, this isn't really part of this bug anyway, we want to change the TabStateCache API to accept either or just the permanentKey where possible. How you deal with and manage the permanentKey is out of scope here.

Comment 24

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

Actually, we probably don't need this bug at all.
Allasso, I believe that it would be simple to adapt your patch of bug 906076 to:
- generate the permanentKey immediately instead of waiting for the browser to be linked;
- make `permanentKey` be a property of the browser that can be accessed through the Proxy without having to instantiate.

Right? If so, we can move back to bug 906076.

Comment 25

[u462496]

The only hitch I see to this is that Tim pointed out that `permanentKey` is needed on preload browser, which the proxy would not cover.

One thought I have is if it still seems advantageous for bug 906076 to have xul:tab and xul:browser share the same permanentKey, we could just land only the changes to TabStateCache.jsm, which would be in the scope of this bug, and any changes we want to make to `permanentKey` to support lazy browser tabs can be done in bug 906076.

Comment 26

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

Tim, as you know the permanentKey better than me, could you discuss a valid strategy with Allasso?

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

I don't know the exact details of the problem but basically, whenever you create a <browser> it needs a permanentKey. Where that comes from doesn't matter. Currently it's created in by the tabbrowser whenever that creates a browser. If the permanentKey comes from a pending tab (one created without a linked browser) then that's also fine. Both the tab and its linked browser should of course share a permanentKey.

Comment 28

[u462496]

(In reply to Tim Taubert [:ttaubert] from comment #27)
> I don't know the exact details of the problem but basically, whenever you
> create a <browser> it needs a permanentKey. Where that comes from doesn't
> matter. Currently it's created in by the tabbrowser whenever that creates a
> browser. If the permanentKey comes from a pending tab (one created without a
> linked browser) then that's also fine. Both the tab and its linked browser
> should of course share a permanentKey.

Thanks, Tim, I have a strategy in mind that will follow those lines.  Unless there are any druthers from anyone, I will submit a patch for this bug which will only change TabStateCache.jsm; mainly documentation to reflect the acceptance of using either xul:tab or xul:browser, in either case still using `permanentKey` as the actual key for the `WeakMap`.

Then we can move back to bug 906076 where I will implement the strategy for creating the `permanentKey`.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

SGTM. Don't forget to update argument names like "browser" to "browserOrTab" or similar.

Comment 30

[u462496]

Attachment: Use xul:tab or xul:browser as arg for TabStateCache

Comment 31

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

Seems goot to me. Sorry for the lag.

Comment 32

[u462496]

(In reply to David Rajchenbach-Teller [:Yoric] (away June 22 - July 7th, use "needinfo") from comment #31)
> Seems goot to me. Sorry for the lag.

Should I request a review?  From you, or Tim?

Comment 33

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

I'd say Tim, since your patch was written in reaction to one of his comments.

Comment 34

[u462496]

I don't know if I have done this right; I have been following the guidelines at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F.  I've made changes to my local repos, verified with `hg status`.  Somewhere down the line after `hg commit` (I think), `hg status` no longer gives output, although the updated file in my local repos has not changed.  Does this mean the remote repos was updated with my patch?

I stopped short of the "How do I check stuff in?" section, ie, I have not `hg push`ed.

I'm a bit in the dark as to what has happened, and how to proceed next.

Comment 35

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

`hg commit` doesn't actually push anything to the repo. To push something to the repo, you need to
1/ First push it to the Try server. I've done it for you here:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e63eb01374c
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=531b7b87a040

2/ If the tests are positive, attach the patch and mark the bug as "checkin-needed". You may not have rights to do that, I never quite understood how Bugzilla authorizations work.

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8617907 [details] [diff] [review]
Use xul:tab or xul:browser as arg for TabStateCache

Can you please add a proper description to the patch? Should be "Bug 12345 - Description of what the patch does or the bug summary r=ttaubert"? Once that is uploaded simply add the "checkin-needed" keyword at the top.

Comment 37

[u462496]

(In reply to Tim Taubert [:ttaubert] from comment #36)
> Comment on attachment 8617907 [details] [diff] [review]
> Use xul:tab or xul:browser as arg for TabStateCache
> 
> Can you please add a proper description to the patch? Should be "Bug 12345 -
> Description of what the patch does or the bug summary r=ttaubert"? Once that
> is uploaded simply add the "checkin-needed" keyword at the top.

Done. changeset:   220139:5a13f43e5a50

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

You need to export and attach a new patch that has the correct summary.

Comment 39

[u462496]

I don't understand, please tell me what I am doing wrong:

hg log -k Allasso
changeset:   220139:5a13f43e5a50
tag:         tip
user:        Allasso Travesser <[e-mail]>
date:        Wed Jun 24 06:56:07 2015 -0600
summary:     Bug 1167923 - Change TabStateCache to use either xul:tab or xul:browser as key r=ttaubert

Comment 40

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

In hg (or git), `commit` doesn't mean that your patch has been uploaded, only that it is committed *locally*. It still needs to be `push`ed to the official repository. For this, since you do not have the rights to push yourself yet, you need to attach the updated patch to this bug and then mark the bug as checkin-needed.

Comment 41

[u462496]

Attachment: bug_1167923_2.patch

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

Great, thanks! Sorry for not being more clear about the requirements. Now let's wait until it's checked in.

Comment 43

[u462496]

(In reply to Tim Taubert [:ttaubert] from comment #42)
> Great, thanks! Sorry for not being more clear about the requirements. Now
> let's wait until it's checked in.

Oh my, I'm sorry I am so slow at getting this stuff; I've never really worked with repositories before and when I have tried the concept has baffled me for some reason.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/732640286a20

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/732640286a20