Closed
Bug 916390
Opened 11 years ago
Closed 11 years ago
Remove browser.__SS_formDataSaved
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 27
People
(Reporter: billm, Assigned: ttaubert)
References
Details
Attachments
(1 file)
10.72 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•11 years ago
|
||
I think
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Assignee | ||
Comment 4•11 years ago
|
||
This bug has probably existed before, though. We just didn't need step (4) because there was no cache in between.
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
I added a test for the ancient bug anyway.
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/86eff36caecf
Whiteboard: [fixed-in-fx-team]
Comment 10•11 years ago
|
||
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
Updated•11 years ago
|
Summary: Figure out what to do about browser.__SS_formDataSaved → Remove browser.__SS_formDataSaved
You need to log in
before you can comment on or make changes to this bug.
Description
•