Closed Bug 1088203 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: New Tab Page, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.1 - 26 Jan
Tracking Status
e10s m5+ ---

People

(Reporter: ehsan.akhgari, Assigned: jimm)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

Attached image Screenshot
This just happened on the latest Nightly.

This is on an e10s browser in case that matters.
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+
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+
See Also: → 1037673
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
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
Closed: 8 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 → ---
bug 1073957, which will fix this, blocks m5.
OS: Mac OS X → All
No longer depends on: 1073957
Duplicate of this bug: 1073957
Blocks: 1062414
Attached patch wip (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Attachment #8551988 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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-
Attached patch patch (obsolete) — Splinter Review
Attachment #8552341 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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+
I'll go with 'shouldStoreThumbnail'
Attached patch patch r=daoSplinter Review
Attachment #8552395 - Attachment is obsolete: true
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
(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
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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
Depends on: 1245118
You need to log in before you can comment on or make changes to this bug.