Closed
Bug 597248
Opened 14 years ago
Closed 14 years ago
Make sure Panorama's thumbnail cache is solid
Categories
(Firefox Graveyard :: Panorama, defect, P2)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: iangilman, Assigned: raymondlee)
References
Details
Attachments
(1 file, 9 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Comment 1•14 years ago
|
||
I'd like to get this resolved before raising bug 595601 to be a blocker.
Reporter | ||
Updated•14 years ago
|
Priority: P3 → P2
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•14 years ago
|
||
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)
Reporter | ||
Comment 3•14 years ago
|
||
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-
Comment 4•14 years ago
|
||
(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)
Assignee | ||
Comment 5•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
What makes steps 3 and 4 necessary? Can't that be simulated in a test?
Reporter | ||
Comment 12•14 years ago
|
||
(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•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
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)
Reporter | ||
Comment 15•14 years ago
|
||
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+
Comment 16•14 years ago
|
||
Can't you just call the appropriate function directly from the test?
Assignee | ||
Comment 17•14 years ago
|
||
Added test
Attachment #482871 -
Attachment is obsolete: true
Attachment #483221 -
Flags: feedback?(ian)
Assignee | ||
Comment 18•14 years ago
|
||
Comment on attachment 483221 [details] [diff] [review]
v1
Passed try
Reporter | ||
Comment 19•14 years ago
|
||
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 20•14 years ago
|
||
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-
Assignee | ||
Comment 21•14 years ago
|
||
(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)
Assignee | ||
Updated•14 years ago
|
Attachment #483221 -
Attachment is obsolete: true
Reporter | ||
Comment 22•14 years ago
|
||
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-
Assignee | ||
Comment 23•14 years ago
|
||
(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)
Comment 24•14 years ago
|
||
(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.
Reporter | ||
Comment 25•14 years ago
|
||
(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.
Reporter | ||
Comment 26•14 years ago
|
||
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-
Assignee | ||
Comment 27•14 years ago
|
||
Updated the patch per Ian's comment
Attachment #490017 -
Attachment is obsolete: true
Attachment #490169 -
Flags: feedback?(ian)
Assignee | ||
Comment 28•14 years ago
|
||
(In reply to comment #27)
> Created attachment 490169 [details] [diff] [review]
> v1
>
> Updated the patch per Ian's comment
Passed try
Reporter | ||
Comment 29•14 years ago
|
||
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+
Assignee | ||
Comment 30•14 years ago
|
||
(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?
Assignee | ||
Updated•14 years ago
|
Attachment #490169 -
Attachment is obsolete: true
Assignee | ||
Comment 31•14 years ago
|
||
(In reply to comment #30)
> Sent it to try 101672ad30e8 and waiting for the result.
Passed try
Assignee | ||
Comment 32•14 years ago
|
||
Could someone give approval2.0 to this please?
Comment 33•14 years ago
|
||
Comment on attachment 490809 [details] [diff] [review]
v1
a=beltzner
Updated•14 years ago
|
Attachment #490809 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 34•14 years ago
|
||
Attachment #490809 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 35•14 years ago
|
||
Reporter | ||
Comment 36•14 years ago
|
||
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
Status: RESOLVED → REOPENED
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
Reporter | ||
Comment 39•14 years ago
|
||
(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.
Assignee | ||
Comment 40•14 years ago
|
||
Updated the test to fix the timing issue mentioned by Ian and passed try.
Attachment #492676 -
Attachment is obsolete: true
Assignee | ||
Comment 41•14 years ago
|
||
Attachment #493997 -
Attachment is obsolete: true
Reporter | ||
Comment 42•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Reporter | ||
Comment 43•14 years ago
|
||
Looks like we've got an intermittent failure: bug 615954
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•