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

VERIFIED FIXED in Firefox 38

Status

()

VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: jimm)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {regression})

unspecified
Firefox 38
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(e10sm5+)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8510503 [details]
Screenshot

This just happened on the latest Nightly.

This is on an e10s browser in case that matters.
(Reporter)

Comment 1

4 years ago
Just noticed that my youtube screenshot (not visible in the attachment) is also from a logged in youtube.)
Summary: New tab screenshots showing my gmail emails (perhaps e10s related) → New tab screenshots showing my gmail emails and logged in youtube (perhaps e10s related)
Assignee: nobody → jmathies
tracking-e10s: --- → m4+
(Reporter)

Comment 3

4 years ago
And now this is happening on Facebook (which I visited yesterday).  I doubt that there is anything site specific here.
Summary: New tab screenshots showing my gmail emails and logged in youtube (perhaps e10s related) → New tab screenshots showing my logged in sites (perhaps e10s related)
Flags: qe-verify+
Flags: firefox-backlog+

Updated

4 years ago
See Also: → bug 1037673
(Assignee)

Comment 4

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

Comment 6

4 years ago
Thanks felipe.. turns out this is a dupe - 

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-thumbnails.js#119
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1073957
We should track things like this separately - it's worth explicitly validating that the fix for bug 1073957 addresses this particular symptom.
Status: RESOLVED → REOPENED
Depends on: 1073957
Resolution: DUPLICATE → ---
(Assignee)

Comment 8

4 years ago
bug 1073957, which will fix this, blocks m5.
tracking-e10s: m4+ → m5+
(Assignee)

Updated

4 years ago
OS: Mac OS X → All
(Assignee)

Updated

4 years ago
No longer depends on: 1073957
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1073957
(Assignee)

Updated

4 years ago
Blocks: 1062414
(Assignee)

Comment 11

4 years ago
Created attachment 8552331 [details] [diff] [review]
patch
Attachment #8551988 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8552341 [details] [diff] [review]
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.
Attachment #8552331 - Attachment is obsolete: true
Attachment #8552331 - Flags: review?(dao)
Attachment #8552341 - Flags: review?(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.
Attachment #8552341 - Flags: review?(dao) → review-
(Assignee)

Comment 15

4 years ago
Created attachment 8552389 [details] [diff] [review]
patch
Attachment #8552341 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 8552395 [details] [diff] [review]
patch

updated per comments
Attachment #8552389 - Attachment is obsolete: true
Attachment #8552395 - Flags: review?(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
Attachment #8552395 - Flags: review?(dao) → review+
(Assignee)

Comment 19

4 years ago
I'll go with 'shouldStoreThumbnail'
(Assignee)

Comment 20

4 years ago
Created attachment 8552440 [details] [diff] [review]
patch r=dao
Attachment #8552395 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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.
Keywords: checkin-needed
(Assignee)

Comment 22

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/0e97e6bdedd8
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38

Updated

4 years ago
Iteration: --- → 38.1 - 26 Jan
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED

Updated

2 years ago
Depends on: 1245118
You need to log in before you can comment on or make changes to this bug.