Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove browser.__SS_data and use a WeakMap instead

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: ttaubert, Assigned: Kevin Jones)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

Trunk
Firefox 23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 743573 [details] [diff] [review]
remove browser.__SS_data and use a WeakMap instead
Attachment #743573 - Flags: review?(dteller)
(Reporter)

Updated

4 years ago
Blocks: 867142
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.
Attachment #743573 - Flags: review?(dteller) → review+
(Reporter)

Comment 3

4 years ago
Created attachment 744566 [details] [diff] [review]
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.
Attachment #743573 - Attachment is obsolete: true
Attachment #744566 - Flags: review?(dteller)
Depends on: 868026
Attachment #744566 - Flags: review?(dteller) → review+
(Reporter)

Comment 4

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/6ddd5fb7f041
(Reporter)

Comment 5

4 years ago
https://hg.mozilla.org/mozilla-central/rev/6ddd5fb7f041
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

4 years ago
Blocks: 869900

Comment 6

4 years ago
now that you removed browser.__SS_data how can extension can read the data from the WeakMap ?
I have just opened bug 871246 for this purpose. Can you detail what kind of info you need from __SS_data?
(Reporter)

Comment 8

4 years ago
(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

4 years ago
Thank you Tim,

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

Updated

4 years ago
Blocks: 874381
(Reporter)

Comment 10

4 years ago
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/0d6e59222717
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 11

4 years ago
Backed out from Aurora:

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

Comment 12

4 years ago
I assume getTabValue will get no data if a tab is not fully restored?
(Reporter)

Comment 13

4 years ago
Why are you assuming that?

Comment 14

4 years ago
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.
(Reporter)

Updated

4 years ago
Assignee: ttaubert → nobody
Blocks: 906076
Tim, can you remind me of why this was backed out?
Flags: needinfo?(ttaubert)
(Reporter)

Comment 16

2 years ago
(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.
Flags: needinfo?(ttaubert)
Allasso has volunteered to work on this as this will simplify the code of bug 906076.
Assignee: nobody → allassopraise
(Assignee)

Comment 18

2 years ago
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.
Flags: needinfo?(dteller)
(Reporter)

Comment 19

2 years ago
(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?
(Assignee)

Comment 20

2 years ago
(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.
Flags: needinfo?(dteller) → needinfo?(ttaubert)
(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.
(Assignee)

Comment 22

2 years ago
Created attachment 8605208 [details] [diff] [review]
Replace __SS_data with weak map
Flags: needinfo?(ttaubert)
Attachment #8605208 - Flags: feedback?(ttaubert)
Attachment #8605208 - Flags: feedback?(dteller)
(Assignee)

Comment 23

2 years ago
Created attachment 8605215 [details] [diff] [review]
Replace __SS_data with weak map
Attachment #8605208 - Attachment is obsolete: true
Attachment #8605208 - Flags: feedback?(ttaubert)
Attachment #8605208 - Flags: feedback?(dteller)
(Assignee)

Updated

2 years ago
Attachment #8605215 - Flags: feedback?(ttaubert)
Attachment #8605215 - Flags: feedback?(dteller)
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.
Attachment #8605215 - Flags: feedback?(dteller) → feedback+
(Assignee)

Comment 25

2 years ago
(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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(dteller)
(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.
(Assignee)

Comment 27

2 years ago
Created attachment 8607218 [details] [diff] [review]
Replace __SS_data with WeakMap
Attachment #8605215 - Attachment is obsolete: true
Attachment #8605215 - Flags: feedback?(ttaubert)
Flags: needinfo?(dteller)
Attachment #8607218 - Flags: feedback?(ttaubert)
Attachment #8607218 - Flags: feedback?(dteller)
(Assignee)

Comment 28

2 years ago
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.
Flags: needinfo?(dteller)
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.
Attachment #8607218 - Flags: feedback?(dteller) → feedback+
Flags: needinfo?(dteller)
(Assignee)

Comment 30

2 years ago
Created attachment 8607488 [details] [diff] [review]
Replace __SS_data with WeakMap

Updated patch with testing for xul:tab
Attachment #8607218 - Attachment is obsolete: true
Attachment #8607218 - Flags: feedback?(ttaubert)
Flags: needinfo?(dteller)
Attachment #8607488 - Flags: feedback?(dteller)
(Assignee)

Updated

2 years ago
Flags: needinfo?(dteller)
Comment on attachment 8607488 [details] [diff] [review]
Replace __SS_data with WeakMap

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

Looks good to me.
Attachment #8607488 - Flags: review?(ttaubert)
Attachment #8607488 - Flags: feedback?(dteller)
Attachment #8607488 - Flags: feedback+
(Assignee)

Comment 32

2 years ago
Good.  What's the next step?
Flags: needinfo?(dteller)
(Reporter)

Comment 33

2 years ago
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.
Attachment #8607488 - Flags: review?(ttaubert)
Flags: needinfo?(dteller)
Tim, does your patch cover the fact that bug 906076 needs to use a `tab` instead of a `browser` as key?
(Reporter)

Comment 35

2 years ago
No, it doesn't.
(Reporter)

Comment 36

2 years ago
Fixed by bug 1166757.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.