627288 - Tabs restored from last session show stale cache data

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1, WIP

This patch fixes all three issues. If that's the right approach I'll write a test :)

Comment 2

[Ian Gilman [:iangilman]]

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.

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Bug 627239 is the important one in this domain.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(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 :)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Passed try.

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 506288 [details] [diff] [review]
patch v2

Looks good

Comment 7

[Dietrich Ayala (:dietrich)]

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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Pushed to try again.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b5ee327bf536