Panorama doesn't render thumbnails for many tabs

VERIFIED FIXED in Firefox 4.0b12

Status

defect
P2
major
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: notforyourmail, Assigned: ttaubert)

Tracking

({regression, ux-consistency})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [ux][hardblocker][has patch])

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110124 Firefox/4.0b10pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110124 Firefox/4.0b10pre

The most recent two builds of Minefield fails to render thumbnails for some or all of the tabs in Panorama.

Reproducible: Always

Steps to Reproduce:
1.  Run Panorama
2.  Some tabs appear blank.  Sometimes, a few render.

Actual Results:  
Some or all tabs are blank in Panorama

Expected Results:  
All tab thumbnails render

Worked until a few days ago.
Blocks: 627096
Priority: -- → P2

Comment 1

8 years ago
Looks like the tabsProgressListener doesn't get called.

Updated

8 years ago
Assignee: nobody → seanedunn

Updated

8 years ago
Blocks: 604699

Comment 2

8 years ago
Posted patch v1 (obsolete) — Splinter Review
A previous patch for bug 597248 had introduced a tab progress listener which would only fire when the page loaded, and which would then let the cached image be hidden. If Panorama was loaded after page load, then this would never happen, and cached images would remain "forever".

I replaced that mechanism by just skipping the _update() if tab.linkedBrowser.contentDocument.readyState != 'complete', guaranteeing that pages are loaded before updating the tab. If it fails, it isn't removed from the update queue.
Attachment #508604 - Flags: review?(ian)
Attachment #508604 - Flags: feedback?(ian)
Comment on attachment 508604 [details] [diff] [review]
v1

Nice! Great to fix a bug and get rid of code at the same time!

I assume just returning if the page is not ready works fine because we get another event when the ready state changes, yes?

I wonder if this patch fixes bug 615954; worth a try. 

>+      // If the tab hasn't loaded completely, skip this update.
>+      if (tab.linkedBrowser.contentDocument.readyState != 'complete')
>+        return;

I think this belongs in update() rather than _update(). That'll keep it off the update queue, and also it means we'll still do a real update if we explicitly call _update, like we do right before zoom (where we want whatever the page has, even if it's not complete).

R+ with that fixed.
Attachment #508604 - Flags: review?(ian)
Attachment #508604 - Flags: review+
Attachment #508604 - Flags: feedback?(ian)
Blocks: 615954

Comment 4

8 years ago
I left it in _update() since a call to update() should guarantee an update in the future. Should update()'s be lossy?
(In reply to comment #4)
> I left it in _update() since a call to update() should guarantee an update in
> the future. Should update()'s be lossy?

Well, as is, the code won't guarantee an update on update(); that'll only work if the update gets put in the queue.

But yes, I think update() can be lossy, because I believe we will get a AttrModified when the ready state changes. Also, I don't think _update() should be lossy, because of the zoom scenario (though we could also accomplish that with a "force" option). 

Of course, this all depends on actually getting that update() call when the ready state changes (due to the AttrModified event). Perhaps you could verify that that is indeed the case? If it's not the case, then perhaps we do need to add a mechanism for making sure the update happens later.

Comment 6

8 years ago
Posted patch v2 (obsolete) — Splinter Review
Attachment #508604 - Attachment is obsolete: true
Attachment #509649 - Flags: review?(ian)

Comment 7

8 years ago
Posted patch v3 (obsolete) — Splinter Review
Apologies, I uploaded the wrong patch. This one's correct.
Attachment #509649 - Attachment is obsolete: true
Attachment #509799 - Flags: review?(ian)
Attachment #509649 - Flags: review?(ian)
Comment on attachment 509799 [details] [diff] [review]
v3

Looks like a good approach. Comments:

>+      // ___ Make sure the tab is complete and ready for updating.
>+      if (!this.isComplete(tab))
>+        return;

If we want _update() and update() to be lossless, you need to add the tab to _tabsWaitingForUpdate here if it's not already in the list (which could happen either through a call to update() that doesn't get deferred, or a call directly to _update(), such as what UI does).

Also, _checkHeartbeat() should prioritize "complete" tabs before those that are still loading; otherwise it'll just keep trying the first tab in the queue over and over again.
Attachment #509799 - Flags: review?(ian) → review-

Comment 10

8 years ago
Posted patch v4 (obsolete) — Splinter Review
I resolved your crits by always taking the tab out of the queue first, and then adding it onto the end if it's incomplete. This way, there's no possibility of eddies in the queue, so to speak.
Attachment #509799 - Attachment is obsolete: true
Attachment #509833 - Flags: review?(ian)
Comment on attachment 509833 [details] [diff] [review]
v4

(In reply to comment #10)
> I resolved your crits by always taking the tab out of the queue first, and then
> adding it onto the end if it's incomplete. This way, there's no possibility of
> eddies in the queue, so to speak.

Nice!

>+      // ___ remove from waiting list now that we have no other 
>+      // early returns

This comment no longer needs "now that we have no other early returns"

r+
Attachment #509833 - Flags: review?(ian) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
There are an awful lot of changes here; can this be tested?
(In reply to comment #12)
> There are an awful lot of changes here; can this be tested?

This is addressing the same issues tabview/browser_tabview_bug597248.js was supposed to test. It had an infamous intermittent orange (documented in bug 615954), so parts of it were eventually disabled. In comment 3 I made a passing reference to re-enabling that test; not sure if Sean has given it a try.

On the other hand, I wouldn't want to pin our hopes on that test; there may be just something structurally wrong with it that has nothing to do with the actual issue. Still, the trouble that test has caused us makes me reticent to hold this patch up until we can come up with a workable test for it.

Sean, any thoughts? Also, could you do a try run with the disabled portions of tabview/browser_tabview_bug597248.js re-enabled on top of this patch?

It should be noted that this bug blocks the blocker bug 604699.
(In reply to comment #13)
> It should be noted that this bug blocks the blocker bug 604699.
True, but that's just a softblocker and still needs work to make it async (we won't be taking any new synchronous disk I/O at this point in the game).

Updated

8 years ago
Blocks: 632280
(In reply to comment #8)
> Sent to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=7aefe32ff2ce

Is this really the latest try? It's all orange. Also looks like, based on comment 14, we shouldn't request approval yet. My bad.

Comment 16

8 years ago
Posted patch v5 (obsolete) — Splinter Review
I improved the tab resolve rate, and fixed all the test failures that came up with these changes (mostly caused by the fact that we save right before window close instead of app quit).

Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d28fbe4e5447
Attachment #509833 - Attachment is obsolete: true
Attachment #511698 - Flags: review?(ian)

Comment 17

8 years ago
Try passed. Ready once you review it, Ian.

Updated

8 years ago
Attachment #511698 - Flags: review?(ian) → feedback?(mitcho)

Updated

8 years ago
No longer blocks: 632280
Comment on attachment 511698 [details] [diff] [review]
v5

>+  // Function: isComplete
>+  // Return whether the xul:tab has fully loaded.
>+  isComplete: function TabItems_update(tab) {
>+      // If our readyState is complete, but we're showing about:blank,
>+      // and we're not loading about:blank, it means we haven't really
>+      // started loading. This can happen to the first few tabs in a
>+      // page.
>+      Utils.assertThrow(tab, "tab");
>+      return (
>+        tab.linkedBrowser.contentDocument.readyState == 'complete' &&
>+        !(tab.linkedBrowser.contentDocument.URL == 'about:blank' &&
>+          tab._tabViewTabItem.url != 'about:blank')
>+      );
>+  },

nit: Your indentation is off here.

>-      // ___ remove from waiting list if needed
>-      let index = this._tabsWaitingForUpdate.indexOf(tab);
>-      if (index != -1)
>-        this._tabsWaitingForUpdate.splice(index, 1);

We need to decide whether we want to try to get this patch landed first, or bug 602432 landed first. I would say bug 602432, as that's in the approval queue now and probably will get prepped before this one. Please repackage on top of bug 602432. (But keep this patch or an updated one around too, in case 602432 gets a-'ed.)

>+   _checkHeartbeat: function TabItems__checkHeartbeat() {

>+       this._update(this._tabsWaitingForUpdate[0]);

nit: indentation is off

>+      // Maintain a simple average of time for each tabitem update
>+      // We can use this as a base by which to delay things like
>+      // tab zooming, so there aren't any hitches. 
>+      let deltaTime = updateEnd - updateBegin;
>+      this._averageUpdateTime = this._averageUpdateTime > 0.0 ?
>+        0.5 * this._averageUpdateTime + 0.5 * deltaTime :
>+        deltaTime;

This is wrong, no? This looks like it'll heavily skew the final averageUpdateTime to later updates... for example, the last update will have a weight of 0.5 in the total... same weight as all the previous updates combined. If we really want to compute this stat, we should probably just keep a updateCount and global accumTime.

Since we're not actually using this for anything right now, though, just removing the averageUpdateTime stuff is another option.

nit: no point having 0.0 in JS. 0 is fine.

>+        
>+      accumTime += deltaTime;
>+     }
>+    

nit: trailing whitespace ^^^

>+     if (this._tabsWaitingForUpdate.length) {
>+       this.startHeartbeat();
>+     }

nit: remove {}

>   _checkHeartbeat: function TabItems__checkHeartbeat() {

Wait wait, you just added a _checkHeartbeat method, but here's another one that wasn't removed. Something's wrong here. :(
Attachment #511698 - Flags: feedback?(mitcho) → feedback-

Comment 19

8 years ago
Posted patch v6 (obsolete) — Splinter Review
Fixed a broken merge and whitespace nits. Updates are much faster now. Go on.. Try it.
Attachment #511698 - Attachment is obsolete: true
Attachment #511803 - Flags: feedback?(mitcho)

Comment 20

8 years ago
Also removed the averageUpdateTime measurement. When we actually need something like that, we can figure out the correct running average then.
Attachment #511803 - Attachment is patch: true
Attachment #511803 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 511803 [details] [diff] [review]
v6

Thanks for catching the spacing issue in pausePainting/resumePainting.
Attachment #511803 - Flags: feedback?(mitcho) → feedback+

Updated

8 years ago
Attachment #511803 - Flags: approval2.0?
Whiteboard: [ux]
Comment on attachment 511803 [details] [diff] [review]
v6

Sean, this no longer applies cleanly on trunk. Please also send most recent (v6+) patch to try so we can list a recent try results link here for approval.
Attachment #511803 - Flags: approval2.0?

Comment 23

8 years ago
Posted patch v7 (obsolete) — Splinter Review
Try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=2c67b86338d1
Attachment #511803 - Attachment is obsolete: true

Updated

8 years ago
Depends on: 602432

Comment 24

8 years ago
Try passed.
OS: Mac OS X → Windows 7

Comment 25

8 years ago
Posted patch v8: unrotted (obsolete) — Splinter Review
Attachment #511982 - Attachment is obsolete: true
Attachment #511991 - Flags: approval2.0?
Comment on attachment 511991 [details] [diff] [review]
v8: unrotted

>   _checkHeartbeat: function TabItems__checkHeartbeat() {
>     this._heartbeat = null;
> 
>-    if (this.isPaintingPaused())
>+    if (this.isPaintingPaused() || !UI.isIdle)
>       return;
> 
>-    if (UI.isIdle())
>+    let accumTime = 0;
>+    // Do as many updates as we can fit into a "perceived" amount
>+    // of time, which is tunable.
>+    while (accumTime < this._maxTimeForUpdating &&
>+      this._tabsWaitingForUpdate.hasItems) {
>+      let updateBegin = Date.now();
>+      //_update will remove the tab from the waiting list
>       this._update(this._tabsWaitingForUpdate.peek());
>+      let updateEnd = Date.now();
>+
>+      // Maintain a simple average of time for each tabitem update
>+      // We can use this as a base by which to delay things like
>+      // tab zooming, so there aren't any hitches.
>+      let deltaTime = updateEnd - updateBegin;
>+      accumTime += deltaTime;
>+    }
> 
>     if (this._tabsWaitingForUpdate.hasItems())
>       this.startHeartbeat();
>   },

I like this addition, but it needs a check to make sure we don't just spin for 200ms on tabs that haven't loaded yet. In other words, we should try each tab only once per heartbeat, even if it gets re-added. Otherwise we'll actually slow our thumbnail painting down significantly simply by keeping the CPU busy half of the time (for 200ms every 200ms) when we should be loading tabs. 

Otherwise the patch looks good.
Attachment #511991 - Flags: approval2.0? → review-
Taking until Sean is back.
Assignee: seanedunn → tim.taubert
Posted patch patch v9 (obsolete) — Splinter Review
(In reply to comment #26)
> I like this addition, but it needs a check to make sure we don't just spin for
> 200ms on tabs that haven't loaded yet. In other words, we should try each tab
> only once per heartbeat, even if it gets re-added. Otherwise we'll actually
> slow our thumbnail painting down significantly simply by keeping the CPU busy
> half of the time (for 200ms every 200ms) when we should be loading tabs.

Fixed.
Attachment #511991 - Attachment is obsolete: true
Attachment #512446 - Flags: review?(ian)
Comment on attachment 512446 [details] [diff] [review]
patch v9

Nice, elegant solution. Seems a bit much to copy the entire queue structure, though. How about instead of copying the entire queue object, you have the queue return an array that contains everything to be updated (in the correct order), and you just pop things off of that?
Attachment #512446 - Flags: review?(ian) → review-
Posted patch patch v10 (obsolete) — Splinter Review
(In reply to comment #30)
> Nice, elegant solution. Seems a bit much to copy the entire queue structure,
> though. How about instead of copying the entire queue object, you have the
> queue return an array that contains everything to be updated (in the correct
> order), and you just pop things off of that?

Fixed.
Attachment #512446 - Attachment is obsolete: true
Attachment #512618 - Flags: review?(ian)
(Assignee)

Updated

8 years ago
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment on attachment 512618 [details] [diff] [review]
patch v10

Beautiful, thanks!
Attachment #512618 - Flags: review?(ian) → review+
(Assignee)

Updated

8 years ago
Attachment #512618 - Flags: approval2.0?
I think I'd rather have a patch of this scope in the beta; blocks a hardblocker and so is a hard blocker.
blocking2.0: --- → betaN+
Whiteboard: [ux] → [ux][hardblocker]

Updated

8 years ago
Whiteboard: [ux][hardblocker] → [ux][hardblocker][has patch]
(Assignee)

Updated

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

>+      function domWinClosedObserver(subject, topic, data) {
>+        if (topic == "domwindowclosed" && subject == gWindow) {
>           if (self.isTabViewVisible())
>             GroupItems.removeHiddenGroups();
>-
>           TabItems.saveAll(true);
>           self._save();
>         }
>       }

Why is this a domwindowclosed observer rather than an unload event listener?
(In reply to comment #35)
> Why is this a domwindowclosed observer rather than an unload event listener?

I don't know much about why domwindowclosed was chosen. I replaced it with an unload listener and all tests are still passing.
(In reply to comment #36)
> (In reply to comment #35)
> > Why is this a domwindowclosed observer rather than an unload event listener?
> 
> I don't know much about why domwindowclosed was chosen. I replaced it with an
> unload listener and all tests are still passing.

Sean would know better, but I think unload was too late in the close sequence; either the information we wanted, or the sessionstore were no longer available at that time.
Attachment #512618 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/e867ecc61368
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
verified with minefield build of 20110217
Status: RESOLVED → VERIFIED

Comment 41

8 years ago
(In reply to comment #37)
> Sean would know better, but I think unload was too late in the close sequence;
> either the information we wanted, or the sessionstore were no longer available
> at that time.

Just getting to reply to this now. Yes, domwindowclosed was used because it fires before the window is closed and saving the sessionstore data requires a valid window.
(Assignee)

Updated

8 years ago
Depends on: 635668
(Assignee)

Updated

8 years ago
No longer depends on: 635668
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.