Closed
Bug 1088203
Opened 10 years ago
Closed 10 years ago
New tab screenshots showing my logged in sites (perhaps e10s related)
Categories
(Firefox :: New Tab Page, defect)
Tracking
()
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)
14.18 KB,
patch
|
Details | Diff | Splinter Review |
This just happened on the latest Nightly.
This is on an e10s browser in case that matters.
Reporter | ||
Comment 1•10 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)
Updated•10 years ago
|
Assignee: nobody → jmathies
tracking-e10s:
--- → m4+
Reporter | ||
Comment 3•10 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)
Updated•10 years ago
|
Flags: qe-verify+
Flags: firefox-backlog+
![]() |
Assignee | |
Comment 4•10 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.
Comment 5•10 years ago
|
||
(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•10 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
Closed: 10 years ago
Resolution: --- → DUPLICATE
Comment 7•10 years ago
|
||
We should track things like this separately - it's worth explicitly validating that the fix for bug 1073957 addresses this particular symptom.
![]() |
Assignee | |
Updated•10 years ago
|
OS: Mac OS X → All
![]() |
Assignee | |
Comment 10•10 years ago
|
||
![]() |
Assignee | |
Comment 11•10 years ago
|
||
Attachment #8551988 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 12•10 years ago
|
||
Comment on attachment 8552331 [details] [diff] [review]
patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cccd0f199e2
Attachment #8552331 -
Flags: review?(dao)
![]() |
Assignee | |
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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•10 years ago
|
||
Attachment #8552341 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 16•10 years ago
|
||
updated per comments
Attachment #8552389 -
Attachment is obsolete: true
Attachment #8552395 -
Flags: review?(dao)
![]() |
Assignee | |
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
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•10 years ago
|
||
I'll go with 'shouldStoreThumbnail'
![]() |
Assignee | |
Comment 20•10 years ago
|
||
Attachment #8552395 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
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•10 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.
![]() |
Assignee | |
Comment 23•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Updated•10 years ago
|
Iteration: --- → 38.1 - 26 Jan
Updated•10 years ago
|
QA Contact: cornel.ionce
Comment 25•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•