Last Comment Bug 627239 - Don't store thumbnails for cache:control:no-store pages
: Don't store thumbnails for cache:control:no-store pages
Status: RESOLVED FIXED
[sg:low local]
: privacy
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P1 major
: Firefox 7
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on: 625217 685104
Blocks: 658210 660175
  Show dependency treegraph
 
Reported: 2011-01-19 16:03 PST by Michael Yoshitaka Erlewine [:mitcho]
Modified: 2016-04-12 14:00 PDT (History)
19 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (1.94 KB, patch)
2011-01-23 09:19 PST, Michael Yoshitaka Erlewine [:mitcho]
no flags Details | Diff | Splinter Review
Patch v1, try 2 (7.21 KB, patch)
2011-01-23 09:23 PST, Michael Yoshitaka Erlewine [:mitcho]
ian: review+
raymond: feedback+
Details | Diff | Splinter Review
Patch v1.1 (7.17 KB, patch)
2011-01-25 12:08 PST, Michael Yoshitaka Erlewine [:mitcho]
ian: review+
Details | Diff | Splinter Review
Patch for checkin (7.43 KB, patch)
2011-01-26 21:28 PST, Michael Yoshitaka Erlewine [:mitcho]
no flags Details | Diff | Splinter Review
WIP (9.01 KB, patch)
2011-05-19 02:12 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v1 (36.08 KB, patch)
2011-05-20 10:32 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v2 (36.93 KB, patch)
2011-05-23 23:01 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v2 (36.87 KB, patch)
2011-05-23 23:10 PDT, Raymond Lee [:raymondlee]
ehsan: review+
Details | Diff | Splinter Review
Patch for checkin (36.30 KB, patch)
2011-05-30 23:52 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Michael Yoshitaka Erlewine [:mitcho] 2011-01-19 16:03:59 PST
Session store has a notion of "privacy level", whereby some information for some tabs are not stored in session store (?). We should follow this in determining whether to store thumbnails or not.

From security review.
Comment 1 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-01-19 16:36:00 PST
Session restore has 2 preferences (browser.sessionstore.privacy_level & privacy_level_deferred) that are used to determine what should be saved (cookies, form data) for HTTPS vs HTTP vs not at all.

But since panorama is just storing data in extData on the tab, we (sessionstore) don't know it's associated with a particular URL. Panorama would either need to look at the pref & url itself, or just decide not to save https thumbnails across sessions & clear it out appropriately.
Comment 2 Michael Yoshitaka Erlewine [:mitcho] 2011-01-19 16:51:18 PST
(In reply to comment #1)
> But since panorama is just storing data in extData on the tab, we
> (sessionstore) don't know it's associated with a particular URL. Panorama would
> either need to look at the pref & url itself, or just decide not to save https
> thumbnails across sessions & clear it out appropriately.

I think not storing https thumbnails across sessions at all is a good idea, particularly as cached thumbnails can be seen indefinitely in subsequent sessions if bartab functionality is turned on. Perhaps we could instead display a "locked" (padlock) icon in lieu of the thumbnail until people actually load that page?
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-01-20 15:32:43 PST
Morphing the title to reflect what sounds like the plan: not saving for https pages at all. I assume the thumbnails would be in memory so still work within a session during which they were visible?

Also, CCing Alex since I'm guessing he won't like the lock icon idea (but he can prove me wrong!)
Comment 4 chris hofmann 2011-01-20 19:19:19 PST
> Also, CCing Alex since I'm guessing he won't like the lock icon idea (but he
> can prove me wrong!)

a larry icon might be a good alternative.
Comment 5 chris hofmann 2011-01-21 06:45:44 PST
or maybe and even better one would be to show text of the leading part of the https url

  ___________________     _________________
  |                 |    |                  |
  |                 |    |                  |
  |     https://    |    |    https://      |
  |mail.google.com/ |    |bankofamerica.com |
  |       mail      |    |accounts-overview |
  |                 |    |                  |
  |                 |    |                  |
  |                 |    |                  |
   __________________     ___________________
Comment 6 Alex Faaborg [:faaborg] (Firefox UX) 2011-01-21 19:26:06 PST
>Perhaps we could instead display
>a "locked" (padlock) icon in lieu of the thumbnail until people actually load
>that page?

Let's just display a blank page, this is consistent with what is about to happen (the page isn't cached, it is going to have to load), and the specific reason that the page isn't cached (in this case https), is too technical and abstract for us to successfully convey to the user, regardless of what metaphors, text or visual treatment we attempt to use.
Comment 7 Michael Yoshitaka Erlewine [:mitcho] 2011-01-23 09:19:48 PST
Created attachment 506240 [details] [diff] [review]
Patch v1

With test. Pushed to try.
Comment 8 Michael Yoshitaka Erlewine [:mitcho] 2011-01-23 09:23:17 PST
Created attachment 506241 [details] [diff] [review]
Patch v1, try 2

I fail at patch-making.
Comment 9 Michael Yoshitaka Erlewine [:mitcho] 2011-01-23 17:30:01 PST
Try passed except a known intermittent orange and one failure of this new test just on linux opt. Requested a rerun (bug 628181) on all platforms to see if this test is nondeterministic. Don't want to introduce a new intermittent. :/
Comment 10 Michael Yoshitaka Erlewine [:mitcho] 2011-01-24 17:16:23 PST
Test passed try.
Comment 11 Ian Gilman [:iangilman] 2011-01-25 11:51:42 PST
Comment on attachment 506241 [details] [diff] [review]
Patch v1, try 2

>+      dump("closed and undid\n");

Kill this dump call. 

Otherwise looks great.
Comment 12 :Ehsan Akhgari 2011-01-25 12:06:25 PST
Hmm, dump calls in tests are safe (albeit noisy)...
Comment 13 Michael Yoshitaka Erlewine [:mitcho] 2011-01-25 12:08:22 PST
Created attachment 506847 [details] [diff] [review]
Patch v1.1

Thanks Ian!
Comment 14 Ian Gilman [:iangilman] 2011-01-26 09:58:47 PST
Comment on attachment 506847 [details] [diff] [review]
Patch v1.1

Stop the presses! One more thing: 

>+  let insecure = win.gBrowser.loadOneTab("http://example.com", {inBackground: true});
>+  let secure   = win.gBrowser.loadOneTab("https://example.com", {inBackground: true});

Evidently (see bug 628867) it's frowned upon to actually hit the network with a mochitest, and now that I think of it, makes sense... seems like it invites intermittent oranges. 

So, the question is, how do we test https if we can't hit the network?
Comment 15 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-01-26 10:08:38 PST
(In reply to comment #14)
> So, the question is, how do we test https if we can't hit the network?

example.com is part of our loopback to the local server so that's fine. I don't know if the https part is going to matter though.
Comment 16 Michael Yoshitaka Erlewine [:mitcho] 2011-01-26 10:10:15 PST
(In reply to comment #14)
> Comment on attachment 506847 [details] [diff] [review]
> Patch v1.1
> 
> Stop the presses! One more thing: 
> 
> >+  let insecure = win.gBrowser.loadOneTab("http://example.com", {inBackground: true});
> >+  let secure   = win.gBrowser.loadOneTab("https://example.com", {inBackground: true});
> 
> Evidently (see bug 628867) it's frowned upon to actually hit the network with a
> mochitest, and now that I think of it, makes sense... seems like it invites
> intermittent oranges. 
> 
> So, the question is, how do we test https if we can't hit the network?

As Paul said, http://example.com and https://example.com are part of our test loopback code
Comment 17 Ian Gilman [:iangilman] 2011-01-26 10:12:02 PST
Comment on attachment 506847 [details] [diff] [review]
Patch v1.1

(In reply to comment #15)
> (In reply to comment #14)
> > So, the question is, how do we test https if we can't hit the network?
> 
> example.com is part of our loopback to the local server so that's fine. I don't
> know if the https part is going to matter though.

Cool, thanks!
Comment 18 Mike Beltzner [:beltzner, not reading bugmail] 2011-01-26 12:09:44 PST
This isn't a blocker, but I guess we'd take the approved patch!
Comment 19 Michael Yoshitaka Erlewine [:mitcho] 2011-01-26 21:28:03 PST
Created attachment 507359 [details] [diff] [review]
Patch for checkin
Comment 20 Jonathan Watt [:jwatt] (catching up after vacation) 2011-01-27 23:56:33 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/97e4df251673
Comment 21 Jonathan Watt [:jwatt] (catching up after vacation) 2011-01-28 05:47:28 PST
Backed out in http://hg.mozilla.org/mozilla-central/rev/993b69aa088a due to being a suspect in random orange. More info to follow once a few more cycles have cleared.
Comment 22 Jonathan Watt [:jwatt] (catching up after vacation) 2011-01-28 07:25:35 PST
The new failure I'm concerned about is:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug627239.js | Insecure tab is showing cached data

(file touched by this patch)

Other than ending up in bug 582216 because of staring for other things that occurred at the same time as this failure since I pushed, this file name does not appear in any comment on any other bugzilla bug, so it's very likely new.

The first time this failure occurred was in the push for this patch. It failed on both Linux opt and WinXP opt:

Fri Jan 28 20:54:23 2011
http://tbpl.mozilla.org/?rev=0e50480c408f

Four pushes later, the same thing, again on Linux opt and WinXP opt:

Fri Jan 28 21:56:34 2011
http://tbpl.mozilla.org/?rev=06d9df6ca0fa

The backout was 2 pushes after that.

Since the backout there have been 8 Linux opt runs without this failure, and 2 WinXP opt without the failure.

This is not to say that this push definitely caused it. Someone should keep an eye on tbpl and see if the error occurs again. Maybe try the push again later, or after fixes if there's something wrong with the patch and someone knows what.
Comment 23 Michael Yoshitaka Erlewine [:mitcho] 2011-01-28 07:54:22 PST
Thank you Jonathan. Because this test was landed with this bug, it shouldn't ever happen again. I'll try to work on figuring out the cause of this intermittent so we can land this again.
Comment 24 Nickolay_Ponomarev 2011-01-28 15:54:19 PST
I'm confused. Bug 424872 changed the defaults for browser.sessionstore.privacy_level* (mentioned in comment 1) to be "save all data for all pages", and this patch makes the *thumbnails* not stored for all https pages, even those without sensitive information (like developer.mozilla.org)?

Why? Comment 2 doesn't really give a reason.
Comment 25 Michael Yoshitaka Erlewine [:mitcho] 2011-01-28 23:01:42 PST
(In reply to comment #24)
> Why? Comment 2 doesn't really give a reason.

Comment 1 said there were two options, for technical reasons: either Panorama looks at those prefs directly in its logic, or Panorama just doesn't save thumbs for https tabs at all. Among those two options, comment 2 gives a reason for choosing the latter.
Comment 26 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-01-29 21:43:04 PST
What comment 24 said. https isn't an indication that a page is privacy-sensitive.

(In reply to comment #25)
> Comment 1 said there were two options, for technical reasons: either Panorama
> looks at those prefs directly in its logic, or Panorama just doesn't save
> thumbs for https tabs at all.

There's a third option: save thumbnails for https pages. I don't see a reason *not* to have thumbnails for https pages (given it's not an indication of a privacy-sensitive page), unless a non-standard hidden preference is set (browser.sessionstore.privacy_level isn't exposed in UI anywhere, AFAIK).
Comment 27 Dão Gottwald [:dao] 2011-01-29 23:40:22 PST
Yet another option is to just not store thumbnails, as suggested in bug 604699.
Comment 28 Michael Yoshitaka Erlewine [:mitcho] 2011-02-04 20:32:00 PST
We seem to have a decision point here. Options seem to be:

1. Not store thumbnails for any tabs at least until we move to using the image cache (bug 604699), essentially punting on the possible privacy implications here for the time being (comment 27);
2. Not store thumbnails for any https tab, implicitly understanding them to be more privacy-sensitive (the original proposal and patch here, suggested in the security review);
3. Make no distinction between https and http tabs, i.e. save all thumbnails by default, unless browser.sessionstore.privacy_level says otherwise (comment 26).

Fwiw, work on bug 604699 seems to be progressing.

Could those more familiar with privacy policy please weigh in on the options here? Thank you! :)
Comment 29 Ian Gilman [:iangilman] 2011-02-07 15:33:39 PST
(In reply to comment #28)
> 1. Not store thumbnails for any tabs at least until we move to using the image
> cache (bug 604699), essentially punting on the possible privacy implications
> here for the time being (comment 27);

This doesn't really address the issue here, unless the image cache is somehow more secure than sessionstore?
Comment 30 Michael Yoshitaka Erlewine [:mitcho] 2011-02-07 18:06:55 PST
(In reply to comment #29)
> (In reply to comment #28)
> > 1. Not store thumbnails for any tabs at least until we move to using the image
> > cache (bug 604699), essentially punting on the possible privacy implications
> > here for the time being (comment 27);
> 
> This doesn't really address the issue here, unless the image cache is somehow
> more secure than sessionstore?

You're right. I was just trying to rehash comment 27, which essentially suggests as an option that we just not store any thumbnails, following a similar suggestion in bug 604699.
Comment 31 Michael Yoshitaka Erlewine [:mitcho] 2011-02-09 23:00:38 PST
[bugspam: mitcho's late-night betaN triage; feel free to comment or reverse]
Comment 32 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 18:26:47 PST
Comment on attachment 506847 [details] [diff] [review]
Patch v1.1

(removing approval for backed out patch)
Comment 33 Kevin Hanes 2011-03-31 10:51:58 PDT
bugspam
Comment 34 Raymond Lee [:raymondlee] 2011-03-31 23:16:02 PDT
We are using the cache service and we still don't want to store thumbnails for https pages, right?
Comment 35 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-04-01 03:46:19 PDT
(In reply to comment #34)
> and we still don't want to store thumbnails for https pages, right?

See comment 24 and comment 26. Personally, I want to see this WONTFIX'ed.
Comment 36 chris hofmann 2011-04-01 08:30:39 PDT
> Why? Comment 2 doesn't really give a reason.

panorama should follow the cache rules found here:

http://mxr.mozilla.org/firefox/source/netwerk/protocol/http/src/nsHttpChannel.cpp#2136

otherwise we are potentially leaking information in the screen captured image that sites take great pains to ensure is not left around on disk for easedroppers to see.   the attack discussed in the security review wasn't a privacy issue, it was a security issue.  if the screen capture fires at a time when a user is looking at bank statement or other https document that the web site intended to *not* persist on disk then we are violating the long established contract with users and web sites.

banks take great pains to expire sessions, log you out, and remove sensitive documents from your screen and panorama would be circumventing that effort in the current design.  these are the kind of things the would get firefox blocked on bank sites.
Comment 37 chris hofmann 2011-04-01 08:44:59 PDT
bug 531801 also is relevant here.  bottom line is that caching control of these images needs to be under the control of the website.
Comment 38 Raymond Lee [:raymondlee] 2011-05-19 02:12:12 PDT
Created attachment 533566 [details] [diff] [review]
WIP

(In reply to comment #36)
> panorama should follow the cache rules found here:
> 
> http://mxr.mozilla.org/firefox/source/netwerk/protocol/http/src/
> nsHttpChannel.cpp#2136
> 

Following the cache rules in the above link.

Will add tests later.
Comment 39 Tim Taubert [:ttaubert] 2011-05-19 02:31:02 PDT
Comment on attachment 533566 [details] [diff] [review]
WIP

Review of attachment 533566 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, that looks pretty good, though I'm not really familiar with all that webProgressListener stuff :)

One thing: I feel like that all shouldn't belong to UI because that is actually only related to thumbnail storage. As soon as we moved away from the SS and used the img cache to store our thumbnails I wished we'd have a "ThumbnailStorage" object that would handle all our thumbnail storage stuff. I don't like how we currently load thumbnails - they could even be loaded asynchronously. This seems like a better place for watching browser progresses to determine whether to save a thumbnail or not. Sorry for the scope creep ;)

Maybe we should add one or more bugs to handle that rather than including it here. But you could actually prepare the ThumbnailStorage object for later use if you concur with me.
Comment 40 Tim Taubert [:ttaubert] 2011-05-19 02:40:39 PDT
(In reply to comment #39)
> Maybe we should add one or more bugs to handle that rather than including it
> here. But you could actually prepare the ThumbnailStorage object for later
> use if you concur with me.

Filed bug 658210.
Comment 41 Raymond Lee [:raymondlee] 2011-05-19 02:43:10 PDT
(In reply to comment #40)
> (In reply to comment #39)
> > Maybe we should add one or more bugs to handle that rather than including it
> > here. But you could actually prepare the ThumbnailStorage object for later
> > use if you concur with me.
> 
> Filed bug 658210.

Agree.  We should have a separate object to do that.
Comment 42 Raymond Lee [:raymondlee] 2011-05-20 10:32:55 PDT
Created attachment 534021 [details] [diff] [review]
v1

Added tests and also moved some code to ThumbnailStorage object.
Comment 43 :Ehsan Akhgari 2011-05-20 12:57:44 PDT
Comment on attachment 534021 [details] [diff] [review]
v1

I probably won't get to review this until next week.  Sorry :(
Comment 44 Tim Taubert [:ttaubert] 2011-05-23 07:14:38 PDT
Comment on attachment 534021 [details] [diff] [review]
v1

Review of attachment 534021 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the new ThumbnailStorage object :) There are lots of nits I tagged below but only because the patch is a bit bigger - I like the overall approach!

F+ with those addressed.

::: browser/base/content/tabview/thumbnailStorage.js
@@ +59,5 @@
> +
> +  // Used to keep track of disk_cache_ssl preference
> +  enablePersistentHttpsCaching: null,
> +
> +  // Used to keep track of browsers and their thumbs should not be saved

Nit: Used to keep track of browsers whose thumbs we shouldn't save

@@ +60,5 @@
> +  // Used to keep track of disk_cache_ssl preference
> +  enablePersistentHttpsCaching: null,
> +
> +  // Used to keep track of browsers and their thumbs should not be saved
> +  inhibitPersistentThumbBrowsers: [],

That name is pretty hard to understand. How about something like "excludedBrowsers" or "browsersToExclude"?

@@ +64,5 @@
> +  inhibitPersistentThumbBrowsers: [],
> +
> +  // ----------
> +  // Function: toString
> +  // Prints [Storage] for debug use.

Nit: [Storage] -> [ThumbnailStorage]

@@ +65,5 @@
> +
> +  // ----------
> +  // Function: toString
> +  // Prints [Storage] for debug use.
> +  toString: function Storage_toString() {

Nit: Storage_toString -> ThumbnailStorage_toString

@@ +88,5 @@
> +      "init");
> +
> +    // store the preference value
> +    this.enablePersistentHttpsCaching =
> +      Services.prefs.getBoolPref("browser.cache.disk_cache_ssl");

Nit: Can we add a pseudo-constant for this? Like "ThumbnailStorage.PREF_DISK_CACHE_SSL"

@@ +90,5 @@
> +    // store the preference value
> +    this.enablePersistentHttpsCaching =
> +      Services.prefs.getBoolPref("browser.cache.disk_cache_ssl");
> +
> +    Services.prefs.addObserver("browser.cache.disk_cache_ssl", this, false);

Nit: pseudo-constant (see above).

@@ +118,5 @@
> +  // Function: uninit
> +  // Should be called when window is unloaded.
> +  uninit: function ThumbnailStorage_uninit() {
> +    gBrowser.removeTabsProgressListener(TabsProgressListener);
> +    Services.prefs.removeObserver("browser.cache.disk_cache_ssl", this);

Nit: pseudo-constant (see above).

@@ +141,5 @@
> +
> +    // switch to synchronous mode if parent window is about to close
> +    if (UI.isDOMWindowClosing) {
> +      let entry = this._cacheSession.openCacheEntry(key, access, true);
> +      let status = Components.results.NS_OK;

Nit: Cr.NS_OK

@@ +160,5 @@
> +  // Function: saveThumbnail
> +  // Saves the <imageData> to the cache using the given <url> as key.
> +  // Calls <callback>(status, data) when finished, passing true or false
> +  // (indicating whether the operation succeeded).
> +  saveThumbnail: function ThumbnailStorage_saveThumbnail(tab, url, imageData, callback) {

I get that we now need the tab argument but do we still need the url? We can determine it from the tab, right?

@@ +165,5 @@
> +    Utils.assert(tab, "tab");
> +    Utils.assert(url, "url");
> +    Utils.assert(imageData, "imageData");
> +    
> +    if (!this._shouldSaveThumbnail(tab)) {

Please make sure to call the callback with "callback(false);"

@@ +166,5 @@
> +    Utils.assert(url, "url");
> +    Utils.assert(imageData, "imageData");
> +    
> +    if (!this._shouldSaveThumbnail(tab)) {
> +      tab._tabViewTabItem._sendToSubscribers("inhibitToSaveCachedImageData");

Can we call this "deniedToCacheImageData" or something similar?

@@ +222,5 @@
> +  // Function: loadThumbnail
> +  // Asynchrously loads image data from the cache using the given <url> as key.
> +  // Calls <callback>(status, data) when finished, passing true or false
> +  // (indicating whether the operation succeeded) and the retrieved image data.
> +  loadThumbnail: function ThumbnailStorage_loadThumbnail(tab, url, callback) {

(url argument - see above)

@@ +290,5 @@
> +  // ----------
> +  // Function: observe
> +  // Implements the observer interface.
> +  observe: function ThumbnailStorage_observe(subject, topic, data) {
> +    if ("nsPref:changed") {

This would be always true :) I think we can remove these extra checks as long as we are observing only one topic.

@@ +293,5 @@
> +  observe: function ThumbnailStorage_observe(subject, topic, data) {
> +    if ("nsPref:changed") {
> +      if (data == "browser.cache.disk_cache_ssl") {
> +        this.enablePersistentHttpsCaching =
> +          Services.prefs.getBoolPref("browser.cache.disk_cache_ssl");

Nit: pseudo-constant (see above).

@@ +300,5 @@
> +  }
> +}
> +
> +// ##########
> +// Class: TabsProgressListener

Do we really need this as an extra object? We could just incorporate this in ThumbnailStorage just like the observe() method.

@@ +304,5 @@
> +// Class: TabsProgressListener
> +// Generic tabs prgress listeer for preventing thumbs to be saved based on the 
> +// cache-control value sent by the site
> +let TabsProgressListener = {
> +  onStateChange: function(browser, webProgress, request, flag, status) {

Nit: "function TabsProgressListener_onStateChange(browser ..."

@@ +330,5 @@
> +                     request.URI.schemeIs("https")) {
> +            let cacheControlHeader;
> +            try {
> +              cacheControlHeader = request.getResponseHeader("Cache-Control");
> +            } catch(e) { }

Is it really necessary to nest these try-catch blocks here?

@@ +337,5 @@
> +          }
> +
> +          if (inhibitPersistentThumb &&
> +              ThumbnailStorage.inhibitPersistentThumbBrowsers.indexOf(browser) == -1)
> +            ThumbnailStorage.inhibitPersistentThumbBrowsers.push(browser);

I don't think this should be contained in the try-catch block. We should keep the amount of code contained in the try-catch blocks as small as possible because there's a lot of code that I think wouldn't throw.

@@ +343,5 @@
> +      }
> +    }
> +  },
> +
> +  QueryInterface: function(iid) {

Let's use XPCOMUtils.generateQI() here (see below - CacheListener)

@@ +371,5 @@
> +    return "[CacheListener]";
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsICacheListener]),
> +  onCacheEntryAvailable: function (entry, access, status) {

Nit: Let's make that "function CacheListener_onCacheEntryAvailable(entry, access, status) {"

::: browser/base/content/test/tabview/browser_tabview_bug627239.js
@@ +33,5 @@
> +  HttpRequestObserver.cacheControlValue = "no-store";
> +  newTab.linkedBrowser.loadURI("http://www.example.com/browser/browser/base/content/test/tabview/dummy_page.html");
> +
> +  afterAllTabsLoaded(function() {
> +    executeSoon(function() {

We don't need executeSoon() here. You can just add that in head.js in the onLoad handler from afterAllTabsLoaded() as we need it there anyway.

@@ +55,5 @@
> +  HttpRequestObserver.cacheControlValue = "private";
> +
> +  newTab.linkedBrowser.loadURI("http://www.example.com/");
> +  afterAllTabsLoaded(function() {
> +    executeSoon(function() {

(see above)

@@ +80,5 @@
> +  contentWindow.ThumbnailStorage.enablePersistentHttpsCaching = true;
> +
> +  newTab.linkedBrowser.loadURI("https://example.com/browser/browser/base/content/test/tabview/dummy_page.html");
> +  afterAllTabsLoaded(function() {
> +    executeSoon(function() {

(see above)

@@ +104,5 @@
> +  contentWindow.ThumbnailStorage.enablePersistentHttpsCaching = false;
> +
> +  newTab.linkedBrowser.loadURI("https://example.com/browser/browser/base/content/test/tabview/");
> +  afterAllTabsLoaded(function() {
> +    executeSoon(function() {

(see above)

@@ +121,5 @@
> +  });
> +}
> +
> +function test5() {
> +  // page with cache-control: private with https caching disabled, should save thumbnail

Wrong description -> should _not_ save the thumbnail.

@@ +126,5 @@
> +  HttpRequestObserver.cacheControlValue = "private";
> + 
> +  newTab.linkedBrowser.loadURI("https://example.com/");
> +  afterAllTabsLoaded(function() {
> +    executeSoon(function() {

(see above)
Comment 45 Raymond Lee [:raymondlee] 2011-05-23 23:01:10 PDT
Created attachment 534683 [details] [diff] [review]
v2

(In reply to comment #44)

> @@ +160,5 @@
> > +  // Function: saveThumbnail
> > +  // Saves the <imageData> to the cache using the given <url> as key.
> > +  // Calls <callback>(status, data) when finished, passing true or false
> > +  // (indicating whether the operation succeeded).
> > +  saveThumbnail: function ThumbnailStorage_saveThumbnail(tab, url, imageData, callback) {
> 
> I get that we now need the tab argument but do we still need the url? We can
> determine it from the tab, right?

Yes, removed the url arg.

> 
>
> @@ +222,5 @@
> > +  // Function: loadThumbnail
> > +  // Asynchrously loads image data from the cache using the given <url> as key.
> > +  // Calls <callback>(status, data) when finished, passing true or false
> > +  // (indicating whether the operation succeeded) and the retrieved image data.
> > +  loadThumbnail: function ThumbnailStorage_loadThumbnail(tab, url, callback) {
> 
> (url argument - see above)
> 

Needs to keep the url because the url might not be fully loaded into the tab.


Addressed other issues.
Comment 46 Raymond Lee [:raymondlee] 2011-05-23 23:10:48 PDT
Created attachment 534684 [details] [diff] [review]
v2

Some minor changes
Comment 47 Tim Taubert [:ttaubert] 2011-05-27 02:23:46 PDT
bugspam
Comment 48 Tim Taubert [:ttaubert] 2011-05-27 02:28:44 PDT
bugspam
Comment 49 :Ehsan Akhgari 2011-05-30 10:44:35 PDT
Comment on attachment 534684 [details] [diff] [review]
v2

Review of attachment 534684 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/storage.js
@@ +142,2 @@
>            callback(imageData);
> +        });

Nit: please fix the indentation.

::: browser/base/content/tabview/ui.js
@@ +685,5 @@
>      // fact that we were there so we can return after PB) and make sure we
>      // don't reenter Panorama due to all of the session restore tab
>      // manipulation (which otherwise we might). When transitioning away from
>      // PB, we reenter Panorama if we had been there directly before PB.
> +    function pbObserver(subject, topic, data) {

Please don't change the variable names needlessly.
Comment 50 Raymond Lee [:raymondlee] 2011-05-30 12:58:45 PDT
(In reply to comment #49)
> Comment on attachment 534684 [details] [diff] [review] [review]
> v2
> 
> Review of attachment 534684 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabview/storage.js
> @@ +142,2 @@
> >            callback(imageData);
> > +        });
> 
> Nit: please fix the indentation.
> 
> ::: browser/base/content/tabview/ui.js
> @@ +685,5 @@
> >      // fact that we were there so we can return after PB) and make sure we
> >      // don't reenter Panorama due to all of the session restore tab
> >      // manipulation (which otherwise we might). When transitioning away from
> >      // PB, we reenter Panorama if we had been there directly before PB.
> > -    function pbObserver(aSubject, aTopic, aData) {
> > -      if (aTopic == "private-browsing") {
> > +    function pbObserver(subject, topic, data) {
>
> Please don't change the variable names needlessly.

The only reason that I changed the variable names was that they don't match the style of other code in Panorama. Is it ok to change them in this patch?
Comment 51 Raymond Lee [:raymondlee] 2011-05-30 23:52:36 PDT
Created attachment 536240 [details] [diff] [review]
Patch for checkin
Comment 52 Raymond Lee [:raymondlee] 2011-05-30 23:53:09 PDT
(In reply to comment #51)
> Created attachment 536240 [details] [diff] [review] [review]
> Patch for checkin

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=13cea4749b4f
Comment 53 Tim Taubert [:ttaubert] 2011-05-31 13:08:04 PDT
http://hg.mozilla.org/mozilla-central/rev/de06b4b383bf
Comment 54 Virgil Dicu [:virgil] [QA] 2011-08-31 06:57:51 PDT
Checked that the tests are passed in 
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314785888.1314787127.20913.gz&fulltext=1

Are there any other guidelines which I can use before verifying this issue?
Comment 55 Tim Taubert [:ttaubert] 2011-09-08 05:31:43 PDT
(In reply to Virgil Dicu [QA] from comment #54)
> Are there any other guidelines which I can use before verifying this issue?

While trying to collect some steps to verify I discovered that there are some issues with the current approach. We'd need to wait until bug 685104 is fixed until we can verify these both.
Comment 56 henryfhchan 2012-08-15 22:36:47 PDT
Can this 'feature' be preffed off?
This causes many user sites, e.g. Gmail and Facebook and Twitter to be always blank.

This is a bad user experience really.

Note You need to log in before you can comment on or make changes to this bug.