Last Comment Bug 793273 - FF16 CTP blocklist should never be triggered
: FF16 CTP blocklist should never be triggered
Status: RESOLVED FIXED
: verifyme
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: 16 Branch
: All All
: -- blocker (vote)
: mozilla16
Assigned To: David Keeler [:keeler] (use needinfo?)
: Paul Silaghi, QA [:pauly]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-21 12:12 PDT by Alex Keybl [:akeybl]
Modified: 2012-10-04 08:08 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
patch (3.70 KB, patch)
2012-09-28 10:34 PDT, David Keeler [:keeler] (use needinfo?)
bmcbride: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Alex Keybl [:akeybl] 2012-09-21 12:12:06 PDT
FF16 CTP blocklisting functionality should never be triggered by a remote blocklist given the minimal QA it has received thus far.
Comment 1 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-21 12:18:04 PDT
Adding Paul Silaghi as the QA Contact since he is the QA Owner for CTP. Paul, once the engineering work is done here, please test whatever Alex needs.
Comment 2 David Keeler [:keeler] (use needinfo?) 2012-09-21 13:10:56 PDT
Can't we use the targetApplication tag in the blocklist to make any ctp blocks not apply to 16? I'd rather do something like that that's a bit more flexible in case we do decide to use blocklisting ctp in 16 (very unlikely, I know, but it would be nice to have the option).
Comment 3 Alex Keybl [:akeybl] 2012-09-21 17:53:22 PDT
(In reply to David Keeler from comment #2)
> Can't we use the targetApplication tag in the blocklist to make any ctp
> blocks not apply to 16? I'd rather do something like that that's a bit more
> flexible in case we do decide to use blocklisting ctp in 16 (very unlikely,
> I know, but it would be nice to have the option).

CTP in FF16 is completely untested and should never be used. The only reason to leave the functionality in is if disabling it completely isn't low risk.
Comment 5 Lucas Adamski [:ladamski] 2012-09-24 13:38:34 PDT
Seems like we can leave this in, test in parallel then decide whether this is usable?
Comment 6 Alex Keybl [:akeybl] 2012-09-26 15:51:27 PDT
(In reply to Lucas Adamski from comment #5)
> Seems like we can leave this in, test in parallel then decide whether this
> is usable?

This isn't a good use of QA or support resources.

We would need to specify all CTP blocklists as FF17 and up if we do not disable the functionality completely. I'm worried about accidentally enabling the untested functionality.
Comment 7 David Keeler [:keeler] (use needinfo?) 2012-09-26 16:09:17 PDT
(In reply to Alex Keybl [:akeybl] from comment #6)
> We would need to specify all CTP blocklists as FF17 and up if we do not
> disable the functionality completely.

Don't we need to do that anyway? If any version of firefox prior to 16 gets a blocklist with a click-to-play entry that applies to it, it'll show the out-of-date plugin infobar, which I thought was also something we didn't want to do (this may be my misunderstanding - I'm just extrapolating from how often we've decided to not use the old blocklist functionality).

I don't understand why we're reluctant to use this nifty feature of the blocklist where we can say "this block only applies to these version of firefox".
Comment 8 Alex Keybl [:akeybl] 2012-09-26 16:42:07 PDT
(In reply to David Keeler from comment #7)
> (In reply to Alex Keybl [:akeybl] from comment #6)
> > We would need to specify all CTP blocklists as FF17 and up if we do not
> > disable the functionality completely.
> 
> Don't we need to do that anyway? If any version of firefox prior to 16 gets
> a blocklist with a click-to-play entry that applies to it, it'll show the
> out-of-date plugin infobar, which I thought was also something we didn't
> want to do (this may be my misunderstanding - I'm just extrapolating from
> how often we've decided to not use the old blocklist functionality).

The infobar block is what we used for the recent Java block - it's the only desirable experience we currently have. So this bug specifically would force us to specifically exclude FF16 from future CTP blocklists.
Comment 9 Jorge Villalobos [:jorgev] 2012-09-27 14:48:48 PDT
Because of the way the blocklist form currently works we can only limit a plugin block to either a plugin version range *or* an application version range. This means that if we needed to implement a CTP block on a plugin version range, we would be forced to include Firefox 16 in that block.
Comment 10 Alex Keybl [:akeybl] 2012-09-27 14:57:50 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> Because of the way the blocklist form currently works we can only limit a
> plugin block to either a plugin version range *or* an application version
> range. This means that if we needed to implement a CTP block on a plugin
> version range, we would be forced to include Firefox 16 in that block.

Given this, and the other risks outlined, I'm designating this bug as a blocker for release.
Comment 11 David Keeler [:keeler] (use needinfo?) 2012-09-28 10:34:01 PDT
Created attachment 665966 [details] [diff] [review]
patch

Blair - we need to prevent click-to-play blocklisting on FF16, and I'm pretty sure this is the simplest, safest way to do it. Since you reviewed the patch that added this feature, I figured you'd be a good pick to review the patch that takes it out :)
Comment 12 Alex Keybl [:akeybl] 2012-09-28 14:38:23 PDT
(In reply to David Keeler from comment #11)
> Created attachment 665966 [details] [diff] [review]
> patch
> 
> Blair - we need to prevent click-to-play blocklisting on FF16, and I'm
> pretty sure this is the simplest, safest way to do it. Since you reviewed
> the patch that added this feature, I figured you'd be a good pick to review
> the patch that takes it out :)

Some context for Blair - we need to land this on mozilla-beta Monday, so please prioritize this review. Thanks!
Comment 13 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-09-28 20:18:23 PDT
Comment on attachment 665966 [details] [diff] [review]
patch

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

Yep.
Comment 14 Alex Keybl [:akeybl] 2012-10-01 10:05:13 PDT
Comment on attachment 665966 [details] [diff] [review]
patch

[Triage Comment]
Approving this disable to land asap so that we can be confident in future CTP/infobar blocklist rollouts.
Comment 15 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-10-01 15:23:36 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/7520d2746f3f
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-01 15:48:20 PDT
Paul, please make sure you target this for extensive testing with the next Firefox 16 Beta.
Comment 17 Paul Silaghi, QA [:pauly] 2012-10-02 04:27:27 PDT
Roger that. Waiting for the beta builds
Comment 18 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-10-04 07:25:28 PDT
https://bugzilla.mozilla.org/show_bug.cgi?id=797378#c23 indicates that this is not working correctly.
Comment 19 Paul Silaghi, QA [:pauly] 2012-10-04 08:08:16 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> https://bugzilla.mozilla.org/show_bug.cgi?id=797378#c23 indicates that this
> is not working correctly.

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=797378#c27

Note You need to log in before you can comment on or make changes to this bug.