1387580 - Some top sites never get screenshots of redirected pages

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Eric Rescorla (:ekr)]

Attachment: image(2).png

The rest of the tiles render properly

Comment 1

[Ed Lee :Mardak]

Attachment: HN and strava for Mardak

Hmm... seems to work for me. Are you logged in to either? (Although I believe screenshots happen without cookies. ursula?)

Comment 2

[Eric Rescorla (:ekr)]

Strava yes, HN no.

LMK if there's any more data you would like me to gather.

Comment 3

[Ed Lee :Mardak]

I'll make this more general. In my main profile, Feedly (logged in) just shows the fallback "F" even after restarts. But a clean profile gets a screenshot for feedly just fine.

Comment 4

[Ed Lee :Mardak]

ekr, could you share the urls of those sites missing screenshots? I think it might be something with redirects

E.g., my missing screenshot Feedly url is http://www.feedly.com/i/latest but that redirects to https://feedly.com/i/welcome without cookies

Comment 5

[Eric Rescorla (:ekr)]

I right-clicked copy link location, and got:

https://www.strava.com/dashboard
http://www.news.ycombinator.com/

Comment 6

[Ed Lee :Mardak]

And those suspiciously do redirect to..

https://www.strava.com/login

Actually that HN link doesn't even work for me. Charter steals it:

$ dig www.news.ycombinator.com
www.news.ycombinator.com. 10	IN	A	198.105.244.24

$ ping search.charter.net
PING search.charter.net (198.105.244.24)

If I set DNS to something else (8.8.8.8), Firefox says
Firefox can’t find the server at www.news.ycombinator.com.

Comment 7

[Ed Lee :Mardak]

Attachment: initial fixes

Here's activity stream with a few fixes when running

> ./mach run --setpref browser.newtabpage.pinned='[{"url":"https://www.mozilla.org/"},{"url":"https://www.strava.com/dashboard"},{"url":"http://www.feedly.com/i/latest"},{"url":"https://www.mozilla.com/"}]'

where those urls redirect to:
https://www.mozilla.org/en-US/
https://www.strava.com/login
https://feedly.com/i/welcome
https://www.mozilla.org/en-US/firefox/

It has a fix for not losing the Promise to a GC https://bugzilla.mozilla.org/show_bug.cgi?id=1387682

which gets mozilla.org and mozilla.com to show a screenshot without needing a restart (thumbnail is created but the Promise got eaten by GC)

And a fix for correctly converting over 500k bytes https://github.com/mozilla/activity-stream/pull/3101

which gets strava to work because it's screenshot turns out to be quite large.

ekr, I filed a separate issue for the HN preferring non-existent www. https://github.com/mozilla/activity-stream/issues/3102

Comment 8

[Ed Lee :Mardak]

Attachment: video with GC fixes without redirect fixes (5x speed)

This is running with a clean profile as well as turning off "tippy top" packaged icons for the default sites.

> ./mach run --setpref browser.newtabpage.pinned='[{"url":"https://www.mozilla.org/"},{"url":"https://www.strava.com/dashboard"},{"url":"https://mzl.la/"},{"url":"http://www.feedly.com/i/latest"},{"url":"https://www.mozilla.com/"},{"url":"https://mint.intuit.com/overview.event"}]'

Some thumbnails load faster as it captures right away but it gets confused with feedly redirect and captures a blank page for mint.

Comment 9

[Ed Lee :Mardak]

Attachment: video with GC and redirect fixes (5x speed)

Same process as the previous attachment #8894056 [details] except with the redirect fix where it waits 2.5sec for the page to settle before capturing. Interestingly the total time of both videos are ~10sec.

Comment 10

[Ed Lee :Mardak]

Attachment: Bug 1387580 - Some top sites never get screenshots of redirected pages.

Review commit: https://reviewboard.mozilla.org/r/165154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165154/

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 8894058 [details]
Bug 1387580 - Some top sites never get screenshots of redirected pages.

https://reviewboard.mozilla.org/r/165154/#review170960

Interesting, LGTM

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1a7a562b59a
Some top sites never get screenshots of redirected pages. r=adw

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1a7a562b59a

Comment 14

[Ed Lee :Mardak]

(In reply to Eric Rescorla (:ekr) from comment #5)
> https://www.strava.com/dashboard
The problem here was strava screenshot had too many bytes and .apply had too many args.
Fixed by https://github.com/mozilla/activity-stream/pull/3101 to be uplifted via bug 1387746.

> http://www.news.ycombinator.com/
The problem here was a SQL query doing GROUP BY with undefined sqlite ordering behavior.
Fixed by bug 1387694.

Comment 15

[Marius Coman [:mcoman], Ecosystem QA]

Screenshots of redirected pages are displayed in the Top Sites tile.
Verified as fixed using latest Nightly 57.0a1 Build ID 20170917220255, on Windows 10 x64 and Mac 10.12.