Closed Bug 951675 Opened 8 years ago Closed 8 years ago

New data collected by the frame script is ignored as long as we don't invalidate the TabStateCache

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 + fixed
firefox29 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

For normal data collection, we don't call _copyFromPersistentCache() when TabStateCache.has() == true. This is bad as the whole idea is to let the frame script control invalidation and collect data. We should also make sure that the persistent data isn't duplicated in the normal tab state cache.
Another thing worth to mention is that we won't keep this forever. The two caches are only used while transitioning to broadcasting session data. As soon as everything is collected in the frame script asynchronously we don't need the tab state cache *and* the persistent tab state cache - all we then have is the persistent one left.
Comment on attachment 8349443 [details] [diff] [review]
0001-Bug-951675-Make-sure-to-always-copy-data-from-the-pe.patch

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

Patch looks good, but I'm not sure I understand the point.
With this patch, we're only protecting against shallow changes, so what's the objective?
Attachment #8349443 - Flags: review?(dteller) → feedback+
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #3)
> Patch looks good, but I'm not sure I understand the point.
> With this patch, we're only protecting against shallow changes, so what's
> the objective?

I'm not trying to protect against changes. When we have scroll data we put it in tabData.scroll. If that's now going into the TabStateCache this property won't go away until something calls TabStateCache.delete(). The user scrolls up and we send scroll data "null" which removes the key from the persistent cache. Thus _copyFromPersistentCache() can't override it and we now have stale scroll data.
Comment on attachment 8349443 [details] [diff] [review]
0001-Bug-951675-Make-sure-to-always-copy-data-from-the-pe.patch

:)
Attachment #8349443 - Flags: review?(dteller)
Comment on attachment 8349443 [details] [diff] [review]
0001-Bug-951675-Make-sure-to-always-copy-data-from-the-pe.patch

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

After our chat, r+, but please make sure that the comments explain much more clearly why you do this.
Attachment #8349443 - Flags: review?(dteller)
Attachment #8349443 - Flags: review+
Attachment #8349443 - Flags: feedback+
https://hg.mozilla.org/mozilla-central/rev/b45acb2d43a4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Let's get an uplift nomination here, if low risk we should take it on Aurora so as never to ship this.
Comment on attachment 8349443 [details] [diff] [review]
0001-Bug-951675-Make-sure-to-always-copy-data-from-the-pe.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 930967
User impact if declined: Various data collected by session store could possibly be discarded on a per-tab basis. AFAICT there have been no reports but this a precautionary measure.
Testing completed (on m-c, etc.): Works fine on m-c for a while now.
Risk to taking this patch (and alternatives if risky): Rather low risk.
String or IDL/UUID changes made by this patch: None.
Attachment #8349443 - Flags: approval-mozilla-aurora?
Depends on: 958809
Attachment #8349443 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Does this or should this have in-testsuite coverage? If not is there something QA can do to verify this is fixed?
Flags: in-testsuite?
This also is probably impossible to verify manually. It was more a precaution than a bug fix and I don't have a great idea how to test this. Luckily it won't be around long and will be removed with bug 960903.
Whiteboard: [qa-]
Setting in-testsuite- since this was removed with bug 960903
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.