1413650 - Capture loaded page instead of relying on background screenshot

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement)

Description

[Ursula Sarracini (:ursula)]

If we use the loaded logged-in browser of a tab to capture the screenshot, it would avoid the cookie-less screenshots from the background thumbnailer.

Could help with bug 1410920

Comment 1

[Ursula Sarracini (:ursula)]

Attachment: Bug 1413650 - Capture loaded page instead of relying on background screenshot

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

Comment 2

[Ursula Sarracini (:ursula)]

Attachment: patch_applied.png

Captured screenshot as we visit the page

Comment 3

[Ursula Sarracini (:ursula)]

Attachment: patch_not_applied_pt1.png

Did not capture screenshot of site while being visited - we fetch the screenshot via activity stream

Comment 4

[Ursula Sarracini (:ursula)]

Attachment: patch_not_applied_pt2.png

Captured screenshot via activity stream in the background

Comment 5

[Ursula Sarracini (:ursula)]

The patch applied does 2 things:
1. Fixes a bug in the way we cache the top sites for activity stream in browser-thumbnails.js by expiring them every minute. Originally it would only expire them when you close the browser, which means that even though you might be getting new top sites as you browse, we aren't collecting their screenshots live until then next time it needs to get the top sites.
2. Only collects screenshots for top sites we know need a screenshot - meaning the icon that we collect for that page is less than 96x96 

Visiting https://www.britishairways.com/en-gb/executive-club/spending-avios/reward-flights with and without the patch shows the difference....
I've attached screenshots of activity stream with the british airways site as a top site, both with and without the patch applied to show the difference

Comment 6

[Ed Lee :Mardak]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

https://reviewboard.mozilla.org/r/195476/#review201138

Let's also remove the expiration filter stuff and add in pinned links.

::: browser/base/content/browser-thumbnails.js:148
(Diff revision 1)
>      // Only capture about:newtab top sites.
>      const topSites = await this._topSiteURLs;
> -    if (!aBrowser.currentURI ||
> -        topSites.indexOf(aBrowser.currentURI.spec) == -1)
> +    const index = topSites.map(l => l.url).indexOf(aBrowser.currentURI.spec);
> +
> +    // if this is not a top site, do not collect a screenshot
> +    if (!aBrowser.currentURI || index == -1)

The `currentURI` guard / not-null check is bypassed with `indexOf`'s argument above.

```js
if (!aBrowser.currentURI || (topSites.find(l => l.url === aBrowser.currentURI.spec) || {}).hasRichIcon) {
  return;
```

?

::: browser/base/content/browser-thumbnails.js:153
(Diff revision 1)
> +    if (!aBrowser.currentURI || index == -1)
>        return;
> +
> +    // at this point we know it's a top site, but now we check if it has a rich
> +    // icon already, and if so we don't bother getting a screenshot
> +    if (topSites[index].hasRichIcon)

Just noting that with tippy top icons that are only added in TopSitesFeed, we might unnecessarily capture thumbnails until we are able to get the state from activity stream.

::: browser/base/content/browser-thumbnails.js:222
(Diff revision 1)
> -  if (gBrowserThumbnails._activityStreamEnabled) {
> -    sites = await NewTabUtils.activityStreamLinks.getTopSites();
> -  } else {
> -    // The _topSiteURLs getter can be expensive to run, but its return value can
> +  // The _topSiteURLs getter can be expensive to run, but its return value can
> -    // change frequently on new profiles, so as a compromise we cache its return
> +  // change frequently on new profiles, so as a compromise we cache its return
> -    // value as a lazy getter for 1 minute every time it's called.
> +  // value as a lazy getter for 1 minute every time it's called.

Doing a places query every minute seems a bit overkill for various reasons:
1) common usage probably doesn't have top sites changing frequently
2) even with a really fast query, it's potential unnecessary jank on the main process

Although adding more complexity with waiting for idle / idle-daily might not be worth it?

So the simplest change is to increase the timer, but by how much? Dunno…

Comment 7

[Ursula Sarracini (:ursula)]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

https://reviewboard.mozilla.org/r/195476/#review201138

> Doing a places query every minute seems a bit overkill for various reasons:
> 1) common usage probably doesn't have top sites changing frequently
> 2) even with a really fast query, it's potential unnecessary jank on the main process
> 
> Although adding more complexity with waiting for idle / idle-daily might not be worth it?
> 
> So the simplest change is to increase the timer, but by how much? Dunno…

As discussed on IRC, profiling for this patch with the timer set to every minute showed no performance overhead so we should be ok here

Comment 8

[Ursula Sarracini (:ursula)]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195476/diff/1-2/

Comment 9

[Ed Lee :Mardak]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

https://reviewboard.mozilla.org/r/195476/#review202070

Looks like we can clean up and avoid a bit of complexity bit without the expiration stuff.

::: browser/base/content/browser-thumbnails.js:59
(Diff revision 2)
>        Services.prefs.getBoolPref(this.PREF_ACTIVITY_STREAM_ENABLED);
>  
> +    // Only add an expiration filter for tiles - activity stream has it's own
> +    // expiration filter in TopSitesFeed.jsm
> +    if (!this._activityStreamEnabled) {
> +      PageThumbs.addExpirationFilter(this);

We shouldn't need to do this for tiles as NewTabUtils already has the expiration filter, so all the expiration stuff can be removed from this file.

::: browser/base/content/browser-thumbnails.js:250
(Diff revision 2)
> +    sites = topSites.concat(pinnedLinks);
> +  } else {
>      sites = NewTabUtils.links.getLinks();
>    }
>    return sites.reduce((urls, link) => {
> -    if (link) urls.push(link.url);
> +    if (link) urls.push({url: link.url, hasRichIcon: link.faviconSize >= 96});

I believe the only consumer of this top sites array if we remove the expiration stuff would be for checking if the url is one to capture. So we could actually just skip those with rich icons and only include the plain url strings as exposed top sites.

Comment 10

[Ursula Sarracini (:ursula)]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195476/diff/2-3/

Comment 11

[Ed Lee :Mardak]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

https://reviewboard.mozilla.org/r/195476/#review202396

Probably add a test that at least gets gBrowserThumbnails._topSiteURLs with a hole in pinned links to make sure it doesn't throw.

::: browser/base/content/browser-thumbnails.js:147
(Diff revision 3)
>      if (!aBrowser.currentURI ||
> -        topSites.indexOf(aBrowser.currentURI.spec) == -1)
> +        topSites.indexOf(aBrowser.currentURI.spec) === -1) {
>        return;
> +    }
> +
> +    // at this point we know it's ok to get a screenshot for this site

nit: looks like these changes aren't needed. And the original comment "Only capture about:newtab top sites." would apply to either activity stream or tiles.

The specific comment about rich icons is probably better next to the actual filtering.

::: browser/base/content/browser-thumbnails.js:224
(Diff revision 3)
> +  if (gBrowserThumbnails._activityStreamEnabled) {
> +    // Get both the top sites returned by the query, and also any pinned sites
> +    // that the user might have added manually that also need a screenshot
> +    let topSites = await NewTabUtils.activityStreamLinks.getTopSites();
> +    let pinnedLinks = NewTabUtils.pinnedLinks.links;
> +    sites = topSites.concat(pinnedLinks).filter(link => !(link.faviconSize >= 96));

We probably want a unit test here. I would think the `if (link) urls.push()` below is to handle holes in pinned links, so concating then filtering could throw. Maybe

```js
// Include top sites that don't have rich icons
sites.push(...topSites.filter(link => …))
sites.push(...NewTabUtils.pinnedLinks.links)
```

Comment 12

[Ursula Sarracini (:ursula)]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195476/diff/3-4/

Comment 13

[Ursula Sarracini (:ursula)]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195476/diff/4-5/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44f0f3508112
Capture loaded page instead of relying on background screenshot r=Mardak

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b78a31f929a
Backed out changeset 44f0f3508112 for eslint failure in /builds/worker/checkouts/gecko/toolkit/components/thumbnails/test/browser_thumbnails_bg_topsites.js:11:53 r=backout on a CLOSED TREE

Comment 16

[Ursula Sarracini (:ursula)]

Comment on attachment 8924260 [details]
Bug 1413650 - Capture loaded page instead of relying on background screenshot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195476/diff/5-6/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f9f5ef2c141
Capture loaded page instead of relying on background screenshot r=Mardak

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Ooop - just saw this one go by and I'm a little worried about it.

I'm reasonably certain that we're doing the cookie-less screenshotting deliberately - specifically because it works around certain privacy concerns people had. I believe one example that was given was that one could have their banking information saved in a screenshot when logged into their bank, and not be aware of it.

We might want to back this out. :( ni'ing markh for confirmation that I'm not off my rocker here.

Comment 20

[Ed Lee :Mardak]

To be clear, the change here makes it so that pinned top sites get screenshots from opened tabs and that frecent sites can get screenshots from opened tabs without requiring a screenshot.

This bug was originally filed thinking only background screenshots were being used, but turns out they were actually already being captured from open pages (but only for frecent top sites after a restart).

Comment 21

[Ed Lee :Mardak]

Er, first line "from opened tabs without requiring a screenshot" s/screenshot/restart/

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Okay, I spoke with ursula today about this IRL, and she convinced me that my fears were unfounded. Originally, I thought we had somehow accidentally opened up some kind of privacy hole here where thumbnails in about:newtab might suddenly start showing data from logged in sites.

Apparently, Tiles about:newtab was already doing that, and we were doing that before this patch landed. Here's a screenshot from my about:newtab:

https://i.imgur.com/Ds2i4l1.png

It's tiny, but I can clearly tell that this Bugzilla thumbnail has me logged in.

Sorry for (yet another) fire drill. :)

While digging into this this morning, I found pre-existing open bugs that seemed relevant, including bug 755996.

I'm going to keep the needinfo? here on markh in case he wants to chime in with anything here, but I suspect we should just let this ride.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

The thumbnail service certainly jumped through many hoops to avoid capturing logged in states of https and certain other sites for exactly the reasons Mike mentions, and was basically the main driver for the background thumbnail service. That service was designed specifically for for the "old" about:newtab

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #22)
> Apparently, Tiles about:newtab was already doing that, and we were doing
> that before this patch landed. Here's a screenshot from my about:newtab:
> 
> https://i.imgur.com/Ds2i4l1.png
> 
> It's tiny, but I can clearly tell that this Bugzilla thumbnail has me logged
> in.

I am quite concerned about that. There's some more context in bug 754608 and bug 756881. Even though this specific bug isn't introducing this new behaviour, I think it would be worthwhile getting a privacy review. Ursula, are you able to arrange that?

Comment 24

[Ursula Sarracini (:ursula)]

Sure thing, I can get that set up.