1088203 - New tab screenshots showing my logged in sites (perhaps e10s related)

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[(no longer active)]

Attachment: Screenshot

This just happened on the latest Nightly.

This is on an e10s browser in case that matters.

Comment 1

[(no longer active)]

Just noticed that my youtube screenshot (not visible in the attachment) is also from a logged in youtube.)

Comment 3

[(no longer active)]

And now this is happening on Facebook (which I visited yesterday).  I doubt that there is anything site specific here.

Comment 4

[Jim Mathies [:jimm]]

Do we have a set of rules we follow here? For example, do we have a rule that we won't thumbnail an ssl page?  I'm trying to understand how this works in non-e10s mode so I can figure out where the bug is.

Also, STR would be helpful.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

(In reply to Jim Mathies [:jimm] from comment #4)
> Do we have a set of rules we follow here? For example, do we have a rule
> that we won't thumbnail an ssl page?  I'm trying to understand how this
> works in non-e10s mode so I can figure out where the bug is.

Yeah, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-thumbnails.js#119

Comment 6

[Jim Mathies [:jimm]]

Thanks felipe.. turns out this is a dupe - 

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-thumbnails.js#119

Comment 7

[:Gavin Sharp [email: [e-mail]]]

We should track things like this separately - it's worth explicitly validating that the fix for bug 1073957 addresses this particular symptom.

Comment 8

[Jim Mathies [:jimm]]

bug 1073957, which will fix this, blocks m5.

Comment 10

[Jim Mathies [:jimm]]

Attachment: wip

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8552331 [details] [diff] [review]
patch

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cccd0f199e2

Comment 13

[Jim Mathies [:jimm]]

Attachment: patch

fixes a bug in ctrlTab code where the canvas didn't update - now I paint the image we load to the canvas we originally handed back and update the cache.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8552341 [details] [diff] [review]
patch

>+    PageThumbs.isSafeForCapture(browser, (aSafeSite) => {
>+      if (aSafeSite && aShouldCache) {
>+        PageThumbs.captureAndStore(browser, function () {
>+          let img = new Image;
>+          img.src = PageThumbs.getThumbnailURL(uri);
>+          aTab.__thumbnail = img;
>+          aTab.__thumbnail_lastURI = uri;
>+          canvas.getContext('2d').drawImage(img, 0, 0);
>+        });
>+      } else {
>+        PageThumbs.captureToCanvas(browser, canvas, () => {
>+          if (aShouldCache) {
>+            aTab.__thumbnail = canvas;
>+            aTab.__thumbnail_lastURI = uri;
>+          }
>+        });
>+      }
>+    });

Calling captureToCanvas when the answer to "is it safe to capture" was "no" doesn't seem to make sense. Can you rename isSafeForCapture to something more accurate? mayStore, shouldPersist, ...?

>+    // Don't capture thumbnails for SVG or XML documents (bug 720575)
>+    if (aDocument instanceof Ci.nsIDOMXMLDocument) {
>+      return false;
>+    }

The original comment mentioned a performance problem. Can you please bring that back? Otherwise it's unclear why we're doing this. Bug 720575 doesn't directly explain this either.

>+  /**
>+   * Asynchronously check the state of aBrowser to see if it passes a set of
>+   * predefined security checks. Consumers should generally refrain from
>+   * snapping, displaying, or storing thumbnails if these checks fail.

This seems inaccurate again. This is only about storing, snapping and displaying shouldn't be a problem.

Comment 15

[Jim Mathies [:jimm]]

Attachment: patch

Comment 16

[Jim Mathies [:jimm]]

Attachment: patch

updated per comments

Comment 17

[Jim Mathies [:jimm]]

mochitests - 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a425959e511c

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8552395 [details] [diff] [review]
patch

>+    PageThumbs.canPersistThumbnail(browser, (aSafeSite) => {

I'd prefer "may" or "should" over "can", because nothing prevents us from storing the thumbnail regardless, as far as I can see.

Since the other method is called captureAndStore, it might be better to say "store" rather than "persist" too.

I also dislike the aSafeSite argument name. This could just mirror the method name, e.g. aCanPersist based on your current method name choice.

>+          canvas.getContext('2d').drawImage(img, 0, 0);

nit: double quotes

Comment 19

[Jim Mathies [:jimm]]

I'll go with 'shouldStoreThumbnail'

Comment 20

[Jim Mathies [:jimm]]

Attachment: patch r=dao

Comment 21

[Dão Gottwald [:dao]]

Looks like this was missed:

(In reply to Dão Gottwald [:dao] from comment #18)
> I also dislike the aSafeSite argument name. This could just mirror the
> method name, e.g. aCanPersist based on your current method name choice.

This was mostly a nit, not an "r- if you don't address this" kind of comment, but even if you chose to ignore it, a brief note on that would be appreciated.

Comment 22

[Jim Mathies [:jimm]]

(In reply to Dão Gottwald [:dao] from comment #21)
> Looks like this was missed:
> 
> (In reply to Dão Gottwald [:dao] from comment #18)
> > I also dislike the aSafeSite argument name. This could just mirror the
> > method name, e.g. aCanPersist based on your current method name choice.
> 
> This was mostly a nit, not an "r- if you don't address this" kind of
> comment, but even if you chose to ignore it, a brief note on that would be
> appreciated.

sorry, just forgot to update.

Comment 23

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e97e6bdedd8

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0e97e6bdedd8

Comment 25

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Firefox/38.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Firefox/38.0

Reproduced the initial issue on 2014-10-24 Nightly.

Verified as fixed on Latest Nightly, build ID: 20150126030202.
The youtube and facebook tiles no longer display the "logged in" state.

Tested on Windows 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.9.5.