867118 - Remove browser.__SS_data and use a WeakMap instead

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We should use WeakMaps to store data related to tabs and browsers instead of tacking those properties directly onto the DOM elements. Let's start with __SS_data.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: remove browser.__SS_data and use a WeakMap instead

Comment 2

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

Comment on attachment 743573 [details] [diff] [review]
remove browser.__SS_data and use a WeakMap instead

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

That small change will simplify the code a lot, thanks.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +331,5 @@
>    // "sessionstore.resume_from_crash" is true.
>    _resume_session_once_on_shutdown: null,
>  
> +  // Keeps tab data around until the tab has fully been restored. This is needed
> +  // to be able to modify and/or query tab data while the tab is still loading.

Could you add a little detail about exactly what "tab data" appears in this map? Also, please mention the nature of keys (i.e. tabs).
Also, I would be tempted to move all of this to a toplevel singleton object RestoringTabsData. This will make it a little simpler to hook type-checks to ensure that we never pass |undefined| or a key that is not a tab.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: remove browser.__SS_data and use a WeakMap instead

Moved the map into an objects of its own. I agree, that's a little more modular and extensible.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/6ddd5fb7f041

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6ddd5fb7f041

Comment 6

[tabmix.onemen]

now that you removed browser.__SS_data how can extension can read the data from the WeakMap ?

Comment 7

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

I have just opened bug 871246 for this purpose. Can you detail what kind of info you need from __SS_data?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to onemen.one from comment #6)
> now that you removed browser.__SS_data how can extension can read the data
> from the WeakMap ?

I assume that there might be valid use cases for extensions to read __SS_data (although that's the same as ss.getTabState()). If that's the case and we can be convinced there is a valid reason to use this data instead of using the existing API, we would certainly not hesitate to extend the API.

As David already said, please give us your thoughts in bug 871246.

Comment 9

[tabmix.onemen]

Thank you Tim,

At the moment i can mange to get all the data i need from ss.getTabState() or tab attributes

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Backed out:

https://hg.mozilla.org/integration/fx-team/rev/0d6e59222717

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Backed out from Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/88f70ea0b754

Comment 12

[ithinc]

I assume getTabValue will get no data if a tab is not fully restored?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Why are you assuming that?

Comment 14

[ithinc]

Sorry, I remember it wrong. The real problem I encountered was the persisted attributes are not restored in a timely manner. It seems fixed in Bug 930269. I will retest it sometime.

Comment 15

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

Tim, can you remind me of why this was backed out?

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #15)
> Tim, can you remind me of why this was backed out?

I don't remember exactly, unfortunately. I have a hunch that it was due to coalescing a few breaking changes around Firefox 29? We might break a few add-ons with that but boy would I love to finally get rid of it.

Comment 17

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

Allasso has volunteered to work on this as this will simplify the code of bug 906076.

Comment 18

[u462496]

David, I've been following Tim's lead on this.  __SS_data is now being used in TabState.jsm and webbrowser.js.  Do you have any suggestions on how to expose RestoringTabsData to those scopes?

Also, I am not clear on how this is going to help us in bug 906076, since browser is being used as the key.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Allasso Travesser from comment #18)
> and webbrowser.js.

Looks like that's only for Fennec. And if it wasn't then that's exactly the reason why we're getting rid of those properties :) But you shouldn't need to touch that.

> Do you have any suggestions on how to
> expose RestoringTabsData to those scopes?

We could move RestoringTabsData into its own JSM.

> Also, I am not clear on how this is going to help us in bug 906076, since
> browser is being used as the key.

I hope you're using browser.permanentKey?

Comment 20

[u462496]

(In reply to Tim Taubert [:ttaubert] from comment #19)
> Looks like that's only for Fennec. And if it wasn't then that's exactly the
> reason why we're getting rid of those properties :) But you shouldn't need
> to touch that.
> 
> We could move RestoringTabsData into its own JSM.
> 
Okay, sounds good.

> I hope you're using browser.permanentKey?

No, you were using browser in your patch, so that is what I was looking at.

So for lazy browser tabs, would the tab and the browser share the same key?  The key is created with the tab, then once the browser is instantiated, it gets handed off to browser?  The whole thing we're trying to do in 906076 is be able to hold a reference to the restore data from the tab's lazy browser proxy.  This is so if a caller needs restore data and calls linkedBrowser.__SS_data, the proxy can pass the restore data in its place without needlessly instantiating the browser.  An example of where this is useful is in TabState.jsm -> _collectBaseTabData.  This saves us from having to re-write the code for _collectBaseTabData.  I believe there are other examples as well.

Comment 21

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

(In reply to Allasso Travesser from comment #18)
> David, I've been following Tim's lead on this.  __SS_data is now being used
> in TabState.jsm and webbrowser.js.  Do you have any suggestions on how to
> expose RestoringTabsData to those scopes?
> 
> Also, I am not clear on how this is going to help us in bug 906076, since
> browser is being used as the key.

I believe that we can change this to use tab as the key.

Comment 22

[u462496]

Attachment: Replace __SS_data with weak map

Comment 23

[u462496]

Attachment: Replace __SS_data with weak map

Comment 24

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

Comment on attachment 8605215 [details] [diff] [review]
Replace __SS_data with weak map

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

Looks good to me.
I seem to remember that you need a public API in SessionStore.jsm for the sake of bug 906076, right? Or can you do without now that you have moved the data to the tab?

::: Firefox_39_ORIG/Contents/Resources/browser/omni/modules/sessionstore/RestoringTabsData.jsm
@@ +13,5 @@
> +// having restored fully.
> +let RestoringTabsData = {
> +  _data: new WeakMap(),
> +
> +  has: function (tab) {

For sanity's sake, I'd prefer if we checked that `tab` is a xul:tab in all these methods.

Comment 25

[u462496]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #24)
> I seem to remember that you need a public API in SessionStore.jsm for the
> sake of bug 906076, right? Or can you do without now that you have moved the
> data to the tab?

I don't recall having to expose an API.  I suppose if that needs to be done, that would fall under bug 906076, and not this one, is that right?

> For sanity's sake, I'd prefer if we checked that `tab` is a xul:tab in all
> these methods.

Hmm, how do you do that?  Do you mean is it a tabbrowser tab?

Comment 26

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

(In reply to Allasso Travesser from comment #25)
> I don't recall having to expose an API.  I suppose if that needs to be done,
> that would fall under bug 906076, and not this one, is that right?

Fair enough.

> > For sanity's sake, I'd prefer if we checked that `tab` is a xul:tab in all
> > these methods.
> 
> Hmm, how do you do that?  Do you mean is it a tabbrowser tab?

Check the tag name and namespace, I guess.

Comment 27

[u462496]

Attachment: Replace __SS_data with WeakMap

Comment 28

[u462496]

Do you see a point in testing in all of the methods?  It seems that if we test in the setter, it never gets set so everything else is covered by default.

Comment 29

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

Comment on attachment 8607218 [details] [diff] [review]
Replace __SS_data with WeakMap

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

Well, attempting to `get` with a wrong key will produce subtle errors just as attempting to `set` with a wrong key, so I'd prefer if you typechecked in all cases. You'll probably want to make this a method.

::: Firefox_39_ORIG/Contents/Resources/browser/omni/modules/sessionstore/RestoringTabsData.jsm
@@ +14,5 @@
> +let RestoringTabsData = {
> +
> +  xulNS : "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> +
> +  _data: new WeakMap(),

Nit: Could you take the opportunity to mention that keys are xul:browser and values are JS objects?

@@ +27,5 @@
> +
> +  set: function (tab, data) {
> +    if (tab.namespaceURI == this.xulNS && tab.localName == "tab") {
> +      this._data.set(tab, data);
> +    }

Please throw a TypeError if the type of `tab` is incorrect.

Comment 30

[u462496]

Attachment: Replace __SS_data with WeakMap

Updated patch with testing for xul:tab

Comment 31

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

Comment on attachment 8607488 [details] [diff] [review]
Replace __SS_data with WeakMap

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

Looks good to me.

Comment 32

[u462496]

Good.  What's the next step?

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8607488 [details] [diff] [review]
Replace __SS_data with WeakMap

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

I feel very shitty about doing that but just this morning I was working on bug 1162871 and discovered that the TabStateCache actually covers the most important use cases for __SS_data and we can probably remove it completely without introducing a new JSM. I filed bug 1166757 to try and get rid of it completely. I wrote the patch to see if it was possible to do that at all and when that was done I didn't really want to come back here and give feedback to arrive at the same result. Sorry.

Comment 34

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

Tim, does your patch cover the fact that bug 906076 needs to use a `tab` instead of a `browser` as key?

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

No, it doesn't.

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Fixed by bug 1166757.