Closed Bug 916390 Opened 11 years ago Closed 11 years ago

Remove browser.__SS_formDataSaved

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: billm, Assigned: ttaubert)

References

Details

Attachments

(1 file)

I was looking at the form saving code, and I can't quite figure out this field. In theory, it acts like a cache so that we don't collect form data if it hasn't changed. However, there doesn't seem to be any placed to save the cached form data, as far as I can see. That is, if browser.__SS_formDataSaved is true, we just don't appear to save any form data for the given tab in sessionstore.js.

The problem is mostly mitigated by the TabStateCache, since we don't typically collect data for a page more than once. However, I wonder how this worked before the TabStateCache was implemented. Perhaps I've misread the code?

In any case, it would be nice if we could just eliminate this field. Now that we have the TabStateCache, I doubt that it saves us much work. What do you guys think?
I think
Bah. I think that in its current state we might even be losing form data when:

1) form data is saved and formDataSaved=true
2) something calls TabStateCache.delete(tab)
3) we re-collect all data except form data

Boom, lost. This flag assumes that the tabData is re-used in _collectBaseTabData() which is only the case if the session history didn't change.
Ok it's a little more complex than that STR above:

1) have a tab with inputs and modify them
2) form data is saved and formDataSaved=true
3) append an "#" to the tab's URI
4) change sessionstorage data
5) collect data for the tab

The tab should still have the same form data but the number of session history entries changed and that makes us re-collect session history entries (and form data). Due to formDataSaved still being true we skip form data collection though.
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
This bug has probably existed before, though. We just didn't need step (4) because there was no cache in between.
Removing __SS_formDataSaved shouldn't really hurt as well because MozStorageChanged seems to be the only event where we call TabStateCache.delete() without at the same time setting formDataSaved=false. There shouldn't be a lot more overhead now when always collecting form data.
I added a test for the ancient bug anyway.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #805340 - Flags: review?(dteller)
Comment on attachment 805340 [details] [diff] [review]
Remove __SS_formDataSaved

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2269,5 @@
>      var isHTTPS = this._getURIFromString((aContent.parent || aContent).
>                                           document.location.href).schemeIs("https");
>      let topURL = aContent.top.document.location.href;
>      let isAboutSR = topURL == "about:sessionrestore" || topURL == "about:welcomeback";
>      if (aIncludePrivateData || this.checkPrivacyLevel(isHTTPS, aIsPinned) || isAboutSR) {

I don't quite understand why there was a |if (aIncludePrivateData)|, but then, I'm not sure why you have removed it.

::: browser/components/sessionstore/test/Makefile.in
@@ +135,5 @@
>  	browser_739805.js \
>  	browser_819510_perwindowpb.js \
>  	browser_833286_atomic_backup.js \
> +	browser_916390.js \
> +	browser_916390_sample.html \

Couldn't we take an opportunity to give a better name to the test?
Attachment #805340 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> >      if (aIncludePrivateData || this.checkPrivacyLevel(isHTTPS, aIsPinned) || isAboutSR) {
> 
> I don't quite understand why there was a |if (aIncludePrivateData)|, but

Because we always want to re-collect form data when cloning a tab, as that might include form data from sites that would usually be excluded due to the current privacy level.

> then, I'm not sure why you have removed it.

The parameter aUpdateFormData was removed because it's always true now. We always want to collect form data. With aUpdateFormData=true the condition (aIncludePrivateData || aUpdateFormData) becomes (aIncludePrivateData || true). So I just removed it.

> ::: browser/components/sessionstore/test/Makefile.in
> > +	browser_916390.js \
> > +	browser_916390_sample.html \
> 
> Couldn't we take an opportunity to give a better name to the test?

Sure.
https://hg.mozilla.org/mozilla-central/rev/86eff36caecf
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Summary: Figure out what to do about browser.__SS_formDataSaved → Remove browser.__SS_formDataSaved
Blocks: 869900
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: