Closed
Bug 604699
Opened 14 years ago
Closed 14 years ago
Store thumbnails in the browser's image cache rather than in sessionstore
Categories
(Firefox Graveyard :: Panorama, defect, P1)
Firefox Graveyard
Panorama
Tracking
(blocking2.0 final+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: iangilman, Assigned: ttaubert)
References
Details
(Keywords: memory-footprint, perf, Whiteboard: [hardblocker][has patch])
Attachments
(1 file, 8 obsolete files)
24.53 KB,
patch
|
Details | Diff | Splinter Review |
The fact we have our thumbnail cache in sessionstore contributes a good deal to the size of that file... it would be more appropriate and efficient to store them in the browser's image cache.
The patches in bug 497543 may be of help; even if we don't use them literally, they may point to how to do it.
Comment 1•14 years ago
|
||
Sounds perf-y. Sean, how do feel about tackling this?
Assignee: nobody → seanedunn
Blocks: 604213
Updated•14 years ago
|
Target Milestone: --- → Firefox 4.0b8
Current status on this:
Thumbnails have been a bit broken, but I'm not sure for how long. First order of business is to fix their behavior. I've changed the bug title to reflect this.
1. They were only being saved via TabItems.saveAll(true), and this was only happening on app quit, and only if you were in TabView, which never happens anymore because exiting the app forces you out of TabView before you can actually quit. Fixed that.
2. They would often get saved before the TabCanvas was painted to. Fixed that by using the last saved image string until the first paint() is called on the TabCanvas. Note that I'll change this over to using the image cache when I get to that part.
3. Because _update() needs to function properly, I've set a dependency on 609685.
4. I'm currently trying to figure out why sometimes, even after painting the correctly loaded webpage bits to the tabitem's canvas, canvas.toDataURL() returns a big white PNG. A possibly related problem is that even when the thumbnails load from storage and display, they're quickly replaced by a white thumbnail during the "Connecting" phase of the page load. I'd like to detect this state and only update when it's a real page.
Summary: Store our thumbnails in the browser's image cache rather than in sessionstore → Fix thumbnail behavior, store them in the browser's image cache rather than in sessionstore
Comment 4•14 years ago
|
||
Please have a look at the patch for bug 597248. There are changes to the thumbnail code.
Thanks, Raymond. I hadn't seen that bug -- I'll make this a dependency upon that.
Summary: Fix thumbnail behavior, store them in the browser's image cache rather than in sessionstore → Store thumbnails in the browser's image cache rather than in sessionstore
Reporter | ||
Comment 6•14 years ago
|
||
See also bug 615704 on a related topic.
Comment 7•14 years ago
|
||
Why are we saving thumbnails in sessionstore to begin with? That doesn't sound like a useful thing to do at all, so I'm quite surprised to hear that we're doing it...
Reporter | ||
Comment 8•14 years ago
|
||
(In reply to comment #7)
> Why are we saving thumbnails in sessionstore to begin with? That doesn't sound
> like a useful thing to do at all, so I'm quite surprised to hear that we're
> doing it...
When we realized Panorama needed to cache thumbnails between sessions (which will become even more important once bug 595601 lands), sessionstore was the path of least resistance, since that's where all of the rest of the Panorama data is stored.
Reporter | ||
Comment 11•14 years ago
|
||
Kyle Huey suggests in bug 615704 that we might use canvas.mozGetAsFile to turn the contents of a canvas into a File and then persist that. I don't know if that's better or worse than looking into the existing image cache, but I thought I'd mention it here.
Reporter | ||
Comment 12•14 years ago
|
||
Note that if we do this we'll need to take responsibility for security concerns associated with stored thumbnails. Say you go to your bank site and it's got your account number and balance, and we make a high-res thumbnail of it and store it to disk. At the very least, we need to make sure we remove the thumbnail as soon as the tab is closed, for instance.
Also see bug 627239 and bug 627237.
Comment 13•14 years ago
|
||
Based on comment 12, tempted to punt this to the future.
Comment 14•14 years ago
|
||
Addressing those is not too much work I think.
The session restore file is read at startup, so fixing this bug will likely improve startup time as well. Though it wouldn't show up in Talos, since we don't measure profiles like this.
Updated•14 years ago
|
blocking2.0: --- → ?
Reporter | ||
Comment 17•14 years ago
|
||
Too late for this for Fx4
No longer blocks: 627096
Target Milestone: Firefox 4.0b8 → Future
Comment 18•14 years ago
|
||
what? you can't store these images in sessionstore. that sounds ...insane.
Comment 19•14 years ago
|
||
Blocking on this for investigation.
We must know whether profiles that actually have data are adversely affected by this or not.
blocking2.0: ? → final+
Whiteboard: [softblocker]
Updated•14 years ago
|
Target Milestone: Future → ---
Comment 20•14 years ago
|
||
why do we cache them at all?
Comment 21•14 years ago
|
||
I'd also like to mention, if this is not clear enough here, that this can potentially undo all of the hard work that has been put into making the user perceivable startup performance improvements made in Firefox 4. We do not need to, and should not, cache these images.
Reporter | ||
Comment 22•14 years ago
|
||
(In reply to comment #20)
> why do we cache them at all?
On a slow machine and/or connection and a number of tabs, it can take many minutes for all of the tabs to load up enough to render thumbnails for them. Once we land bug 595601, tabs in other groups won't even load until you go to them. In both cases, having cached thumbnails allows you to see your pages immediately in Panorama.
So yes, there are a lot of good reasons to not store them in sessionstore, and there are also a lot of good reasons to store them somewhere (i.e. the image cache).
Comment 23•14 years ago
|
||
Are these thumbnails used when restoring a session?
Reporter | ||
Comment 24•14 years ago
|
||
(In reply to comment #23)
> Are these thumbnails used when restoring a session?
They are not... they're actually pretty low-res.
Severity: blocker → normal
Priority: P1 → --
Reporter | ||
Comment 25•14 years ago
|
||
So we're getting started on this, but no one on the Panorama team knows anything about hooking into the browser's existing image cache. Does anyone on this bug have a recommendation for who to talk to?
Comment 26•14 years ago
|
||
the patch on bug 497543 uses the disk cache service.
there might be a better way though, so it'd be worth asking in #developers, or digging through mxr to see how we do it for web images.
Comment 27•14 years ago
|
||
Can you first stop using session restore for this and then explore using the image cache as a second step?
Comment 28•14 years ago
|
||
Attachment #509797 -
Flags: review?(ian)
Attachment #509797 -
Flags: feedback?(ian)
Comment 29•14 years ago
|
||
Reporter | ||
Comment 30•14 years ago
|
||
Comment on attachment 509797 [details] [diff] [review]
v1
Looks good to me, but I'm not familiar with the cache routines. Could you get feedback from someone familiar with that domain?
My comments:
* Clearly this is a patch in progress, but once it's time to land, please clean out the dump and log commands.
* Doesn't look like you need to pass a tab into saveTabThumb and loadTabThumb, so please omit that argument
* Any idea what the perf implications are for this change?
Attachment #509797 -
Flags: review?(ian)
Attachment #509797 -
Flags: feedback?(ian)
Attachment #509797 -
Flags: feedback+
Comment 32•14 years ago
|
||
Updated again. To answer your question about perf, loading from the cache feels faster (as it should be), but I haven't measured this.
Attachment #509828 -
Attachment is obsolete: true
Reporter | ||
Comment 33•14 years ago
|
||
(In reply to comment #32)
> Created attachment 509829 [details] [diff] [review]
> v3
>
> Updated again. To answer your question about perf, loading from the cache feels
> faster (as it should be), but I haven't measured this.
Cool.
Do you know who to ask for feedback on the patch? Joe? Someone else who gave you info about the cache system?
Comment 34•14 years ago
|
||
Comment on attachment 509829 [details] [diff] [review]
v3
Shawn, would you mind feedbacking here? :)
Attachment #509829 -
Flags: feedback?(sdwilsh)
Comment 35•14 years ago
|
||
Bjarne and Michal, can you guys have a look at the patch here and share thoughts on this approach?
Comment 36•14 years ago
|
||
Comment on attachment 509829 [details] [diff] [review]
v3
>+ this._inputStreamFactory = Cc["@mozilla.org/scriptableinputstream;1"];
In general, you want to use Components.constructor for this, not what you are doing (however, you don't need this).
>+ saveThumbnail: function Storage_saveThumbnail(url, imageData) {
>+ let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url,
>+ Ci.nsICache.ACCESS_WRITE, true);
>+ let outputStream = entry.openOutputStream(0);
>+ outputStream.write(imageData, imageData.length);
>+ outputStream.close();
>+ entry.close();
Noooooo! This is synchronous I/O on the GUI thread (my most hated enemy)! What you really want to do here is use NetUtil.asyncCopy. Your sink will be the output stream, and your source will have to be an input string that you create.
>+ loadThumbnail: function Storage_loadThumbnail(url) {
>+ Utils.log("loading tab thumb");
>+ let imageChunks = [];
>+ let entry = null;
>+ try {
>+ entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url,
>+ Ci.nsICache.ACCESS_READ, true);
>+ } catch (e) {
>+ Utils.log(e);
>+ }
>+ if(entry) {
>+ let nativeInputStream = entry.openInputStream(0);
>+ let inputStream = this._inputStreamFactory.
>+ createInstance(Ci.nsIScriptableInputStream);
>+ inputStream.init(nativeInputStream);
>+ try {
>+ while (true) {
>+ // read a megabyte at a time if needed
>+ let chunk = inputStream.read(1024*1024);
>+ if (chunk.length == 0)
>+ break;
>+ imageChunks.push(chunk);
>+ }
>+ } catch (e) {
>+ Utils.log(e);
>+ }
>+ entry.close();
>+ }
>+ let imageData = imageChunks.join("");
>+ return imageData;
>+ },
There's no good way to do this async, but I'm going to go file the bug to add a method to NetUtil to make your life easier. We should be able to take that for 2.0 too!
You'll want to get feedback from zpao too on the session store bits.
Attachment #509829 -
Flags: feedback?(sdwilsh) → feedback-
Comment 37•14 years ago
|
||
There is some ongoing work in initializing the cache-service which may be relevant here. See bug #630420, comment #17 in particular. Relevance depends on what time in the startup-sequence this stuff is supposed to happen.
Comment 38•14 years ago
|
||
(In reply to comment #36)
> There's no good way to do this async, but I'm going to go file the bug to add a
> method to NetUtil to make your life easier. We should be able to take that for
> 2.0 too!
Filed bug 632227, and it has a patch up you can use too. The callers to loadThumbnail are going to need to be changed to handle the fact that it'll be async, but getting the data is just going to be:
NetUtil.asyncFetch(entry.openInputStream(0), callbackfunction);
Comment 39•14 years ago
|
||
(In reply to comment #38)
> Filed bug 632227, and it has a patch up you can use too. The callers to
> loadThumbnail are going to need to be changed to handle the fact that it'll be
> async, but getting the data is just going to be:
> NetUtil.asyncFetch(entry.openInputStream(0), callbackfunction);
I've chosen to use asyncOpenCacheEntry() on the session instead, since the above would have just opened it synchronously and done the copy async (which isn't as big of a win, since this isn't a huge amount of data).
Also adding mitcho for feedback, since the load behavior is now async, and I want a second set of eyes to make sure I've covered my bases.
Attachment #509829 -
Attachment is obsolete: true
Attachment #510770 -
Flags: review?(sdwilsh)
Attachment #510770 -
Flags: feedback?(ian)
Comment 40•14 years ago
|
||
Er, I mean ian. Added Ian for feedback :)
Comment 41•14 years ago
|
||
Comment on attachment 510770 [details] [diff] [review]
v4
>+++ b/browser/base/content/tabview/storage.js
> init: function Storage_init() {
> this._sessionStore =
> Cc["@mozilla.org/browser/sessionstore;1"].
> getService(Ci.nsISessionStore);
>+
>+ // Create stream-based cache session for tabview
>+ let cacheService =
>+ Cc["@mozilla.org/network/cache-service;1"].
>+ getService(Ci.nsICacheService);
>+ this._cacheSession = cacheService.createSession(
>+ this.CACHE_CLIENT_IDENTIFIER, Ci.nsICache.STORE_ON_DISK, true);
> },
>
> // ----------
> // Function: uninit
> uninit: function Storage_uninit () {
> this._sessionStore = null;
I strongly suspect you want to null out the new things you do in init here as well
>+ saveThumbnail: function Storage_saveThumbnail(url, imageData) {
>+ let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url,
>+ Ci.nsICache.ACCESS_WRITE, true);
I had completely forgotten about asyncOpenCacheEntry (and partially how the disk cache works all together). After a brief refresher on the code, I see that this also does synchronous disk I/O. Fun times!
I think what you'll want to do is make a generic cache listener that will give you a callback when the entry is open. At that point, you'll want to do the work in each method, so something like this:
let listener = new CacheListener(function(result, entry) {
if (Components.isSuccessCode(result)) {
// current saveThumbnail code goes here
}
});
this._cacheSession.asyncOpenCacheEntry(...)
>+ let StringInputStream = Components.Constructor(
>+ "@mozilla.org/io/string-input-stream;1", "nsIStringInputStream");
note: using Components.constructor is really only useful if you move it up (either as a property on the Storage object or globally in the file). Otherwise, it's less expensive to just do Cc["..."].createInstance(...)
>+ loadThumbnail: function Storage_loadThumbnail(url, callback) {
>+ let entry = null;
>+ try {
>+ function CacheListener() {
>+ }
>+ CacheListener.prototype = {
>+ QueryInterface : function(iid) {
>+ if (iid.equals(Ci.nsICacheListener))
>+ return this;
>+ throw Components.results.NS_NOINTERFACE;
>+ },
never implement QI yourself, this is what XPCOMUtils.generateQI is for!
>+ onCacheEntryAvailable: function(entry, access, status) {
>+ let imageChunks = [];
>+ if(entry) {
>+ let nativeInputStream = entry.openInputStream(0);
>+ let ScriptableInputStream = Components.Constructor(
>+ "@mozilla.org/scriptableinputstream;1",
>+ "nsIScriptableInputStream", "init");
>+ let inputStream = new ScriptableInputStream(nativeInputStream);
>+ try {
>+ while (true) {
>+ // read up to a megabyte at a time
>+ let chunk = inputStream.read(1024*1024);
>+ if (chunk.length == 0)
>+ break;
>+ imageChunks.push(chunk);
>+ }
>+ } catch (e) {
>+ Utils.log(e);
>+ }
>+ entry.close();
>+ }
When you get this callback, it actually just means that the cache entry is opened, but it didn't do any reading from the disk for you. You'll still want to read from the disk asynchronously (I know you filed bug 632238, but this cannot land if it's adding synchronous disk I/O).
Attachment #510770 -
Flags: review?(sdwilsh) → review-
Comment 42•14 years ago
|
||
All above issues should be addressed now.
Attachment #510770 -
Attachment is obsolete: true
Attachment #511155 -
Flags: review?(sdwilsh)
Attachment #510770 -
Flags: feedback?(ian)
Comment 43•14 years ago
|
||
> + saveThumbnail: function Storage_saveThumbnail(url, imageData) {
> + let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url,
> + Ci.nsICache.ACCESS_WRITE, true);
You should use asyncOpenCacheEntry() when saving thumbnail too.
Comment 44•14 years ago
|
||
Renominating for hardblocking.
This bug increases I/O, memory usage and can really hurt startup (sessionstore file can become huge). See also comment 21.
blocking2.0: final+ → ?
Whiteboard: [softblocker]
Comment 45•14 years ago
|
||
I'm blocking on this defensively - there are no numbers at all in this bug. Assertions like the ones comment #44 make might be correct...to a degree that's surprising in one way or the other. We can't know without some profiling.
How much bigger is sessionstore.js with and without these images? And how does that effect startup time? (Mine is 5mb fwiw)
How does this change effect Panorama thumbnail display performance? (they fill in pretty slowly for me now fwiw)
We can't just throw code at the tree and see what happens. Please do some measurements, even if it's just dump()ing times, to see what the effect of these changes are. For startup, you can use about:startup now. I'd be glad to provide my megasessionstore.js if you'd like it for testing.
Also, does the patch clean out old thumbnail data from pre-existing sessions?
Updated•14 years ago
|
Whiteboard: [hardblocker] → [hardblocker] [has patch]
Comment 46•14 years ago
|
||
Here are some numbers, using Win7 x64.
Test of about:startup times: (main, firstPain, sessionRestored)
Half of the samples were taken with TabView visible on startup, the other half taken with the browser visible.
Without patch:
Tabview visible:
118,869,1723
132,888,1755
31,784,1627
Browser visible:
39,801,1663
106,816,1670
659,1361,2199
With patch:
Tabview visible:
342,1101,1944
81,845,1698
36,788,1628
Browser visible:
116,843,1681
470,1211,2048
107,826,1657
Test of sessionstore.js sizes, 100 random pages from metafilter.com.
Stacked to largest size before showing the group:
Without patch: 2,763kB
With patch: 1,783kB
Group maximized to window (2 sample sets):
Without patch: 3,026kB, 3,246kB
With patch: 2,138kB, 2,363kB
Analysis:
Startup times, given noise in samples, does not appear to be affected by this patch. Image data takes up ~10kB on average per tab. For 400 tabs (our current upper bar), this would increase the session store by ~4MB.
Comment 47•14 years ago
|
||
about:startup's values for each datapoint are ms since the event occurred, so something's not right with the numbers above - they're in various different orders.
Also, does the patch clean old thumbnail data from the session restore data?
Comment 48•14 years ago
|
||
Filesize results look great, down ~1mb for that sample.
Comment 49•14 years ago
|
||
Yes, the patch cleans out old thumbnail data, which is why the file sizes were down :)
Can you explain a bit better why the numbers don't look correct to you? Those numbers are from the process start to the first event of each. Each data point is from a separate process. What order should they be in?
Comment 50•14 years ago
|
||
Re: cleaning - wasn't clear whether you tested w/ new profile, or just ran the same profile without-and-then-with the patch to get those numbers. Thanks for clarifying.
Re: about:startup
> Test of about:startup times: (main, firstPain, sessionRestored)
> Without patch:
> Tabview visible:
> 118,869,1723
> 132,888,1755
> 31,784,1627
The above indicates that session restored occurred before main, which isn't possible :)
Comment 51•14 years ago
|
||
Let's look at the first data line:
main 118, firstPaint 869, sessionRestored 1723
This is saying that main was hit 118ms after process start.
firstPaint was 869ms after process start.
sessionRestored was 1723ms after process start.
So, this all makes sense to me, in that sessionRestored happened 1605ms after main. Am I interpreting this wrong?
Comment 52•14 years ago
|
||
Sean, ha I was reading it as 3 lines of really huge times. Never mind, makes sense now!
Comment 53•14 years ago
|
||
Fixed all test failures. I had to change the async load code to load what it can from the sessionstore, and defer the thumbnail as a separate update. The reason for this was that going into private browsing mode and back does not play nicely if the tabs are not loaded in a synchronous manner. The thumbnails are still fine as async loaders, though.
Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d28fbe4e5447
Attachment #511155 -
Attachment is obsolete: true
Attachment #511699 -
Flags: review?(sdwilsh)
Attachment #511155 -
Flags: review?(sdwilsh)
Comment 54•14 years ago
|
||
(In reply to comment #53)
> Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d28fbe4e5447
You should really use the try-chooser syntax (http://people.mozilla.org/~lsblakk/trychooser/trychooser.html) and get performance data on Ts and post the results in the bug (http://perf.snarkfest.net/compare-talos/index.html?tests=ts,ts_shutdown).
Also, getting tp4 and ts numbers from your local machine (https://wiki.mozilla.org/StandaloneTalos) with a profile with panorama data might be a good idea too.
Comment 55•14 years ago
|
||
Sent to try, with Talos enabled: http://tbpl.mozilla.org/?tree=MozillaTry&rev=8ec3dcb3eeff
Comment 56•14 years ago
|
||
Perf comparison results. Not finished yet, but this link should remain valid:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b57524d6706f&newRev=8ec3dcb3eeff&tests=ts,ts_shutdown&submit=true
Comment 57•14 years ago
|
||
sdwilsh, it looks like the delay% is up a few percent, down for others, so I'm assuming that this is within reason. What else do you need for an r+ so we can move forward to approval?
Comment 58•14 years ago
|
||
(In reply to comment #57)
> sdwilsh, it looks like the delay% is up a few percent, down for others, so I'm
> assuming that this is within reason. What else do you need for an r+ so we can
> move forward to approval?
I need to sit down and actually review it, which will probably happen Monday (I have been out of town Wed - Fri).
Comment 59•14 years ago
|
||
Apart Ts, Tp4_pbytes and Tp4_rss seem to show a slight improvement too. Unfortunately all of Talos tests don't have a sized sessionstore, so the results are mostly near noise
Comment 60•14 years ago
|
||
Given the numbers that we've got now, I propose this be dropped from hardblocker.
Comment 61•14 years ago
|
||
(In reply to comment #59)
> Apart Ts, Tp4_pbytes and Tp4_rss seem to show a slight improvement too.
> Unfortunately all of Talos tests don't have a sized sessionstore, so the
> results are mostly near noise
We can run with an arbitrary profile with standalone talos.
Comment 62•14 years ago
|
||
(In reply to comment #56)
With more baseline data points:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b57524d6706f,da205ca44184,409d5db222eb&newRev=8ec3dcb3eeff&tests=ts,ts_shutdown&submit=true
Comment 63•14 years ago
|
||
(In reply to comment #53)
> Fixed all test failures. I had to change the async load code to load what it
> can from the sessionstore, and defer the thumbnail as a separate update. The
> reason for this was that going into private browsing mode and back does not
> play nicely if the tabs are not loaded in a synchronous manner. The thumbnails
> are still fine as async loaders, though.
This should be fine. AFAIK, session store keeps it's data in memory.
Comment 64•14 years ago
|
||
Comment on attachment 511699 [details] [diff] [review]
v6
>+++ b/browser/base/content/tabview/storage.js
>+ // Function: saveThumbnail
>+ // Save passed in image data for given url to cache
>+ saveThumbnail: function Storage_saveThumbnail(url, imageData) {
>+ let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url,
>+ Ci.nsICache.ACCESS_WRITE, true);
>+ let entryOutStream = entry.openOutputStream(0);
>+ let inputStream = new this.StringInputStream(imageData, imageData.length);
>+ NetUtil.asyncCopy(inputStream, entryOutStream, function(){
>+ entry.close();
>+ });
>+ },
You seem to have missed my comments in comment 41 about this method.
> saveTab: function Storage_saveTab(tab, data) {
> Utils.assert(tab, "tab");
>
>+ if (data != null) {
>+ let imageData = data.imageData;
>+ // Remove imageData from payload
>+ delete data.imageData;
>+ if(imageData != null) {
>+ this.saveThumbnail(data.url, imageData);
>+ // Notify subscribers
>+ tab._tabViewTabItem._sendToSubscribers("savedCachedImageData");
Do you really want to notify this before we have written the cached image data to disk? saveThumbnail is async now, so it hasn't actually happened when we dispatch this.
>+ // Load tab data from session store and return it, but pass
>+ // imageDataCB on to the thumbnail loader to run asynchronously.
>+ getTabData: function Storage_getTabData(tab, imageDataCb) {
What is imageDataCb? The variable doesn't tell me what it is, and the comments don't help either. Please be more descriptive and/or add more comments.
>+// ##########
>+// Class: CacheListener
>+// Generic CacheListener for feeding to asynchronous cache calls.
>+function CacheListener(callback) {
>+ this.callback = callback;
Describe what the callback gets here please. In general, the comments for methods need to be more descriptive than what has been done in the past. I shouldn't have to read a bunch of code to figure out this information if we provide comments like this.
>+ onCacheEntryAvailable: function(entry, access, status) {
>+ if (this.callback) {
When would callback be null?
>+ this.callback(entry, access, status);
In the callback you've written above, you do not check that status is succesful, nor do you check that the access you were granted was what you asked for. You need to do both of these things (and you can then stop checking entry).
>+++ b/browser/base/content/tabview/tabview.js
> Cu.import("resource:///modules/tabview/AllTabs.jsm");
> Cu.import("resource:///modules/tabview/utils.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/NetUtil.jsm");
Does this load at startup? If so, you should turn this into a lazy getter.
Also, it should be noted that I'm not a browser peer, so while my review is useful, it's not enough to land this.
Attachment #511699 -
Flags: review?(sdwilsh) → review-
Reporter | ||
Comment 65•14 years ago
|
||
(In reply to comment #64)
> Comment on attachment 511699 [details] [diff] [review]
> Also, it should be noted that I'm not a browser peer, so while my review is
> useful, it's not enough to land this.
Shawn, thank you for all your help on this bug! Who do you think should be doing the final review?
Updated•14 years ago
|
Whiteboard: [hardblocker] [has patch] → [hardblocker][needs new patch]
Assignee | ||
Comment 66•14 years ago
|
||
Taking until Sean is back.
Assignee: seanedunn → tim.taubert
Status: NEW → ASSIGNED
Comment 67•14 years ago
|
||
(In reply to comment #66)
> Taking until Sean is back.
If you have other blockers to work on, I can steal this too.
Assignee | ||
Comment 68•14 years ago
|
||
(In reply to comment #67)
> If you have other blockers to work on, I can steal this too.
I have no other blockers at the moment, sorry :)
But I have one question right away: I modified the patch like you said in comment #41 so that saveThumbnail() works async now. With this I get a lot of errors in the console:
JavaScript Error: "attempt to run compile-and-go script on a cleared scope"
I think this is only happens when our DOMWindowClosed observer is notified and calls TabItems.saveAll(). So the window is recycled and the storage code tries to call already GCed listeners. So we should save thumbnails synchronously when the DOM window is closed, right?
Comment 69•14 years ago
|
||
Copying brendan and gal for more looking into the "attempt to run compile-and-go script on a cleared scope" error above.
Comment 70•14 years ago
|
||
(In reply to comment #68)
> I think this is only happens when our DOMWindowClosed observer is notified and
> calls TabItems.saveAll(). So the window is recycled and the storage code tries
> to call already GCed listeners. So we should save thumbnails synchronously when
> the DOM window is closed, right?
To be clear, are you talking about when calling saveTab? saveThumbnail doesn't notify any observers.
Don't the observers release themselves when their windows goes away? If they do, I am a bit confused as to how we'd be notifying them if the window went away.
Assignee | ||
Comment 71•14 years ago
|
||
function domWinClosedObserver(subject, topic, data) {
if (topic == "domwindowclosed" && subject == gWindow) {
if (self.isTabViewVisible())
GroupItems.removeHiddenGroups();
TabItems.saveAll(true);
self._save();
}
}
This is where we save all tab thumbnails when the parent chrome window gets closed. Now that tab thumbnails are stored asynchronously we call cache.asyncOpenCacheEntry() and pass a cache listener. After the window got GC'ed the cache entry is available and the cache listener is notified. Because the scope has already been cleared we get the error mentioned above. The execution order must be something like:
1.) parent chrome windows is about to be closed ("domwindowclosed")
2.) TabItems.saveAll() -> cache.asyncOpenCacheEntry()
3.) parent chrome window is closed and GC'ed
4.) the cache entry is now available and the service tries to notify the cache listener
5.) JavaScript Error: "attempt to run compile-and-go script on a cleared scope"
(This is what I assume happens.)
Comment 72•14 years ago
|
||
(In reply to comment #71)
That (unfortunately) makes sense. I suppose the right fix here is to move this saving code to a JSM so that it lives longer, but I'm not sure we want to do that for Firefox 4.0. The tradeoff is doing synchronous disk I/O every time we save a tab thumbnail.
Do we save thumbnails even if the user has not opened panorama?
Comment 73•14 years ago
|
||
The exception is new but what you are trying to do there didn't work before either. After a window is closed you can't run code on it. Thats a bug we will try to fix for 5.
Assignee | ||
Comment 74•14 years ago
|
||
(In reply to comment #73)
> The exception is new but what you are trying to do there didn't work before
> either. After a window is closed you can't run code on it. Thats a bug we will
> try to fix for 5.
I'm ok with that limitation :)
(In reply to comment #72)
> (In reply to comment #71)
> That (unfortunately) makes sense. I suppose the right fix here is to move this
> saving code to a JSM so that it lives longer, but I'm not sure we want to do
> that for Firefox 4.0. The tradeoff is doing synchronous disk I/O every time we
> save a tab thumbnail.
What about switching to synchronous thumbnail saving _after_ having received "domwindowclosed"? I think that could work and keep the window alive until we finished.
Comment 75•14 years ago
|
||
(In reply to comment #73)
> The exception is new but what you are trying to do there didn't work before
> either. After a window is closed you can't run code on it. Thats a bug we will
> try to fix for 5.
So comment 71 step 3 in Tim's diagnosis should say "parent window is closed", not "... and GC'ed". The parent is not garbage as it is referenced from live objects. This mandatory "scope clearing" is ancient and it compensates for leaks, which (Andreas is noting here) we'll fix after fx4.
/be
Assignee | ||
Updated•14 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 76•14 years ago
|
||
(In reply to comment #64)
> >+ // Save passed in image data for given url to cache
> >+ saveThumbnail: function Storage_saveThumbnail(url, imageData) {
> >+ },
Works now asnychronously. After receiving "domwindowclosed" this method switches to synchronous mode.
> Do you really want to notify this before we have written the cached image data
> to disk? saveThumbnail is async now, so it hasn't actually happened when we
> dispatch this.
Fixed.
> >+ // Load tab data from session store and return it, but pass
> >+ // imageDataCB on to the thumbnail loader to run asynchronously.
> >+ getTabData: function Storage_getTabData(tab, imageDataCb) {
> What is imageDataCb? The variable doesn't tell me what it is, and the comments
> don't help either. Please be more descriptive and/or add more comments.
Added and corrected comments (on other places, too).
> >+ onCacheEntryAvailable: function(entry, access, status) {
> >+ if (this.callback) {
> When would callback be null?
Removed.
> >+ this.callback(entry, access, status);
> In the callback you've written above, you do not check that status is
> succesful, nor do you check that the access you were granted was what you asked
> for. You need to do both of these things (and you can then stop checking
> entry).
Now handled in Storage._openCacheEntry().
> >+Cu.import("resource://gre/modules/NetUtil.jsm");
> Does this load at startup? If so, you should turn this into a lazy getter.
Fixed.
Additionally: adapted tests and added a new one ensuring the new thumbnail storage works as expected.
Attachment #511699 -
Attachment is obsolete: true
Attachment #513215 -
Flags: review?(sdwilsh)
Comment 77•14 years ago
|
||
Comment on attachment 513215 [details] [diff] [review]
patch v7
>+++ b/browser/base/content/tabview/storage.js
>+ _openCacheEntry: function Storage__openCacheEntry(url, access, successCallback, errorCallback) {
>+ let onCacheEntryAvailable = function (entry, accessGranted, status) {
>+ if (entry && access == accessGranted && status == 0)
status is an nsresult. Use Components.isSuccessCode instead.
>+ try {
>+ this._cacheSession.asyncOpenCacheEntry(this.CACHE_PREFIX + url,
>+ access, new CacheListener(onCacheEntryAvailable));
>+ } catch (e) {
>+ errorCallback();
>+ }
When would this throw?
>+ saveThumbnail: function Storage_saveThumbnail(url, imageData, callback) {
>+ // save thumbnail using synchronous IO if the window is about to close
>+ if (UI.isDOMWindowClosing) {
>+ let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url,
>+ Ci.nsICache.ACCESS_WRITE, true);
>+ if (!entry) {
>+ callback(false);
>+ return;
>+ }
It seems like you really want to move the opening too _openCacheEntry and have it call the sync or async method depending on the dom window closing too.
>+ gNetUtil.asyncCopy(inputStream, entryOutStream, function () {
>+ entry.close();
>+ callback(true);
The asyncCopy could have failed. In the callback, it gives you an nsresult indicating success or failure that you should be checking and passing true or false here accordingly.
>+ // 65k chunks with a max of PR_UINT32_MAX
>+ let storageStream = new self.StorageStream(0x10000, 0xFFFFFFFF, null);
Please pull both of these magic numbers out into constants.
>+ gNetUtil.asyncCopy(nativeInputStream, storageOutStream, function (result) {
>+ // check if the parent windows is closed or about to close
>+ if (typeof UI == "object" && !UI.isDOMWindowClosing) {
>+ let storageInStream = storageStream.newInputStream(0);
>+ let str = gNetUtil.readInputStreamToString(storageInStream,
>+ storageInStream.available());
>+ callback(true, str);
You also don't check result here with Components.isSuccessCode
> saveTab: function Storage_saveTab(tab, data) {
> Utils.assert(tab, "tab");
>
>+ if (data != null) {
>+ let imageData = data.imageData;
>+ // Remove imageData from payload
>+ delete data.imageData;
>+ if (imageData != null) {
>+ this.saveThumbnail(data.url, imageData, function (status) {
>+ // Notify subscribers
>+ tab._tabViewTabItem._sendToSubscribers("savedCachedImageData");
>+ });
>+ }
>+ }
>+
> this._sessionStore.setTabValue(tab, this.TAB_DATA_IDENTIFIER,
> JSON.stringify(data));
In what instances would data be null? Why do we still want to call into sessionStore when it is null?
>+ if (existingData)
>+ this.loadThumbnail(existingData.url, function (status, imageData) {
>+ callback(imageData);
>+ // Notify subscribers
>+ tab._tabViewTabItem._sendToSubscribers("loadedCachedImageData");
You don't actually check status here, although it could easily be false.
>+++ b/browser/base/content/tabview/tabitems.js
>+ let currentUrl = self.tab.linkedBrowser.currentURI.spec;
>+ // If we have a cached image, then show it if the loaded URL matches
>+ // what the cache is from, OR the loaded URL is blank, which means
>+ // that the page hasn't loaded yet.
>+ if (tabData.imageData && (tabData.url == currentUrl ||
>+ currentUrl == 'about:blank')) {
I am going to suggest formatting it like this so it's easier to read:
>+ if (tabData.imageData &&
>+ (tabData.url == currentUrl || currentUrl == 'about:blank')) {
Can we test the case where it's about:blank too?
>+++ b/browser/base/content/tabview/tabview.js
>+XPCOMUtils.defineLazyGetter(this, "gNetUtil", function() {
>+ Cu.import("resource://gre/modules/NetUtil.jsm");
>+ return NetUtil;
>+});
I believe you actually want to import that into a local object, and return the NetUtil property off of that:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/NetworkHelper.jsm#60
I didn't look at the tests yet.
Attachment #513215 -
Flags: review?(sdwilsh) → review-
Assignee | ||
Comment 78•14 years ago
|
||
(In reply to comment #77)
> status is an nsresult. Use Components.isSuccessCode instead.
Fixed.
> >+ try {
> >+ this._cacheSession.asyncOpenCacheEntry(this.CACHE_PREFIX + url,
> >+ access, new CacheListener(onCacheEntryAvailable));
> >+ } catch (e) {
> >+ errorCallback();
> >+ }
> When would this throw?
Never, removed.
> It seems like you really want to move the opening too _openCacheEntry and have
> it call the sync or async method depending on the dom window closing too.
Of course, fixed.
> The asyncCopy could have failed. In the callback, it gives you an nsresult
> indicating success or failure that you should be checking and passing true or
> false here accordingly.
Fixed.
> >+ // 65k chunks with a max of PR_UINT32_MAX
> >+ let storageStream = new self.StorageStream(0x10000, 0xFFFFFFFF, null);
> Please pull both of these magic numbers out into constants.
Fixed.
> > saveTab: function Storage_saveTab(tab, data) {
> > Utils.assert(tab, "tab");
> >
> >+ if (data != null) {
> >+ let imageData = data.imageData;
> >+ // Remove imageData from payload
> >+ delete data.imageData;
> >+ if (imageData != null) {
> >+ this.saveThumbnail(data.url, imageData, function (status) {
> >+ // Notify subscribers
> >+ tab._tabViewTabItem._sendToSubscribers("savedCachedImageData");
> >+ });
> >+ }
> >+ }
> >+
> > this._sessionStore.setTabValue(tab, this.TAB_DATA_IDENTIFIER,
> > JSON.stringify(data));
> In what instances would data be null? Why do we still want to call into
> sessionStore when it is null?
We call saveTab(tab, null) when wiping session store data (e.g. the tab gets closed).
> >+ if (existingData)
> >+ this.loadThumbnail(existingData.url, function (status, imageData) {
> >+ callback(imageData);
> >+ // Notify subscribers
> >+ tab._tabViewTabItem._sendToSubscribers("loadedCachedImageData");
> You don't actually check status here, although it could easily be false.
Fixed.
> >+ let currentUrl = self.tab.linkedBrowser.currentURI.spec;
> >+ // If we have a cached image, then show it if the loaded URL matches
> >+ // what the cache is from, OR the loaded URL is blank, which means
> >+ // that the page hasn't loaded yet.
> >+ if (tabData.imageData && (tabData.url == currentUrl ||
> >+ currentUrl == 'about:blank')) {
> I am going to suggest formatting it like this so it's easier to read:
Fixed.
> >+ if (tabData.imageData &&
> >+ (tabData.url == currentUrl || currentUrl == 'about:blank')) {
>
> Can we test the case where it's about:blank too?
browser_tabview_bug597248.js tests now whether the thumbnail for about:blank is restored, too.
> >+XPCOMUtils.defineLazyGetter(this, "gNetUtil", function() {
> >+ Cu.import("resource://gre/modules/NetUtil.jsm");
> >+ return NetUtil;
> >+});
> I believe you actually want to import that into a local object, and return the
> NetUtil property off of that:
Fixed.
Attachment #513215 -
Attachment is obsolete: true
Attachment #513559 -
Flags: review?(sdwilsh)
Comment 79•14 years ago
|
||
Comment on attachment 513559 [details] [diff] [review]
patch v8
>+++ b/browser/base/content/tabview/storage.js
>+ let stringInputStream = this.StringInputStream;
Capitalize 'string' please so it looks like other JS objects
>+ let chunkSize = 0x10000; // 65k
I never did ask how we came up with this number...
>+ let chunkSize = 0x10000; // 65k
>+ let maxBytes = 0xFFFFFFFF; // PR_UINT32_MAX
I actually meant const for both of these, and using PR_UINT32_MAX for the name of the second one is preferred.
>+++ b/browser/base/content/test/tabview/browser_tabview_bug597248.js
>-let gTabsProgressListener = {
>+/*let gTabsProgressListener = {
> onStateChange: function(browser, webProgress, request, stateFlags, status) {
> // ensure about:blank doesn't trigger the code
> if ((stateFlags & Ci.nsIWebProgressListener.STATE_STOP) &&
> (stateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW) &&
> browser.currentURI.spec != "about:blank") {
> if (newTabOne.linkedBrowser == browser)
> restoredNewTabOneLoaded = true;
> else if (newTabTwo.linkedBrowser == browser)
>@@ -226,18 +175,9 @@ function updateAndCheck() {
> ok(!tabItem.isShowingCachedData(),
> "Tab item is not showing cached data anymore. " +
> tabItem.tab.linkedBrowser.currentURI.spec);
> });
>
> // clean up and finish
> restoredWin.close();
> finish();
>-}
>-
>-function load(tab, url, callback) {
>- tab.linkedBrowser.addEventListener("load", function (event) {
>- tab.linkedBrowser.removeEventListener("load", arguments.callee, true);
>- callback();
>- }, true);
>- tab.linkedBrowser.loadURI(url);
>-}
>-
>+}*/
Hmm, what's this about?
r=sdwilsh with comments/questions addressed/answered.
Attachment #513559 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 80•14 years ago
|
||
(In reply to comment #79)
> >+ let chunkSize = 0x10000; // 65k
> I never did ask how we came up with this number...
I don't know either :/ Is there a reasonable chunk size to use when reading cache data?
> >+++ b/browser/base/content/test/tabview/browser_tabview_bug597248.js
> >-let gTabsProgressListener = {
> >+/*let gTabsProgressListener = {
> > onStateChange: function(browser, webProgress, request, stateFlags, status) {
> > // ensure about:blank doesn't trigger the code
> > if ((stateFlags & Ci.nsIWebProgressListener.STATE_STOP) &&
> > (stateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW) &&
> > browser.currentURI.spec != "about:blank") {
> > if (newTabOne.linkedBrowser == browser)
> > restoredNewTabOneLoaded = true;
> > else if (newTabTwo.linkedBrowser == browser)
> >@@ -226,18 +175,9 @@ function updateAndCheck() {
> > ok(!tabItem.isShowingCachedData(),
> > "Tab item is not showing cached data anymore. " +
> > tabItem.tab.linkedBrowser.currentURI.spec);
> > });
> >
> > // clean up and finish
> > restoredWin.close();
> > finish();
> >-}
> >-
> >-function load(tab, url, callback) {
> >- tab.linkedBrowser.addEventListener("load", function (event) {
> >- tab.linkedBrowser.removeEventListener("load", arguments.callee, true);
> >- callback();
> >- }, true);
> >- tab.linkedBrowser.loadURI(url);
> >-}
> >-
> >+}*/
> Hmm, what's this about?
These parts of the test were disabled some time ago (because of intermittent oranges) by commenting out the calls to these functions. I just commented out the whole part to make sure nothing of this is still called in the test - and to gain some more overview :)
Reporter | ||
Updated•14 years ago
|
Attachment #513559 -
Flags: review+
Updated•14 years ago
|
Whiteboard: [hardblocker][needs new patch] → [hardblocker][has patch]
Assignee | ||
Comment 81•14 years ago
|
||
Comment on attachment 513559 [details] [diff] [review]
patch v8
Passed try:
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=d1251450e3a7
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs landing]
Per comment 64 this still needs review from a browser peer, no?
Keywords: checkin-needed
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker][has patch]
Assignee | ||
Comment 83•14 years ago
|
||
Yeah and the attached patch is also not up-to-date because I'm waiting for response.
Comment 84•14 years ago
|
||
(In reply to comment #82)
> Per comment 64 this still needs review from a browser peer, no?
Actually, we're good now. Ian is the sub module owner of panorama, and he r+ the patch, so it's good to go.
Assignee | ||
Comment 85•14 years ago
|
||
(In reply to comment #79)
> >+++ b/browser/base/content/tabview/storage.js
> >+ let stringInputStream = this.StringInputStream;
> Capitalize 'string' please so it looks like other JS objects
Fixed.
> >+ let chunkSize = 0x10000; // 65k
> I never did ask how we came up with this number...
I didn't change the number for now. If you have a reasonable value for this please tell me :)
> >+ let chunkSize = 0x10000; // 65k
> >+ let maxBytes = 0xFFFFFFFF; // PR_UINT32_MAX
> I actually meant const for both of these, and using PR_UINT32_MAX for the name
> of the second one is preferred.
Fixed.
Attachment #513559 -
Attachment is obsolete: true
Comment 86•14 years ago
|
||
If this is ready for checkin, please say so explicitly and I'll see if I have time to push it today.
Assignee | ||
Comment 87•14 years ago
|
||
(In reply to comment #84)
> Actually, we're good now. Ian is the sub module owner of panorama, and he r+
> the patch, so it's good to go.
Ok, cool :)
(In reply to comment #86)
> If this is ready for checkin, please say so explicitly and I'll see if I have
> time to push it today.
Thanks in advance!
Keywords: checkin-needed
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs landing]
Comment 88•14 years ago
|
||
(In reply to comment #85)
> I didn't change the number for now. If you have a reasonable value for this
> please tell me :)
I don't. This is probably fine.
Comment 89•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker][has patch]
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
•