912763 - Disable background thumbnails on Beta

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: preprocess out the front-end caller

The background thumbnail service isn't ready for prime time, mainly because we're still tracking down e10s-related crashes it causes (bug 899758, bug 902755), and also because it's just rough around the edges (see dependency trees of bug 870100 and bug 841495).  We'd like to disable it once it makes its way to Beta.

This patch uses the text preprocessor to preprocess out the only front-end caller of the bg service so far, in sites.js, in the release, beta, and esr channels.  Since this doesn't disable the service, add-ons and Firefox hackers can call still call it if they know about it, and I think that's OK.  I looked around and there seem to be many release channels: esr, release, beta, aurora, nightly, nightly-elm, nightly-profiling, nightly-oak, default.  So checking for the first three seemed like the easiest thing to do.  Once the service is ready, we can revert this patch and let that ride the trains.

Comment 1

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

Comment on attachment 799828 [details] [diff] [review]
preprocess out the front-end caller

Review of attachment 799828 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me (although can't we just write a single expression instead of the empty #elif's?), but I think we have enough time to wait for feedback from Gavin.

Comment 2

[Drew Willcoxon :adw]

Attachment: preprocess out the front-end caller v2

You're right: http://mxr.mozilla.org/mozilla-central/source/config/Expression.py

The text preprocessor doc doesn't mention expressions, and no examples I found use them, so I just assumed it wouldn't work.

Comment 3

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

Comment on attachment 799839 [details] [diff] [review]
preprocess out the front-end caller v2

Review of attachment 799839 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome!

Comment 4

[:Gavin Sharp [email: [e-mail]]]

We should use https://wiki.mozilla.org/Platform/Channel-specific_build_defines for this, rather than MOZ_UPDATE_CHANNEL.

I wonder whether we should use EARLY_BETA_OR_EARLIER or RELEASE_BUILD, though... it might be helpful to get a short amount of beta feedback to gauge the severity/magnitude of the bugs. On the other hand, enabling the service for beta users even for a short time will end up skewing future testing results, because we'll have collected a bunch of thumbnails (mostly solving the "missing thumbnails" problem indefinitely, even if the service later gets disabled).

Comment 5

[Drew Willcoxon :adw]

Attachment: RELEASE_BUILD

Thanks, Gavin, page bookmarked.  RELEASE_BUILD does seem better to me right now than EARLY_BETA_OR_EARLIER.  We could use EARLY_BETA_OR_EARLIER down the road when we're more confident in the service's stability, but before we're ready to turn it on in releases for other, smaller problems.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 799870 [details] [diff] [review]
RELEASE_BUILD

Are we super sure this is the only consumer, and will be for the foreseeable future?

I would be a bit more comfortable disabling the service entirely (or at least having its public methods renamed to something very visibly testing-only). Seems like too much of a footgun (for addon authors or other chrome-code authors) otherwise.

Comment 7

[Drew Willcoxon :adw]

It's the only consumer now, and Mark and I can make sure to bracket any new consumers, but I don't foresee any new ones in desktop.  I like the idea of add-ons and hackers being able to play with it early, but other teams might start using it without our knowledge (Windows Metro: bug 850909), so disabling it or warning against its use are definitely reasonable.

Comment 8

[Paul Silaghi, QA [:pauly]]

(In reply to Drew Willcoxon :adw from comment #0)
> We'd like to disable it once it makes its way to Beta.
Will this be accomplished until the merge day, or just somewhere in the 25 betas?

Comment 9

[Drew Willcoxon :adw]

Attachment: option 2: exclude BPT.jsm from build

This excludes BackgroundPageThumbs.jsm from the build entirely.

Comment 10

[Drew Willcoxon :adw]

Attachment: option 3: no-op BPT.capture

The option 2 patch I just posted has warnings in BPT.jsm, but people sometimes overlook things, and it's still possible for other teams to use it without realizing it's excluded from releases.  So as a fail-safe alternative, this patch makes BPT.capture a no-op.  Since it's a no-op, it doesn't ifdef out the PT.captureIfStale caller.

Comment 11

[Drew Willcoxon :adw]

Attachment: option 3: no-op BPT.capture

Sorry, wrong file.

Comment 12

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

Comment on attachment 800526 [details] [diff] [review]
option 3: no-op BPT.capture

Review of attachment 800526 [details] [diff] [review]:
-----------------------------------------------------------------

Note however that the horse has already bolted for this - the release channel already has background thumbnails - http://mxr.mozilla.org/mozilla-release/source/toolkit/components/thumbnails/BackgroundPageThumbs.jsm

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 800524 [details] [diff] [review]
option 2: exclude BPT.jsm from build

(I didn't verify that this was exhaustive, but I think that's not necessary)

Comment 14

[Drew Willcoxon :adw]

Attachment: option 2: exclude BPT.jsm from build + move captureIfStale to BPT

The patch Gavin r+'ed isn't exactly right, because I added preprocessing directives to PageThumbs.jsm but didn't modify the moz.build to put it in EXTRA_PP_JS_MODULES.  Those directives are necessary because PT.captureIfStale calls BPT.  It's a symptom of the fact that captureIfStale doesn't really belong in PT but in BPT, like I say in bug 906615.

So instead of adding PT.jsm to EXTRA_PP_JS_MODULES, this patch moves captureIfStale to BPT.  I'd understand if you r- and want to add PT.jsm to EXTRA_PP_JS_MODULES instead, since these changes are bigger, and all we're trying to do in this bug is disable BPT.  However, keeping all BPT functionality in BPT makes disabling it cleaner, and it makes it easier for callers to see what they can and can't use since the disabled functionality is all contained in one file, and I want to make this change at some point anyway (if not here, then in bug 906615).

Comment 15

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/67329f8dfe7a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67329f8dfe7a

Comment 17

[Drew Willcoxon :adw]

Attachment: Aurora (25) patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): background thumbnailing, bug 870100
User impact if declined: possible crashes, hangs due to background thumbnailing on Beta and †hen release
Testing completed (on m-c, etc.): no testing specifically for this patch; its purpose is to disable a feature on Beta/release
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 18

[Drew Willcoxon :adw]

https://hg.mozilla.org/releases/mozilla-aurora/rev/24fe7575387d

Comment 19

[Paul Silaghi, QA [:pauly]]

I see thumbnails incrementally appearing in FF 25.0a2 (2013-09-12), 26.0a1 (2013-09-12), in comparison with a nightly before the landing of bug 870100.
So...I assume this is not really backed out ?

Comment 20

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

It's not backed out as such - but it's disabled for all beta/release versions (ie, it will stop working in 25 once that hits beta).  We will back out *this* bug when thumbnails are ready to hit beta (probably 26)

Comment 21

[Paul Silaghi, QA [:pauly]]

That makes sense now. Thanks for explaining, Mark.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking relnote? in case we keep this in beta 26

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified disabled on 25.0.1 release, 26 beta 5.