Closed Bug 1167923 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: Yoric, Assigned: u462496)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Assignee: nobody → allassopraise
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.
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?
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.
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.
Can you mentor Allasso on that? This is blocking his work faster startup.
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.
Attachment #8609854 - Flags: review?(dteller)
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.
Attachment #8609854 - Attachment is obsolete: true
Attachment #8609854 - Flags: review?(dteller)
(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?
Flags: needinfo?(ttaubert)
Attached patch bug_1167923_patch_2.diff (obsolete) — Splinter Review
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.
Attachment #8609907 - Flags: feedback?(ttaubert)
Attachment #8609907 - Flags: feedback?(dteller)
Attached patch bug_1167923_patch_2.diff (obsolete) — Splinter Review
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.
Attachment #8609910 - Flags: feedback?(ttaubert)
Attachment #8609910 - Flags: feedback?(dteller)
Attachment #8609907 - Attachment is obsolete: true
Attachment #8609907 - Flags: feedback?(ttaubert)
Attachment #8609907 - Flags: feedback?(dteller)
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.
Attachment #8609910 - Flags: feedback?(dteller) → feedback+
(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);
  },
Flags: needinfo?(dteller)
Attachment #8609910 - Flags: feedback?(ttaubert)
Attachment #8609910 - Flags: feedback-
Attachment #8609910 - Flags: feedback+
Attachment #8609910 - Flags: feedback-
> 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.
> 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.
Flags: needinfo?(dteller)
David, did you see my first question in comment 12?
Flags: needinfo?(dteller)
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?
Attachment #8609910 - Attachment is obsolete: true
Attachment #8611193 - Flags: feedback?(dteller)
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.
Flags: needinfo?(dteller)
(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.
(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.
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert)
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)
Attachment #8611193 - Flags: feedback?(dteller)
(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.
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(allassopraise)
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.
Flags: needinfo?(allassopraise) → needinfo?(dteller)
Tim, as you know the permanentKey better than me, could you discuss a valid strategy with Allasso?
Flags: needinfo?(dteller) → needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert)
(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`.
Flags: needinfo?(ttaubert)
Flags: needinfo?(dteller)
SGTM. Don't forget to update argument names like "browser" to "browserOrTab" or similar.
Flags: needinfo?(ttaubert)
Attachment #8611193 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #8617907 - Flags: feedback?(ttaubert)
Attachment #8617907 - Flags: feedback?(dteller)
Attachment #8617907 - Flags: feedback?(ttaubert) → feedback+
Attachment #8617907 - Flags: feedback?(dteller) → feedback+
Seems goot to me. Sorry for the lag.
(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?
I'd say Tim, since your patch was written in reaction to one of his comments.
Attachment #8617907 - Flags: review?(ttaubert)
Attachment #8617907 - Flags: review?(ttaubert) → review+
Status: NEW → ASSIGNED
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.
Flags: needinfo?(ttaubert)
Flags: needinfo?(dteller)
Attachment #8617907 - Flags: checkin?(ttaubert)
`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.
Flags: needinfo?(dteller)
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.
Flags: needinfo?(ttaubert)
Attachment #8617907 - Flags: checkin?(ttaubert)
(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
Keywords: checkin-needed
You need to export and attach a new patch that has the correct summary.
Keywords: checkin-needed
I don't understand, please tell me what I am doing wrong:

hg log -k Allasso
changeset:   220139:5a13f43e5a50
tag:         tip
user:        Allasso Travesser <allassopraise@gmail.com>
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
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert)
Keywords: checkin-needed
Attachment #8617907 - Attachment is obsolete: true
Great, thanks! Sorry for not being more clear about the requirements. Now let's wait until it's checked in.
(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.
https://hg.mozilla.org/mozilla-central/rev/732640286a20
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.