Store thumbnails in the browser's image cache rather than in sessionstore

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: iangilman, Assigned: ttaubert)

Tracking

({memory-footprint, perf})

Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker][has patch])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
Sounds perf-y. Sean, how do feel about tackling this?
Assignee: nobody → seanedunn
Blocks: 604213
Targeting beta 8. Sean, does that sound alright?
Blocks: 597043
Target Milestone: --- → Firefox 4.0b8

Updated

9 years ago
Depends on: 609685

Comment 3

9 years ago
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
Please have a look at the patch for bug 597248.  There are changes to the thumbnail code.

Comment 5

9 years ago
Thanks, Raymond. I hadn't seen that bug -- I'll make this a dependency upon that.

Updated

9 years ago
Depends on: 597248

Updated

9 years ago
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

9 years ago
See also bug 615704 on a related topic.
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

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

Comment 9

9 years ago
bug spam (moving to b10)
Blocks: 608028

Comment 10

9 years ago
bug spam
No longer blocks: 597043
(Reporter)

Comment 11

8 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

8 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.
Based on comment 12, tempted to punt this to the future.
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.
blocking2.0: --- → ?

Comment 15

8 years ago
bugspam. Moving b10 to b11
Blocks: 627096

Comment 16

8 years ago
bugspam. Removing b10
No longer blocks: 608028
(Reporter)

Comment 17

8 years ago
Too late for this for Fx4
No longer blocks: 627096
Target Milestone: Firefox 4.0b8 → Future

Comment 18

8 years ago
what? you can't store these images in sessionstore. that sounds ...insane.

Updated

8 years ago
Keywords: footprint
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]
Target Milestone: Future → ---

Comment 20

8 years ago
why do we cache them at all?

Comment 21

8 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

8 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

8 years ago
Are these thumbnails used when restoring a session?

Updated

8 years ago
Severity: normal → blocker
Priority: -- → P1
(Reporter)

Comment 24

8 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

8 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?
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.
Can you first stop using session restore for this and then explore using the image cache as a second step?
(Reporter)

Updated

8 years ago
Blocks: 627096
Priority: -- → P1

Updated

8 years ago
Depends on: 628701

Comment 28

8 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #509797 - Flags: review?(ian)
Attachment #509797 - Flags: feedback?(ian)
(Reporter)

Comment 30

8 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 31

8 years ago
Posted patch v2 (obsolete) — Splinter Review
Updated
Attachment #509797 - Attachment is obsolete: true

Comment 32

8 years ago
Posted patch v3 (obsolete) — Splinter Review
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

8 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 on attachment 509829 [details] [diff] [review]
v3

Shawn, would you mind feedbacking here? :)
Attachment #509829 - Flags: feedback?(sdwilsh)
Bjarne and Michal, can you guys have a look at the patch here and share thoughts on this approach?
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-
Depends on: 632227

Comment 37

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

Updated

8 years ago
Blocks: 632238

Updated

8 years ago
Depends on: 632280

Comment 39

8 years ago
Posted patch v4 (obsolete) — Splinter Review
(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

8 years ago
Er, I mean ian. Added Ian for feedback :)
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

8 years ago
Posted patch v5 (obsolete) — Splinter Review
All above issues should be addressed now.
Attachment #510770 - Attachment is obsolete: true
Attachment #511155 - Flags: review?(sdwilsh)
Attachment #510770 - Flags: feedback?(ian)

Updated

8 years ago
Depends on: 602432

Updated

8 years ago
No longer depends on: 602432
> +  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.
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]
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?
blocking2.0: ? → final+
Keywords: perf
Whiteboard: [hardblocker]

Updated

8 years ago
Whiteboard: [hardblocker] → [hardblocker] [has patch]

Comment 46

8 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.
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?
Filesize results look great, down ~1mb for that sample.

Comment 49

8 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?
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

8 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?
Sean, ha I was reading it as 3 lines of really huge times. Never mind, makes sense now!

Comment 53

8 years ago
Posted patch v6 (obsolete) — Splinter Review
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)
(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 56

8 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

8 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?
(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).
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
Given the numbers that we've got now, I propose this be dropped from hardblocker.
(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.
(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 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

8 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?
Whiteboard: [hardblocker] [has patch] → [hardblocker][needs new patch]
Taking until Sean is back.
Assignee: seanedunn → tim.taubert
Status: NEW → ASSIGNED
(In reply to comment #66)
> Taking until Sean is back.
If you have other blockers to work on, I can steal this too.
(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?
Copying brendan and gal for more looking into the "attempt to run compile-and-go script on a cleared scope" error above.
(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.
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.)
(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

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

8 years ago
Version: unspecified → Trunk
Posted patch patch v7 (obsolete) — Splinter Review
(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 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-
Posted patch patch v8 (obsolete) — Splinter Review
(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 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+
(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

8 years ago
Attachment #513559 - Flags: review+

Updated

8 years ago
Whiteboard: [hardblocker][needs new patch] → [hardblocker][has patch]
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]
Yeah and the attached patch is also not up-to-date because I'm waiting for response.
(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.
(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
If this is ready for checkin, please say so explicitly and I'll see if I have time to push it today.
(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]
(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.
http://hg.mozilla.org/mozilla-central/rev/4ae4b97f392b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker][has patch]
(Assignee)

Updated

8 years ago
Depends on: 635668
(Assignee)

Updated

8 years ago
No longer depends on: 635668
(Assignee)

Updated

8 years ago
Depends on: 638083
Depends on: 664670
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.