Make sure Panorama's thumbnail cache is solid

RESOLVED FIXED

Status

defect
P2
normal
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: iangilman, Assigned: raymondlee)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

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/poshannessy@mozilla.com-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?
I'd like to get this resolved before raising bug 595601 to be a blocker.
Priority: P3 → P2
Blocks: 597043
Status: NEW → ASSIGNED
Blocks: 602547
Posted patch v1 (obsolete) — Splinter Review
This patch doesn't have a test since I am not sure what sort of tests we can write for it.
Attachment #481876 - Flags: feedback?(ian)
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?
Attachment #481876 - Flags: feedback?(ian) → feedback-
(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)
(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.
(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.
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?
(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.
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!  :-)
(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.
What makes steps 3 and 4 necessary?  Can't that be simulated in a test?
(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.
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.
Posted patch v1 (obsolete) — Splinter Review
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?
Attachment #481876 - Attachment is obsolete: true
Attachment #482871 - Flags: feedback?(ian)
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.
Attachment #482871 - Flags: feedback?(ian) → feedback+
Can't you just call the appropriate function directly from the test?
Posted patch v1 (obsolete) — Splinter Review
Added test
Attachment #482871 - Attachment is obsolete: true
Attachment #483221 - Flags: feedback?(ian)
Comment on attachment 483221 [details] [diff] [review]
v1

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

Looks good
Attachment #483221 - Flags: review?(dolske)
Attachment #483221 - Flags: feedback?(ian)
Attachment #483221 - Flags: feedback+
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. :)
Attachment #483221 - Flags: review?(dolske) → review-
Posted patch v1 (obsolete) — Splinter Review
(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.
Attachment #489859 - Flags: feedback?(ian)
Attachment #483221 - Attachment is obsolete: true
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.
Attachment #489859 - Flags: feedback?(ian) → feedback-
Posted patch v1 (obsolete) — Splinter Review
(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
Attachment #489859 - Attachment is obsolete: true
Attachment #490017 - Flags: feedback?(ian)
(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.
(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 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.
Attachment #490017 - Flags: feedback?(ian) → feedback-
Posted patch v1 (obsolete) — Splinter Review
Updated the patch per Ian's comment
Attachment #490017 - Attachment is obsolete: true
Attachment #490169 - Flags: feedback?(ian)
(In reply to comment #27)
> Created attachment 490169 [details] [diff] [review]
> v1
> 
> Updated the patch per Ian's comment

Passed try
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.
Attachment #490169 - Flags: feedback?(ian) → review+
Posted patch v1 (obsolete) — Splinter Review
(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.
Attachment #490809 - Flags: approval2.0?
Attachment #490169 - Attachment is obsolete: true
(In reply to comment #30)
> Sent it to try 101672ad30e8 and waiting for the result.

Passed try
Could someone give approval2.0 to this please?
Attachment #490809 - Flags: approval2.0? → approval2.0+
Posted patch Patch for check-in (obsolete) — Splinter Review
Attachment #490809 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5cdcba8db3a2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
Blocks: 604699
Sent it to try and passed.
Keywords: checkin-needed
Blocks: 609685
(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.
Posted patch Patch for check-in (obsolete) — Splinter Review
Updated the test to fix the timing issue mentioned by Ian and passed try.
Attachment #492676 - Attachment is obsolete: true
Attachment #493997 - Attachment is obsolete: true
No longer blocks: 609685
http://hg.mozilla.org/mozilla-central/rev/3e199c023eaf
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Looks like we've got an intermittent failure: bug 615954
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.