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

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

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 bugmail, email directly)]

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]]

(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 bugmail, email directly)]

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]

> 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]

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)]

>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]]

Attachment: Patch v1

With test. Pushed to try.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1, try 2

I fail at patch-making.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

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]]

Test passed try.

Comment 11

[Ian Gilman [:iangilman]]

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

[(no longer active)]

Hmm, dump calls in tests are safe (albeit noisy)...

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1.1

Thanks Ian!

Comment 14

[Ian Gilman [:iangilman]]

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 bugmail, email directly)]

(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]]

(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]]

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]]

This isn't a blocker, but I guess we'd take the approved patch!

Comment 19

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 20

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/97e4df251673

Comment 21

[Jonathan Watt [:jwatt]]

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]]

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]]

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]

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]]

(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] (UNAVAILABLE)]

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]]

Yet another option is to just not store thumbnails, as suggested in bug 604699.

Comment 28

[Michael Yoshitaka Erlewine [:mitcho]]

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]]

(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]]

(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]]

[bugspam: mitcho's late-night betaN triage; feel free to comment or reverse]

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 506847 [details] [diff] [review]
Patch v1.1

(removing approval for backed out patch)

Comment 33

[Kevin Hanes]

bugspam

Comment 34

[Raymond Lee [:raymondlee]]

We are using the cache service and we still don't want to store thumbnails for https pages, right?

Comment 35

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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]

> 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]

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]]

Attachment: 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] (inactive)]

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] (inactive)]

(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]]

(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]]

Attachment: v1

Added tests and also moved some code to ThumbnailStorage object.

Comment 43

[(no longer active)]

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] (inactive)]

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]]

Attachment: 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]]

Attachment: v2

Some minor changes

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 49

[(no longer active)]

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]]

(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]]

Attachment: Patch for checkin

Comment 52

[Raymond Lee [:raymondlee]]

(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] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/de06b4b383bf

Comment 54

[Virgil Dicu [:virgil] [QA]]

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] (inactive)]

(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]

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.