597248 - Make sure Panorama's thumbnail cache is solid

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Ian Gilman [:iangilman]]

One issue I believe we have is that it'll load the cached thumbnails right away, but then pull them off to show the live thumbnail too soon (while the page is still loading). 

Also, zpao encountered this:

http://grab.by/6qsr

... though it may have to do with bug 586068 (hopefully landing soon). Here's a recent try build from his work on that bug:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-1caf0030ec16/

At any rate, we need to verify that the thumbnail cache is working properly, especially if we're going to get bug 595601 (which we would very much like to). 

I don't know if a unit test is appropriate here; doesn't seem like the sort of thing you can write a mochitest for. Perhaps there's some other sort of test we can use?

Comment 1

[Aza Raskin [:aza]]

I'd like to get this resolved before raising bug 595601 to be a blocker.

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

This patch doesn't have a test since I am not sure what sort of tests we can write for it.

Comment 3

[Ian Gilman [:iangilman]]

Comment on attachment 481876 [details] [diff] [review]
v1

Yeah, I'm not really sure how to test this either. Ehsan, Paul, any ideas?

>   saveAll: function TabItems_saveAll(saveImageData) {
>-    var items = this.getItems();
>+    let self = this;
>+    let items = this.getItems();
>     items.forEach(function(item) {
>+      // if it requires to save image data and the cached image is being displayed,
>+      // update the canvas before saving item data to the tab.
>+      if (saveImageData && item.isShowingCachedData)
>+        self._update(item.tab);
>       item.save(saveImageData);
>     });
>   },

We shouldn't force a repaint; instead recache the same cached image. This will be especially important once we have bug 595601, since many tabs will never load during a session, but even before then we don't want to introduce a slow down on quit.

>         if (tabData.imageData) {
>           item.showCachedData(tabData);
>           // the code in the progress listener doesn't fire sometimes because
>-          // tab is being restored so need to catch that.
>+          // tab is being restored from caches so we need to handle that.
>+          let self = this;
>           setTimeout(function() {
>             if (item && item.isShowingCachedData) {
>-              item.hideCachedData();
>+              if (UI.isTabViewVisible())
>+                self._update(item.tab);
>+              else
>+                item.hideCachedData();
>             }
>           }, 15000);

We need to get rid of this as well before we can have bug 595601; sometimes cached thumbnails will hang around forever as the pages never load (by design). 

Is there any other way we can deal with the "progress listener not firing" issue?

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> Yeah, I'm not really sure how to test this either. Ehsan, Paul, any ideas?

Not really. The best thing might just be to make a litmus test for it. A MozMill test might work, but I'm not quite sure what's actually happening (haven't looked at this code at all)

Comment 5

[Raymond Lee [:raymondlee]]

(In reply to comment #3)
> Comment on attachment 481876 [details] [diff] [review]
> Is there any other way we can deal with the "progress listener not firing"
> issue?

That load (DOM) event isn't dispatched in some cases for restored tabs. https://developer.mozilla.org/En/Using_Firefox_1.5_caching

However, I think we might be able to add a web progress listener to each restored tabs and to detect when the cached page is restored.
https://developer.mozilla.org/En/Listening_to_events#Web_progress_listeners

Will give this a try.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #5)
> However, I think we might be able to add a web progress listener to each
> restored tabs and to detect when the cached page is restored.
> https://developer.mozilla.org/En/Listening_to_events#Web_progress_listeners
> 
> Will give this a try.

You can do that. I might suggest actually just using gBrowser.add/removeTabsProgressListener, which only adds a single listener that gBrowser will call into from the progress listeners it has. That way you only add 1 listener instead of N.

It's what I did in sessionstore for cascaded session restore.

Comment 7

[(no longer active)]

It's probably possible to test this in a browser-chrome test.  Can you just give me an example of a set of STRs which were broken before this patch, and will be fixed by it?

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #7)
> It's probably possible to test this in a browser-chrome test.  Can you just
> give me an example of a set of STRs which were broken before this patch, and
> will be fixed by it?

My original STR were to restart firefox with browser.sessionstore.max_concurrent_tabs = 0 and then enter tabview. This is why I said what I did in comment #4.

Could perhaps reproduce by having a window open with tabs, ensure there are thumbnails, close & reopen the window & check for thumbnails. I'm not sure that actually repros the issue though.

Comment 9

[(no longer active)]

Does that pref take effect only at startup?  Or are there listeners set up to catch its changes?

Still, I'd like to know what the STR actually looks like!  :-)

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #9)
> Does that pref take effect only at startup?  Or are there listeners set up to
> catch its changes?

There are listeners.

> Still, I'd like to know what the STR actually looks like!  :-)

0. Set that pref
1. Open tabs to awesome websites
2. Open tab view, see awesome thumbnails
3. Quit Firefox
4. Start Firefox
5. Enter tabview, don't see awesome thumbnails

Step 0 isn't strictly necessary. You can see this if you enter tabview at startup with default prefs and see that thumbnails aren't generated until each tab has loaded.

Comment 11

[(no longer active)]

What makes steps 3 and 4 necessary?  Can't that be simulated in a test?

Comment 12

[Ian Gilman [:iangilman]]

(In reply to comment #11)
> What makes steps 3 and 4 necessary?  Can't that be simulated in a test?

We want a bunch of tabs that we had previously loaded to now be loading anew all at the same time. While they're loading (or before they even start loading), they should be showing cached thumbnails. Once they're done loading, they should be showing "live" thumbnails. Restarting the browser is one way to make that happen, but I imagine you could also do that with sessionstore in much the same way that private browsing does it. 

Speaking of which, a good additional test would be to make sure that the thumbnail cache does the right thing when entering/exiting private browsing. 

Note that our cached thumbnails are <img> elements and our "live" thumbnails are <canvas> elements, for what it's worth.

Comment 13

[(no longer active)]

You should be able to load all of those tabs in a window, close the window, undo the window closing, and examine the cache once right after the new window is opened, and once after all of its tabs are finished loading.  This should be possible to do in a browser-chrome test pretty easily.

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v1

Used a tabsProgressListener in this patch.  

I still find it hard to write the test because when a tab is loaded, a flag is set to hide the cached image.  And it would depend on the update canvas algorithm to invoke TabItems__update(tab) to paint the canvas and then hide the cached image. In other words, the cached image isn't set to hidden immediately after a tab is loaded.  Ian: any suggestions how to write the test?

Comment 15

[Ian Gilman [:iangilman]]

Comment on attachment 482871 [details] [diff] [review]
v1

Looking good. Comments:

>+    gBrowser.addTabsProgressListener(self._tabsProgressListener);

This line doesn't need "self"; "this" will do. 

>   uninit: function TabItems_uninit() {
>+    let self = this;
>+
>+    if (self._tabsProgressListener)
>+      gBrowser.removeTabsProgressListener(self._tabsProgressListener);

Likewise, don't need "self". 

>-      if (tabItem.isShowingCachedData && !tab.hasAttribute("busy"))
>+      // when a tab is opened, the url would be "about:blank" before the actual
>+      // url gets restored.  In this case, the cached thumb image should not be 
>+      // hidden.
>+      if (tabItem.isShowingCachedData() && 
>+           (tabItem.shouldHideCachedData || (!tab.hasAttribute("busy") && 
>+             tab.linkedBrowser.currentURI.spec != "about:blank")))
>         tabItem.hideCachedData();

Is shouldHideCachedData reliable enough that we can get rid of the rest of the check? Would be nice to keep it simple if possible.

Comment 16

[(no longer active)]

Can't you just call the appropriate function directly from the test?

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: v1

Added test

Comment 18

[Raymond Lee [:raymondlee]]

Comment on attachment 483221 [details] [diff] [review]
v1

Passed try

Comment 19

[Ian Gilman [:iangilman]]

Comment on attachment 483221 [details] [diff] [review]
v1

Looks good

Comment 20

[Justin Dolske [:Dolske]]

Comment on attachment 483221 [details] [diff] [review]
v1


>+    this._tabsProgressListener = {
>+      onLocationChange: function(browser, webProgress, request, locationURI) {
>+      },

No need for arguments in stubbed functions. But better yet, in this case you don't even need the stubs... tabbrowser.xml's _callProgressListeners() won't bother trying to call functions that don't exist in your listener.


>+      onStateChange: function(browser, webProgress, request, stateFlags, status) {
>+        if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
>+            stateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK) {

Add safety parens around the bitwise ops: (a & b) && (c & d)

Pretty sure you actually want STATE_IS_DOCUMENT here, otherwise you can get triggered before the tab has actually loaded. EG, if it has frames.

>+          self.items.forEach(function(tabItem) {

It makes me sad to have O(n^2) update. :( Granted, the code goes away after the browser has restored tabs (which is when we're using cached data, right?), but that's also a busy time for the browser.

Worth looking at having tabitems cached on the browser element, or have a private lookup hash (by the tab's URL or something)? ISTR this kind of loop happens elsewhere too.

>+            if (tabItem.tab.linkedBrowser == browser)
>+              tabItem.shouldHideCachedData = true;
>+
>+            if (tabItem.isShowingCachedData())
>+              cachedDataCounter++;

A modest win to the existing code would be to |break| after matching the browser. You'd basically need to initialize cachedDataCounter once beforehand (at startup), and just decrement it when you match the browser. Half the work, on average. [Actually, maybe more if session restore is loading tabs in the same order as .items]. Of course, eliminating the loop entirely would be better still. :)

Comment 21

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to comment #20)
> Comment on attachment 483221 [details] [diff] [review]
> v1
> 
> 
> >+    this._tabsProgressListener = {
> >+      onLocationChange: function(browser, webProgress, request, locationURI) {
> >+      },
> 
> No need for arguments in stubbed functions. But better yet, in this case you
> don't even need the stubs... tabbrowser.xml's _callProgressListeners() won't
> bother trying to call functions that don't exist in your listener.
> 

Removed the stubs 

> 
> >+      onStateChange: function(browser, webProgress, request, stateFlags, status) {
> >+        if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
> >+            stateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK) {
> 
> Add safety parens around the bitwise ops: (a & b) && (c & d)

Added

> 
> Pretty sure you actually want STATE_IS_DOCUMENT here, otherwise you can get
> triggered before the tab has actually loaded. EG, if it has frames.
> 

Used STATE_IS_WINDOW instead.

> >+          self.items.forEach(function(tabItem) {
> 
> It makes me sad to have O(n^2) update. :( Granted, the code goes away after the
> browser has restored tabs (which is when we're using cached data, right?), but
> that's also a busy time for the browser.
> 
> Worth looking at having tabitems cached on the browser element, or have a
> private lookup hash (by the tab's URL or something)? ISTR this kind of loop
> happens elsewhere too.
> 
> >+            if (tabItem.tab.linkedBrowser == browser)
> >+              tabItem.shouldHideCachedData = true;
> >+
> >+            if (tabItem.isShowingCachedData())
> >+              cachedDataCounter++;
> 
> A modest win to the existing code would be to |break| after matching the
> browser. You'd basically need to initialize cachedDataCounter once beforehand
> (at startup), and just decrement it when you match the browser. Half the work,
> on average. [Actually, maybe more if session restore is loading tabs in the
> same order as .items]. Of course, eliminating the loop entirely would be better
> still. :)

Fixed.

Comment 22

[Ian Gilman [:iangilman]]

Comment on attachment 489859 [details] [diff] [review]
v1

* It seems to me that the _tabsProgressListener should be added in showCachedData if the cachedDataCounter goes from 0 to 1, and it should be removed in hideCachedData when the cachedDataCounter goes from 1 to 0; this keeps it clean.

* Since linkedBrowser doesn't belong to us, and since this is for a specific usage, we should use a more obscure name than linkedBrowser.tabItem; one that describes what it is, where it comes from, and what it's for. For instance, linkedBrowser._tabViewTabItemWithCachedData.

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to comment #22)
> Comment on attachment 489859 [details] [diff] [review]
> v1
> 
> * It seems to me that the _tabsProgressListener should be added in
> showCachedData if the cachedDataCounter goes from 0 to 1, and it should be
> removed in hideCachedData when the cachedDataCounter goes from 1 to 0; this
> keeps it clean.

IMO, both showCachedData() and hideCachedData() shouldn't be called in the _tabsProgressListener.onStateChange() because the cached images should already be displayed before _tabsProgressListener.onStateChange is called for the first time and we also have the update algorithm code to invoke hideCachedData() in TabItems__update() if necessary.  

The code in the _tabsProgressListener.onStateChange() should only set a shouldHideCachedData flag and let the update code to hideCachedData when the time is right.

> 
> * Since linkedBrowser doesn't belong to us, and since this is for a specific
> usage, we should use a more obscure name than linkedBrowser.tabItem; one that
> describes what it is, where it comes from, and what it's for. For instance,
> linkedBrowser._tabViewTabItemWithCachedData.

Changed the name to _tabViewTabItemWithCachedData

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #22)
> * Since linkedBrowser doesn't belong to us, and since this is for a specific
> usage, we should use a more obscure name than linkedBrowser.tabItem; one that
> describes what it is, where it comes from, and what it's for. For instance,
> linkedBrowser._tabViewTabItemWithCachedData.

The same applies to tab.tabItem, by the way.

Comment 25

[Ian Gilman [:iangilman]]

(In reply to comment #24)
> (In reply to comment #22)
> > * Since linkedBrowser doesn't belong to us, and since this is for a specific
> > usage, we should use a more obscure name than linkedBrowser.tabItem; one that
> > describes what it is, where it comes from, and what it's for. For instance,
> > linkedBrowser._tabViewTabItemWithCachedData.
> 
> The same applies to tab.tabItem, by the way.

Agreed... I think you mentioned this before, and it slipped my mind. I've now filed bug 611715. Thanks for the reminder.

Comment 26

[Ian Gilman [:iangilman]]

Comment on attachment 490017 [details] [diff] [review]
v1

(In reply to comment #23)
> > * It seems to me that the _tabsProgressListener should be added in
> > showCachedData if the cachedDataCounter goes from 0 to 1, and it should be
> > removed in hideCachedData when the cachedDataCounter goes from 1 to 0; this
> > keeps it clean.
> 
> IMO, both showCachedData() and hideCachedData() shouldn't be called in the
> _tabsProgressListener.onStateChange() because the cached images should already
> be displayed before _tabsProgressListener.onStateChange is called for the first
> time and we also have the update algorithm code to invoke hideCachedData() in
> TabItems__update() if necessary.  
> 
> The code in the _tabsProgressListener.onStateChange() should only set a
> shouldHideCachedData flag and let the update code to hideCachedData when the
> time is right.

Yes, I agree with you. I'm asking for something different from that. I'm suggesting that everything should stay the same except that this line: 

  gBrowser.addTabsProgressListener(this._tabsProgressListener);
  
... should be in showCachedData (when the counter goes from 0 to 1), and this line:

  gBrowser.removeTabsProgressListener(this._tabsProgressListener);
  
... should be in hideCachedData (when the counter goes from 1 to 0). Does that make sense? Sorry I wasn't clear before.

Otherwise you're removing the listener based on a counter but you're not checking it when the counter changes.

Comment 27

[Raymond Lee [:raymondlee]]

Attachment: v1

Updated the patch per Ian's comment

Comment 28

[Raymond Lee [:raymondlee]]

(In reply to comment #27)
> Created attachment 490169 [details] [diff] [review]
> v1
> 
> Updated the patch per Ian's comment

Passed try

Comment 29

[Ian Gilman [:iangilman]]

Comment on attachment 490169 [details] [diff] [review]
v1

Looks good, except:

>+      if (TabItems.cachedDataCounter == 0) {
>+        gBrowser.removeTabsProgressListener(TabItems.tabsProgressListener);
>+        TabItems.tabsProgressListener = null;

Don't null tabsProgressListener here.  I suppose theoretically we'll never increment cachedDataCounter once it has returned to zero, but you never know.  

r+ with that fix.

Comment 30

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to comment #29)
> Comment on attachment 490169 [details] [diff] [review]
> v1
> 
> Looks good, except:
> 
> >+      if (TabItems.cachedDataCounter == 0) {
> >+        gBrowser.removeTabsProgressListener(TabItems.tabsProgressListener);
> >+        TabItems.tabsProgressListener = null;
> 
> Don't null tabsProgressListener here.  I suppose theoretically we'll never
> increment cachedDataCounter once it has returned to zero, but you never know.  
> 
> r+ with that fix.

Removed TabItems.tabsProgressListener = null;

Sent it to try 101672ad30e8 and waiting for the result.

Comment 31

[Raymond Lee [:raymondlee]]

(In reply to comment #30)
> Sent it to try 101672ad30e8 and waiting for the result.

Passed try

Comment 32

[Raymond Lee [:raymondlee]]

Could someone give approval2.0 to this please?

Comment 33

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

Comment on attachment 490809 [details] [diff] [review]
v1

a=beltzner

Comment 34

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 35

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/5cdcba8db3a2

Comment 36

[Ian Gilman [:iangilman]]

This patch is causing a lot of oranges: 

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290555881.1290559857.21068.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290558486.1290559719.20375.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290558345.1290559665.20193.gz 

... so I've backed it out: 

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

Comment 37

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

This bug was part of a mass backout to fix the permanent leak on OS X 64 that this push caused.

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

Comment 38

[Raymond Lee [:raymondlee]]

Sent it to try and passed.

Comment 39

[Ian Gilman [:iangilman]]

(In reply to comment #38)
> Sent it to try and passed.

I'm still concerned about the test failures mentioned in comment 36; they seem unrelated to the leak mentioned in comment 37. Can you please look into this, Raymond? It didn't happen every time, but there were three over the course of maybe 5 build runs. Perhaps there's a timing thing that needs to be tightened up.

Comment 40

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Updated the test to fix the timing issue mentioned by Ian and passed try.

Comment 41

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 42

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/3e199c023eaf

Comment 43

[Ian Gilman [:iangilman]]

Looks like we've got an intermittent failure: bug 615954