1387682 - Screenshots missing when thumbnailer Promise is garbage collected

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

Looks like bug 1353542 made the promise GC-able see https://bugzilla.mozilla.org/show_bug.cgi?id=1353542#c15

try test enabled without fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3028b854e8d7a616708e768ada0b2f4e96aaf055

try test enabled and with fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53019ce73ea870adfc0accb81aea6879306aabf

Comment 1

[Ed Lee :Mardak]

Attachment: Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected.

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

Comment 2

[Ed Lee :Mardak]

Ah these are probably the leaks florian was trying to avoid:

22:33:05    ERROR - TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js | leaked 12 window(s) until shutdown [url = about:newtab]
22:33:05    ERROR - TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_undo.js | leaked 2 window(s) until shutdown [url = about:newtab]

These shouldn't be activity stream related but I guess the shutdown cleanup of observers isn't enough.

Comment 3

[Ed Lee :Mardak]

https://treeherder.mozilla.org/logviewer.html#?job_id=121155958&repo=try&lineNumber=3730

Comment 4

[Ed Lee :Mardak]

Comment on attachment 8894041 [details]
Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected.

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

Comment 5

[Ed Lee :Mardak]

Comment on attachment 8894041 [details]
Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected.

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

Comment 6

[Ed Lee :Mardak]

Comment on attachment 8894041 [details]
Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected.

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

Comment 7

[Ursula Sarracini (:ursula)]

Comment on attachment 8894041 [details]
Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected.

https://reviewboard.mozilla.org/r/165136/#review171218

Looks good to me, just one change to the comment. Thanks!

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm:93
(Diff revision 4)
>     * Asynchronously captures a thumbnail of the given URL if one does not
>     * already exist.  Otherwise does nothing.
>     *
>     * @param url      The URL to capture.
>     * @param options  An optional object that configures the capture.  See
>     *                 capture() for description.

Can you add 'unloadingPromise' to the options param comment in capture() like this comment suggests?

Comment 8

[Ed Lee :Mardak]

Comment on attachment 8894041 [details]
Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected.

https://reviewboard.mozilla.org/r/165136/#review171218

> Can you add 'unloadingPromise' to the options param comment in capture() like this comment suggests?

Added comment to `captureIfMissing` as that's where it's used.

Comment 9

[Ed Lee :Mardak]

Comment on attachment 8894041 [details]
Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02f357c8e980
Screenshots missing when thumbnailer Promise is garbage collected. r=ursula

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02f357c8e980

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/e8285110be3850d6628f874616d0e865b4612ed0
chore(tests): Re-enable getScreenshots test with bug 1387682 fixed (#3110)