Closed Bug 867118 Opened 11 years ago Closed 9 years ago

Remove browser.__SS_data and use a WeakMap instead

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: ttaubert, Assigned: u462496)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 4 obsolete files)

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.
Attachment #743573 - Flags: review?(dteller)
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+
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)
Attachment #744566 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/6ddd5fb7f041
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Blocks: 869900
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?
(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.
Thank you Tim,

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

https://hg.mozilla.org/integration/fx-team/rev/0d6e59222717
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I assume getTabValue will get no data if a tab is not fully restored?
Why are you assuming that?
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.
Assignee: ttaubert → nobody
Tim, can you remind me of why this was backed out?
Flags: needinfo?(ttaubert)
(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
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)
(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?
(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.
Attached patch Replace __SS_data with weak map (obsolete) — Splinter Review
Flags: needinfo?(ttaubert)
Attachment #8605208 - Flags: feedback?(ttaubert)
Attachment #8605208 - Flags: feedback?(dteller)
Attached patch Replace __SS_data with weak map (obsolete) — Splinter Review
Attachment #8605208 - Attachment is obsolete: true
Attachment #8605208 - Flags: feedback?(ttaubert)
Attachment #8605208 - Flags: feedback?(dteller)
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+
(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?
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.
Attached patch Replace __SS_data with WeakMap (obsolete) — Splinter Review
Attachment #8605215 - Attachment is obsolete: true
Attachment #8605215 - Flags: feedback?(ttaubert)
Flags: needinfo?(dteller)
Attachment #8607218 - Flags: feedback?(ttaubert)
Attachment #8607218 - Flags: feedback?(dteller)
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)
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)
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+
Good.  What's the next step?
Flags: needinfo?(dteller)
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?
No, it doesn't.
Fixed by bug 1166757.
Status: REOPENED → RESOLVED
Closed: 11 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.