The default bug view has changed. See this FAQ.

Don't store thumbnails for cache:control:no-store pages

RESOLVED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
P1
major
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: mitcho, Assigned: raymondlee)

Tracking

({privacy})

Trunk
Firefox 7
privacy
Dependency tree / graph

Details

(Whiteboard: [sg:low local])

Attachments

(1 attachment, 8 obsolete attachments)

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.
(Reporter)

Updated

6 years ago
Depends on: 625217
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.
(Reporter)

Comment 2

6 years ago
(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?

Updated

6 years ago
blocking2.0: --- → ?
Keywords: privacy
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!)
Keywords: uiwanted
Summary: Storing thumbnail data should respect sessionstore privacy level → Don't store thumbnails for https pages
(Assignee)

Updated

6 years ago
Severity: normal → major

Comment 4

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

6 years ago
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 |
  |                 |    |                  |
  |                 |    |                  |
  |                 |    |                  |
   __________________     ___________________
Blocks: 627096
>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.
(Reporter)

Updated

6 years ago
Keywords: uiwanted
(Reporter)

Updated

6 years ago
Assignee: nobody → mitcho
(Reporter)

Comment 7

6 years ago
Created attachment 506240 [details] [diff] [review]
Patch v1

With test. Pushed to try.
Attachment #506240 - Flags: review?(ian)
Attachment #506240 - Flags: feedback?(raymond)
(Reporter)

Comment 8

6 years ago
Created attachment 506241 [details] [diff] [review]
Patch v1, try 2

I fail at patch-making.
Attachment #506240 - Attachment is obsolete: true
Attachment #506241 - Flags: review?(ian)
Attachment #506241 - Flags: feedback?(raymond)
Attachment #506240 - Flags: review?(ian)
Attachment #506240 - Flags: feedback?(raymond)
(Assignee)

Updated

6 years ago
Attachment #506241 - Flags: feedback?(raymond) → feedback+
(Reporter)

Comment 9

6 years ago
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. :/
Status: NEW → ASSIGNED
Test passed try.
Comment on attachment 506241 [details] [diff] [review]
Patch v1, try 2

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

Kill this dump call. 

Otherwise looks great.
Attachment #506241 - Flags: review?(ian) → review+
Hmm, dump calls in tests are safe (albeit noisy)...
Created attachment 506847 [details] [diff] [review]
Patch v1.1

Thanks Ian!
Attachment #506241 - Attachment is obsolete: true
Attachment #506847 - Flags: approval2.0?
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?
Attachment #506847 - Flags: approval2.0? → review-
(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.
(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 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!
Attachment #506847 - Flags: review-
Attachment #506847 - Flags: review+
Attachment #506847 - Flags: approval2.0?
Attachment #506847 - Flags: approval2.0? → approval2.0+
This isn't a blocker, but I guess we'd take the approved patch!
blocking2.0: ? → -
Created attachment 507359 [details] [diff] [review]
Patch for checkin
Attachment #506847 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
Pushed http://hg.mozilla.org/mozilla-central/rev/97e4df251673
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: checkin-needed
Target Milestone: --- → Firefox 4.0b11
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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

6 years ago
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.
(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.
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).
Yet another option is to just not store thumbnails, as suggested in bug 604699.
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! :)
Whiteboard: [security] → [security][needs spec][options presented in comment 28]
(Reporter)

Updated

6 years ago
Attachment #507359 - Attachment is obsolete: true
(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?
(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.
[bugspam: mitcho's late-night betaN triage; feel free to comment or reverse]
Blocks: 603789
No longer blocks: 627096
(Reporter)

Updated

6 years ago
Target Milestone: Firefox 4.0b11 → Future
Comment on attachment 506847 [details] [diff] [review]
Patch v1.1

(removing approval for backed out patch)
Attachment #506847 - Flags: approval2.0+
(Reporter)

Updated

6 years ago
Assignee: mitcho → nobody

Updated

6 years ago
Whiteboard: [security][needs spec][options presented in comment 28] → [security][needs spec][options presented in comment 28][not-ready]

Comment 33

6 years ago
bugspam
Target Milestone: Future → ---
(Assignee)

Comment 34

6 years ago
We are using the cache service and we still don't want to store thumbnails for https pages, right?
(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

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

6 years ago
bug 531801 also is relevant here.  bottom line is that caching control of these images needs to be under the control of the website.
No longer blocks: 603789
Blocks: 653099
(Assignee)

Comment 38

6 years ago
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.
Assignee: nobody → raymond
Status: REOPENED → ASSIGNED
Attachment #533566 - Flags: feedback?(tim.taubert)
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.
Attachment #533566 - Flags: feedback?(tim.taubert) → feedback+
(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.
(Assignee)

Comment 41

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #533566 - Flags: feedback+ → feedback?(ehsan)
(Assignee)

Updated

6 years ago
Blocks: 658210
(Assignee)

Comment 42

6 years ago
Created attachment 534021 [details] [diff] [review]
v1

Added tests and also moved some code to ThumbnailStorage object.
Attachment #533566 - Attachment is obsolete: true
Attachment #534021 - Flags: feedback?(tim.taubert)
Attachment #534021 - Flags: feedback?(ehsan)
Attachment #533566 - Flags: feedback?(ehsan)
Comment on attachment 534021 [details] [diff] [review]
v1

I probably won't get to review this until next week.  Sorry :(
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)
Attachment #534021 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 45

6 years ago
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.
Attachment #534021 - Attachment is obsolete: true
Attachment #534683 - Flags: review?(ehsan)
Attachment #534021 - Flags: feedback?(ehsan)
(Assignee)

Comment 46

6 years ago
Created attachment 534684 [details] [diff] [review]
v2

Some minor changes
Attachment #534683 - Attachment is obsolete: true
Attachment #534684 - Flags: review?(ehsan)
Attachment #534683 - Flags: review?(ehsan)
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
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.
Attachment #534684 - Flags: review?(ehsan) → review+
(Assignee)

Comment 50

6 years ago
(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?
(Assignee)

Comment 51

6 years ago
Created attachment 536240 [details] [diff] [review]
Patch for checkin
Attachment #534684 - Attachment is obsolete: true
(Assignee)

Comment 52

6 years ago
(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
http://hg.mozilla.org/mozilla-central/rev/de06b4b383bf
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [security][needs spec][options presented in comment 28][not-ready] → [security]
Target Milestone: --- → Firefox 7
Version: unspecified → Trunk

Updated

6 years ago
Summary: Don't store thumbnails for https pages → Don't store thumbnails for cache:control:no-store pages
Whiteboard: [security] → [sg:low local]
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?
Depends on: 685104
(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

5 years ago
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.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.