628701 - Panorama doesn't render thumbnails for many tabs

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Michael]

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.

Comment 1

[Sean Dunn]

Looks like the tabsProgressListener doesn't get called.

Comment 2

[Sean Dunn]

Attachment: v1

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.

Comment 3

[Ian Gilman [:iangilman]]

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.

Comment 4

[Sean Dunn]

I left it in _update() since a call to update() should guarantee an update in the future. Should update()'s be lossy?

Comment 5

[Ian Gilman [:iangilman]]

(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

[Sean Dunn]

Attachment: v2

Comment 7

[Sean Dunn]

Attachment: v3

Apologies, I uploaded the wrong patch. This one's correct.

Comment 8

[Sean Dunn]

Sent to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=7aefe32ff2ce

Comment 9

[Ian Gilman [:iangilman]]

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.

Comment 10

[Sean Dunn]

Attachment: v4

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.

Comment 11

[Ian Gilman [:iangilman]]

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+

Comment 12

[Shawn Wilsher :sdwilsh]

There are an awful lot of changes here; can this be tested?

Comment 13

[Ian Gilman [:iangilman]]

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

Comment 14

[Shawn Wilsher :sdwilsh]

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

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

(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

[Sean Dunn]

Attachment: v5

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

Comment 17

[Sean Dunn]

Try passed. Ready once you review it, Ian.

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

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

Comment 19

[Sean Dunn]

Attachment: v6

Fixed a broken merge and whitespace nits. Updates are much faster now. Go on.. Try it.

Comment 20

[Sean Dunn]

Also removed the averageUpdateTime measurement. When we actually need something like that, we can figure out the correct running average then.

Comment 21

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 511803 [details] [diff] [review]
v6

Thanks for catching the spacing issue in pausePainting/resumePainting.

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

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.

Comment 23

[Sean Dunn]

Attachment: v7

Try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=2c67b86338d1

Comment 24

[Sean Dunn]

Try passed.

Comment 25

[Sean Dunn]

Attachment: v8: unrotted

Comment 26

[Ian Gilman [:iangilman]]

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.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Taking until Sean is back.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v9

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

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 512446 [details] [diff] [review]
patch v9

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=0bdb2f5d00ac

Comment 30

[Ian Gilman [:iangilman]]

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?

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v10

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

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 512618 [details] [diff] [review]
patch v10

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=a85b47b48390

Comment 33

[Ian Gilman [:iangilman]]

Comment on attachment 512618 [details] [diff] [review]
patch v10

Beautiful, thanks!

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

I think I'd rather have a patch of this scope in the beta; blocks a hardblocker and so is a hard blocker.

Comment 35

[Dão Gottwald [:dao]]

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?

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 37

[Ian Gilman [:iangilman]]

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

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 40

[Tracy Walker [:tracy]]

verified with minefield build of 20110217

Comment 41

[Sean Dunn]

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