If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove superfluous __SS_tabStillLoading property

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {addon-compat})

Trunk
Firefox 28
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
browser.__SS_data is used to retain a tab's data until it has fully been restored. When the tab has loaded and SSTabRestored fires, the property is deleted. Now, for some probably historical reason it was decided to be re-used to cache session history entries.

We should clean up and clarify the code by just removing the cache for now. It seems like browser.__SS_tabStillLoading was only introduced to tell the reason __SS_data exists apart: if stillLoading=true then it's the tab data until it's been restored. If stillLoading=false then it's the history entry cache.
(Assignee)

Updated

4 years ago
Blocks: 687814
(Assignee)

Comment 1

4 years ago
Created attachment 743571 [details] [diff] [review]
remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property
Attachment #743571 - Flags: review?(dteller)
(Assignee)

Updated

4 years ago
Blocks: 867118
Comment on attachment 743571 [details] [diff] [review]
remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property

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

Could you take the opportunity to add a section documenting (some) of the _SS_* fields that session restore adds to DOM objects?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1256,2 @@
>      delete browser.__SS_formDataSaved;
>      delete browser.__SS_hostSchemeData;

Site-note: We should consider getting rid of all these semantically incorrect |delete foo.bar| and replace them with |foo.bar = undefined|.

::: browser/components/sessionstore/test/browser_579868.js
@@ -18,5 @@
>      // Undo pinning
>      gBrowser.unpinTab(tab1);
>  
> -    is(tab1.linkedBrowser.__SS_tabStillLoading, true,
> -       "_tabStillLoading should be true.");

Should we check tab1.linkedBrowser.__SS_data?

::: browser/components/sessionstore/test/browser_625257.js
@@ -38,5 @@
>    // Trigger a save state.
>    ss.getBrowserState();
>  
>    is(gBrowser.tabs[1], tab, "newly created tab should exist by now");
> -  ok(tab.linkedBrowser.__SS_data, "newly created tab should be in save state");

Now, we could even reverse the test, couldn't we?
Attachment #743571 - Flags: review?(dteller) → review+
(Assignee)

Comment 3

4 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> >      delete browser.__SS_formDataSaved;
> >      delete browser.__SS_hostSchemeData;
> 
> Site-note: We should consider getting rid of all these semantically
> incorrect |delete foo.bar| and replace them with |foo.bar = undefined|.

I agree, let's just slap anyone using 'delete' :)

> > -    is(tab1.linkedBrowser.__SS_tabStillLoading, true,
> > -       "_tabStillLoading should be true.");
> 
> Should we check tab1.linkedBrowser.__SS_data?

Yeah, good point.

> >    is(gBrowser.tabs[1], tab, "newly created tab should exist by now");
> > -  ok(tab.linkedBrowser.__SS_data, "newly created tab should be in save state");
> 
> Now, we could even reverse the test, couldn't we?

Technically, we could reverse the test because the tab should've stopped loading by now. OTOH this has nothing to do with the actual test and is probably quite confusing. I'm not even sure why the test originally checked that the __SS_data cache exists as this seems not really specific to the issue addressed.
(Assignee)

Comment 4

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/4a496e6b99af
Blocks: 868026
(Assignee)

Comment 5

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

Updated

4 years ago
Blocks: 869900

Updated

4 years ago
Depends on: 873835
Blocks: 874381
(Assignee)

Comment 6

4 years ago
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/222621f74e96
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

4 years ago
Backed out from Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5a7dc83250b
(Assignee)

Comment 8

4 years ago
Created attachment 754775 [details] [diff] [review]
remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property

Rebased and removed leftover code that sets the now unused '_sessionhistory_max_entries' property. Carrying over r+.
Attachment #743571 - Attachment is obsolete: true
Attachment #754775 - Flags: review+
Depends on: 871246
(Assignee)

Updated

4 years ago
Target Milestone: Firefox 23 → ---
(Assignee)

Comment 9

4 years ago
Created attachment 8336706 [details] [diff] [review]
0001-Bug-867097-Remove-superfluous-__SS_tabStillLoading-p.patch

Now that the __SS_data changes from the previous patch here have already landed I don't think there's much need to withhold this until Australis lands. Very few add-ons will be affected by removing __SS_tabStillLoading and most of them will still work afaict from reading the sources.

To re-iterate: browser.__SS_tabStillLoading was only introduced to tell the reason __SS_data exists apart: if stillLoading=true then it's the tab data until it's been restored. If stillLoading=false then it's the history entry cache.

Said history entry cache has been removed moons ago and thus nowadays (!!__SS_data) == (!!__SS_tabStillLoading). Easy to get rid of.
Attachment #754775 - Attachment is obsolete: true
Attachment #8336706 - Flags: review?(dteller)
(Assignee)

Updated

4 years ago
Summary: Remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property → Remove superfluous __SS_tabStillLoading property
Keywords: addon-compat
Comment on attachment 8336706 [details] [diff] [review]
0001-Bug-867097-Remove-superfluous-__SS_tabStillLoading-p.patch

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

Yeah, one __SS_* gone!
Attachment #8336706 - Flags: review?(dteller) → review+
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/b285dc236bef
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b285dc236bef
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.