Tabs restored from last session show stale cache data

RESOLVED FIXED in Firefox 4.0b11

Status

defect
P3
normal
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 4.0b11
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

There are three issues with tabs restored from last session:

1.) When re-using a restored tab and navigating to a different website _before_ panorama was loaded the tabitem is not updated and shows stale cache data.

2.) When entering panorama for the first time restored tabs are not update when resized.

3.) Enter panorama for the first time and then close it. Navigate to a different website in a restored tab. Enter panorama again and see that the tab's title is still showing stale cache data.
(Assignee)

Updated

8 years ago
Summary: Tabs restored from last session show stale cached data → Tabs restored from last session show stale cache data
Posted patch patch v1, WIP (obsolete) — Splinter Review
This patch fixes all three issues. If that's the right approach I'll write a test :)
Attachment #505343 - Flags: feedback?(ian)
Comment on attachment 505343 [details] [diff] [review]
patch v1, WIP

Thanks for hunting down these issues! 

>-    if (tabData && TabItems.storageSanity(tabData)) {
>+    if (tabData && tabData.url == this.tab.linkedBrowser.currentURI.spec &&
>+        TabItems.storageSanity(tabData)) {

If the URL has changed, we still want to reconnect. Otherwise a tab that had been in a group will suddenly be misplaced. This check would be good if applied only to the cached image and title. 

>       if (rect.width != this.bounds.width || options.force) {
>+        // if the item is resized for the first time while showing cached data
>+        // we should hide the cached data to let the thumbnail update accordingly
>+        if (!options.force && this._hasBeenDrawn && this.isShowingCachedData())
>+          this.shouldHideCachedData = true;

Theoretically we only show the cached image until the real page has loaded enough that we can get a decent thumbnail off it. If that's the case, this new test is a bad idea, as it would throw away a blurry but complete thumbnail in favor of a sharp but incomplete (or even blank) thumbnail. 

If, however, we're not hiding the cached image until long after the real page has loaded, or if in this resize case we just never get around to updating the thumbnail even after the cached image is hidden, then those themselves are problems that should be fixed.

>-      if (!tabItem.isShowingCachedData() && $name.text() != label)
>+      let labelUpdateIsAllowed = !tabItem.isShowingCachedData() ||
>+                                 tabItem.shouldHideCachedData;
>+      if (labelUpdateIsAllowed && $name.text() != label)
>         $name.text(label);

Looks good.
Attachment #505343 - Flags: feedback?(ian) → feedback-
Blocks: 627096
Priority: -- → P3
Bug 627239 is the important one in this domain.
Posted patch patch v2 (obsolete) — Splinter Review
(In reply to comment #2)
> If the URL has changed, we still want to reconnect. Otherwise a tab that had
> been in a group will suddenly be misplaced. This check would be good if applied
> only to the cached image and title.

Fixed.

> Theoretically we only show the cached image until the real page has loaded
> enough that we can get a decent thumbnail off it. If that's the case, this new
> test is a bad idea, as it would throw away a blurry but complete thumbnail in
> favor of a sharp but incomplete (or even blank) thumbnail.

You're right of course. We cannot update any thumbnail with data that is not yet available. I removed this whole part. So point 2 from comment #0 is invalid.

> If, however, we're not hiding the cached image until long after the real page
> has loaded, or if in this resize case we just never get around to updating the
> thumbnail even after the cached image is hidden, then those themselves are
> problems that should be fixed.

No, that code seems all correct :)
Attachment #505343 - Attachment is obsolete: true
Attachment #506288 - Flags: review?(ian)
Passed try.
Comment on attachment 506288 [details] [diff] [review]
patch v2

Looks good
Attachment #506288 - Flags: review?(ian) → review+
(Assignee)

Updated

8 years ago
Attachment #506288 - Flags: approval2.0?
Comment on attachment 506288 [details] [diff] [review]
patch v2

Does this test thoroughly emulate the tabview codepath for when a session is restored?

If not, you could use the session restore API to load tabs in, and test that way.
(In reply to comment #7)
> Does this test thoroughly emulate the tabview codepath for when a session is
> restored?
> 
> If not, you could use the session restore API to load tabs in, and test that
> way.

This emulates what tabview does when tabs are restored from session. I verified that the test fails when the patch is not applied.
Attachment #506288 - Flags: approval2.0? → approval2.0+
Posted patch patch for checkin (obsolete) — Splinter Review
Pushed to try again.
Attachment #506288 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Attachment #507810 - Attachment is obsolete: true

Comment 11

8 years ago
http://hg.mozilla.org/mozilla-central/rev/b5ee327bf536
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b11
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.