Closed Bug 820678 Opened 12 years ago Closed 11 years ago

Only showing the CTP doorhanger popup if there is a vulnerable plugin on the page

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 24

People

(Reporter: tetsuharu, Assigned: tetsuharu)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 12 obsolete files)

Show optional notification for hidden or vulnerable plugins.
7.00 KB, patch
Details | Diff | Splinter Review
part2-v5: Change the case of showing CTP popup
5.14 KB, patch
jaws
: review+
Details | Diff | Splinter Review
part3-v3: Fix test
19.19 KB, patch
Details | Diff | Splinter Review 
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #0)
> Please see bug 819992 comment #3

Oops I typo.
Correctly,  "Please see bug 819992 comment #28"
Blocks: 819992
Attached patch work in progress v1 (obsolete) — — Splinter Review 
I have not tested this yet.
Attached patch wip v2 (obsolete) — — Splinter Review 
Changed:
* opening doorhanger popup once per-window *only blocking outdated plugins*.

And the reason of change is following:
---------
I agree the patch of bug 819992 is good for blocking outdated plugins.

However, at "normal" click-to-play (plugins.click_to_play=true), I feel this change which is shows per window per session has been not good yet.
We can regard as that user who uses normal click-to-play recognizes the feature of normal click-to-play, because it is an opt-in feature.

Thus showing the doorhanger popup per browser window might be very annoying for user who is used to use normal opt-in click-to-play, because doorhanger popup steals user focus.
It's caused by web page which user is visiting but user does not know a constituent element of.  Some doorhanger (geolocation, accessing IndexedDB) is shown by user action or confirming user privacy. So they need to get user focus and it's better.
But click-to-play is different from them. The doorhanger of CTP is shown automatically by astructure of page content. It's not user action.
Even if showing popup is once, it's make user stress.

This feature of showing doorhange popup once per-window is very effective for user to alert that your plugin is outdated and please update it. This should be automatic.

I thought so, and changed the behavior in this patch.
Attachment #691272 - Attachment is obsolete: true
Attached patch patch v1 (obsolete) — — Splinter Review 
I added tests.
try-server: https://tbpl.mozilla.org/?tree=Try&rev=de92ec0ab346
Attachment #691290 - Attachment is obsolete: true
Attachment #691671 - Flags: review?(gavin.sharp)
Comment on attachment 691671 [details] [diff] [review]
patch v1

I think Jared is probably a better reviewer for this.
Attachment #691671 - Flags: review?(gavin.sharp) → review?(jaws)
Jared, How do you think about this patch?

Could you review this patch?
If you are busy now, I'll find other reviewer.
Comment on attachment 691671 [details] [diff] [review]
patch v1

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

::: browser/base/content/browser-plugins.js
@@ -274,5 @@
> -      let isInvisible = ((computedStyle.width == "240px" &&
> -                          computedStyle.height == "200px") ||
> -                         gPluginHandler.isTooSmall(plugin, overlay));
> -      return (!isInvisible &&
> -              gPluginHandler.canActivatePlugin(objLoadingContent));

Why is all this code being removed?
(In reply to Jared Wein [:jaws] from comment #7)
> Why is all this code being removed?

Jared, Thank you for your speedy response.

I'll account why this patch removes its part.
(Some parts is duplication I commented at comment #3)

First, this patch change the current behavior to like this:
* The doorhanger icon which notifies click-to-play for outdated plugins will show its popup only once per window per session. (This behavior is very similary tha patch which you fixed in bug 819992.)
* The doorhanger icon which notifis normal click-to-play will not shown automatically any time.

Why does this patch change the current behavior?  Because, I thought that,  click-to-play for blocking outdated plugins and normal one are similar behavior. but their property is different.
Click-to-play for blocking outdated plugins has a property that urge & alert using vulnerability plugins to user. This fetature works automatically.
On the other hand, normal click-to-play has a property which indicate that this page has some plugins and they have not run yet. But normal click-to-play is opt-in feature enabled by user. This point is diffenrent from CTP for blocking.

If it is opt-in feature, we can assume that user enabled its feature with understanding its behavior. If there is CTP doorhanger after user start to run all visible plugins with clicking, user must concern that this page has any invisible plugin content.

In a related move, providing over-information will prevent user task. The current behavior shows the doorhanger popup suddenly. This is annoying for user browsing because showing doorhanger popup steals user focus. And this *sudden* popup is shown randamly when the page which user is visiting has some invidible plugin contents. User cannot know whether the page has some invidible plugins or not. Suprise attack is not good.
(Of course, It's not the bad way to show only once the normal CTP popup after user set plugins.click_to_play=true. But I don't need it.)

As the result I consider these reasons, I remove all its part that checks whether plugins is invisible or not.
I think the patch which you fixed in bug 819992 is good. However, we can make it more smart with considering a good role for it.
Comment on attachment 691671 [details] [diff] [review]
patch v1

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

Thanks for the explanation.

It looks like this would now show up on the first page that a user visits with plugins on it, instead of the first page that a user visits with invisible plugins? Is that correct? I think there are at least two benefits to the way this worked before this patch: 1) Some users may never see this doorhanger 2) Those that do see it, see the doorhanger when it is more relevant and necessary, since we are pretty sure there is nothing on the page that can be clicked on to activate the plugin.

::: browser/base/content/browser-plugins.js
@@ +617,5 @@
>        }
>      }];
> +
> +    let dismissed = !haveVulnerablePlugin || this._notificationDisplayedOnce;
> +    if (!this._notificationDisplayedOnce) {

This should be |if (!dismissed)|. If the first page that the user visits does not have a vulnerable plugin, then the notification will be dismissed but this._notificationDisplayedOnce will become true, which is incorrect. After you make this change, I think you'll need to revisit the changes you have made to the tests (they can all probably be reverted).

Also, no need for curly brackets on the if-statement if the if-block is only one line.
Attachment #691671 - Flags: review?(jaws) → review-
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
(In reply to Jared Wein [:jaws] from comment #9)
> It looks like this would now show up on the first page that a user visits
> with plugins on it, instead of the first page that a user visits with
> invisible plugins? Is that correct?

No, that isn't. This patch aim to change that it would now show up on the first page that a user visits with some *vulnerability* plugins. When there are only safty plugins, it would not show up the popup on the first page.
(I describe on next paragraph about why we can remove showing the popup on the first page which has safety plugins.)

> I think there are at least two benefits to the way this worked before this patch:
> 1) Some users may never see this doorhanger
> 2) Those that do see it, see the doorhanger when it is more
> relevant and necessary, since we are pretty sure there is nothing on the
> page that can be clicked on to activate the plugin.

I was worrying about it…
But as the result of considering about other browsers which have click-to-play, I feel that they do not have any problem even if they did not notifying about click-to-play at first time for user. So I thought we can remove it.
And also, some user use click-to-play as substitute of flashblock addon. If such case, the showing popup would be interruption for them.

I'm rebasing the patch now. I would be able to attach the new patch in early.
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #10)
> But as the result of considering about other browsers which have
> click-to-play, I feel that they do not have any problem even if they did not
> notifying about click-to-play at first time for user.

Sorry. My account was less word.
This "even if they did not notifying about click-to-play" means "even if they did not describe about click-to-play with using any popup". Other browsers which have click-to-play shows only click-to-play icons in their location-bar.
Attached patch patch v2 (obsolete) — — Splinter Review 
Rebased on latest m-c.

try-server: https://tbpl.mozilla.org/?tree=Try&rev=068e75383c69
Attachment #691671 - Attachment is obsolete: true
Attachment #693566 - Flags: review?(jaws)
Comment on attachment 693566 [details] [diff] [review]
patch v2

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

The accessing of gPluginHandler's private variables is fragile. Please see the change that I requested in bug 822720. You should probably wait for that bug to get fixed first.

Second (and more importantly), this means that we won't ever show the doorhanger for users using click-to-play, and it also means that we lose some test coverage for the blocklisted plugin code path. I don't see the big issue with showing this once per session for click-to-play users.
Attachment #693566 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] from comment #13)
> The accessing of gPluginHandler's private variables is fragile. Please see
> the change that I requested in bug 822720. You should probably wait for that
> bug to get fixed first.

Yes. It's right.
But it's principle. In some case, we need to touch private variables in some case to increase test's independence. I think that this problem is low risk for testable code base. (Before change the code, we can use MXR/DXR to find a code caller.)


> Second (and more importantly), this means that we won't ever show the
> doorhanger for users using click-to-play, and it also means that we lose
> some test coverage for the blocklisted plugin code path. I don't see the big
> issue with showing this once per session for click-to-play users.

This big issue is not showing this once per session for click-to-play users.
The big issue is showing this randomly once per session.

User don't have any way to know whether the page he visits has invisible plugin contents or not. This means that click-to-play doorhanger popup will shows randomly and steals the focus randomly for user. This behavior surprises user. Because there are no regularity.

Does user always open the page which has invisible plugins at first after browser started-up? We cannot confirm it. So we only take the way which does not prevent user task.

Even if this popup is useful to inform click-to-play for user at first time, this will be very annoying for user who experienced this popup many times. (User will launch Firefox many times. They will see the popup & be robbed the focus.)


I don't know well about tests of click-to-play & blocklisted plugins..
Would you tell me where we lose test coverage?
I'll fix them if we need to fix it.
The random showing popup cannot have the meaning of how to use, because it may be shown after user understand how to use it.
Attached patch patch v2.1 (obsolete) — — Splinter Review 
Rebased.
Attachment #693566 - Attachment is obsolete: true
Attachment #695307 - Flags: review?(jaws)
Comment on attachment 695307 [details] [diff] [review]
patch v2.1

Stephen, I'd like to get your opinion on this.

Saneyuki is proposing that we only show the doorhanger if there is a vulnerable plugin on the page (disregarding normal pages with the click-to-play plugins but no vulnerable ones) and showing it regardless of the size of the plugin.

It would continue to be shown only once per window session, but it would appear the first time a vulnerable plugin is included in a page (as compared to how it works now where it shows the first time a hidden plugin is included in a page).

Do you think this is an OK move to make?
Attachment #695307 - Flags: review?(jaws) → feedback?(shorlander)
What, exactly, is the purpose and scope of this bug? It seems to have mutated significantly from a simple code optimization, and is now changing behavior? I'm confused.
(In reply to Justin Dolske [:Dolske] from comment #18)
> What, exactly, is the purpose and scope of this bug? It seems to have
> mutated significantly from a simple code optimization, and is now changing
> behavior? I'm confused.

At first, I thought a simply change. (attachment 691272 [details] [diff] [review])
But after that, I thought that the current behavior is not good and changing the behavior is optimizing gPluginHandler.handlePluginScripted() holistically.
It probably would have been better to simply fix the optimization here (especially since a previous but explicitly spun it out here), and deal with "holistic" changes in a separate bug.

Please update the bug title?

(In reply to Jared Wein [:jaws] from comment #17)

> Saneyuki is proposing that we only show the doorhanger if there is a
> vulnerable plugin on the page (disregarding normal pages with the
> click-to-play plugins but no vulnerable ones) and showing it regardless of
> the size of the plugin.
> 
> It would continue to be shown only once per window session, but it would
> appear the first time a vulnerable plugin is included in a page (as compared
> to how it works now where it shows the first time a hidden plugin is
> included in a page).

Hmm, can you briefly restate that in terms of what this is changing, why it should be done, and how it changes from current behavior? I'm still finding things a bit muddled here.
Summary: Optimize gPluginHandler.handlePluginScripted() (follow-up bug 819992) → Only showing the CTP doorhanger popup if there is a vulnerable plugin on the page
Blocks: 825201
(In reply to Justin Dolske [:Dolske] from comment #20)
> It probably would have been better to simply fix the optimization here
> (especially since a previous but explicitly spun it out here), and deal with
> "holistic" changes in a separate bug.
> 
> Please update the bug title?

OK. I changed this bug title and clone Bug 825201 to optimize gPluginHandler.handlePluginScripted() only.
Attached patch patch v2.2 (obsolete) — — Splinter Review 
Rebased on latest m-c.
Comment on attachment 697985 [details] [diff] [review]
patch v2.2

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

Can you undo the change that removes the plugin-size check, and keep the rest? Only showing the doorhanger the first time there is a vulnerable plugin seems fine to me.
Attachment #697985 - Flags: feedback+
(In reply to Jared Wein [:jaws] from comment #23)
> Can you undo the change that removes the plugin-size check, and keep the
> rest?

I think, we cannot do it. The plugin-size check depends on |_notificationDisplayedOnce|. After changing the behavior, its flags need not consider the plugin-size.

> Only showing the doorhanger the first time there is a vulnerable
> plugin seems fine to me.

Thank you very much for employing my proposal!
I'll update the patch.
Attachment #695307 - Attachment is obsolete: true
Attachment #695307 - Flags: feedback?(shorlander)
Attachment #697985 - Attachment is obsolete: true
In changing the behavior, we need to modify PopupNotifications.jsm for simplifying PopupNotifications::Notification.show(). This should provide as only public wrapper PopupNotifications._update().
Attachment #701430 - Flags: review?(gavin.sharp)
Rebased & the new patch series.
Attachment #701431 - Flags: review?(jaws)
Attached patch part3-v1: Fix test (obsolete) — — Splinter Review 
Fix the test.
This patch includes fixes to care part1 & part2.
But I've care only tests which occurs some error on try-server. Maybe, there is some tests which will be needless by this behavior change.
Attachment #701432 - Flags: review?(jaws)
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #26)
> In changing the behavior, we need to modify PopupNotifications.jsm for
> simplifying PopupNotifications::Notification.show(). This should provide as
> only public wrapper PopupNotifications._update().

Can you explain why this change is needed? I'm not sure I understand.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #29)
> 
> Can you explain why this change is needed? I'm not sure I understand.

By the current, PopupNotifications::Notification.reshow() calls PopupNotifications_reshowNotificationForAnchor. But this method surely shows doorhanger popups as non-dismissed. Thus we cannot update CTP doorhanger simply with using Notification.reshow(). If we update the CTP doorhanger which doesn't reshow its popup, we'll need to call PopupNotifications._update(). This is not good.

I interpreted that PopupNotifications::Notification.reshow() aims the new public simple wrapper method to call PopupNotifications._update(). But it aims to call PopupNotifications._update() & reshowing doorhanger popup. If we should add other public method to call PopupNotifications._update() only, I'll try to do it on next patch.

(I should have accounted these discussion in Bug 821170. I apologize it.)
I think the previous patches makes some invalid assumptions.

1) "CTP is hidden as an about:config preference, so only people who know what they're doing will enable it"

There are plenty of websites already talking about it. Users are known for blindly changing settings just because someone talked about it.

http://www.google.com/search?hl=en&safe=off&sout=1&q=firefox%20click%20to%20play

2) "Users who enable CTP already know how it works"

This also assumes that users know about hidden plugins, which may not be the case.



I agree that the once per-window-per-session pop-up is annoying. However, I see its value for other people. IMHO this behavior should not be removed, but instead be controlled with a preference.

I'd also go so far as to suggest that the vulnerable plugin notification also be optional. There are bound to be those who don't care or don't have the means to update their plugins. Constantly poking them about it would be a less than idea experience for them. Plus, there are already other options to disable/override security warnings.

(DISCLAIMER this patch has not been compiled and is untested, so I don't know if it works or would cause test breakage)
(In reply to Matthew Turnbull [Bluefang] from comment #31)
> I think the previous patches makes some invalid assumptions.
> 
> 1) "CTP is hidden as an about:config preference, so only people who know
> what they're doing will enable it"
> 
> There are plenty of websites already talking about it. Users are known for
> blindly changing settings just because someone talked about it.
> 
> http://www.google.com/
> search?hl=en&safe=off&sout=1&q=firefox%20click%20to%20play
> 
> 2) "Users who enable CTP already know how it works"
> 
> This also assumes that users know about hidden plugins, which may not be the
> case.
> 
> 
> 
> I agree that the once per-window-per-session pop-up is annoying. However, I
> see its value for other people. IMHO this behavior should not be removed,
> but instead be controlled with a preference.

OK. There is some sense.

BUT, we can/should care these knowledge problem with help document like SUMO, even if we add visible GUI preference. Overmuch warning popup does not make user benefits. Stealing user focus is "very" annoying. After user settled & know hidden plugins well, this popup must be interruption.
How about do you think about that we'll add the link which pull help page of CTP in doorhanger popup of normal CTP ?


> I'd also go so far as to suggest that the vulnerable plugin notification
> also be optional. There are bound to be those who don't care or don't have
> the means to update their plugins. Constantly poking them about it would be
> a less than idea experience for them. Plus, there are already other options
> to disable/override security warnings.

There is some sense, too.
It's may be good that vulnerable plugin notification should be optional.
However, I don't follow it in this time. Because:

1. Today, vulnerable plugin is big security risk for browser. Browser vender (we) can update our browser, but cannot update plugins with only our updater. Sadly, we need ask user to update it.
2. From the other warning notification behavior, vulnerable plugin notification should be optional. But this case is a little special. This notification will be shown only in per-window-per-session. With reason (1), I think that we need not make it as optional.

Of course, I have a little ambivalence. If other says this should be so, I'll make the patch which makes vulnerable plugin notification as optional.
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #30)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #29)
> > 
> > Can you explain why this change is needed? I'm not sure I understand.
> 
> By the current, PopupNotifications::Notification.reshow() calls
> PopupNotifications_reshowNotificationForAnchor. But this method surely shows
> doorhanger popups as non-dismissed. Thus we cannot update CTP doorhanger
> simply with using Notification.reshow(). If we update the CTP doorhanger
> which doesn't reshow its popup, we'll need to call
> PopupNotifications._update(). This is not good.
> 
> I interpreted that PopupNotifications::Notification.reshow() aims the new
> public simple wrapper method to call PopupNotifications._update(). But it
> aims to call PopupNotifications._update() & reshowing doorhanger popup. If
> we should add other public method to call PopupNotifications._update() only,
> I'll try to do it on next patch.
> 
> (I should have accounted these discussion in Bug 821170. I apologize it.)

Before calling .reshow(), couldn't the consumer set dismissed=false? That would make it so that the notification is reshown in a non-dismissed state.
We're stretching the PopupNotifications API beyond its original design boundaries, and so we need to step back and figure out what behavior makes the most sense. Popup notifications were originally meant to be relatively transient - you show them, and then you either remove them or let the popup notification code remove them. "Reshowing" a notification wasn't supported, the idea being that you could just re-call PopupNotifications.show(). Is there a reason that couldn't work here? Are we storing too much state on the notifications themselves? If we could avoid changes to PopupNotifications for this bug (and even undo the addition of reshow() from the other bug) I think that would be desirable, but I'm not sure whether that is feasible.

I will be away for a week starting tomorrow, so I do not have time to look into this further now. I'll delegate the decision here to Jared.
Could you please post a screenshot or two showing what the UI will be?

In bug 829111, users complained that the current UI isn't discoverable enough. I think that this will be a problem, especially if/when we start proactively making some/most/nearly-all/all plugins click-to-play even before they are known to have vulnerabilities.

Also, people raised concerns about clickjacking regarding the in-content activation of plugins. I am curious about whether the UI in this bug would make the address-bar-based UI easy enough to use that we could remove the in-content activation. (That would happen in a different bug; I'm just curious as to what the proposed UI is like.)
(In reply to Brian Smith (:bsmith) from comment #35)
> Also, people raised concerns about clickjacking regarding the in-content
> activation of plugins. I am curious about whether the UI in this bug would
> make the address-bar-based UI easy enough to use that we could remove the
> in-content activation. (That would happen in a different bug; I'm just
> curious as to what the proposed UI is like.)

Indeed, I filed bug 832481 for this
I apologize that my reply is very late.

(In reply to Jared Wein [:jaws] from comment #33)
> Before calling .reshow(), couldn't the consumer set dismissed=false? That
> would make it so that the notification is reshown in a non-dismissed state.

No. We could set dismissed=false before calling .reshow(). But If we set it, CTP popup will be reshown in a non-dissmissed state even if the CTP is normal one.


(In reply to :Gavin Sharp (away Jan 16-23) from comment #34)
> We're stretching the PopupNotifications API beyond its original design
> boundaries, and so we need to step back and figure out what behavior makes
> the most sense. Popup notifications were originally meant to be relatively
> transient - you show them, and then you either remove them or let the popup
> notification code remove them. "Reshowing" a notification wasn't supported,
> the idea being that you could just re-call PopupNotifications.show(). Is
> there a reason that couldn't work here? Are we storing too much state on the
> notifications themselves? If we could avoid changes to PopupNotifications
> for this bug (and even undo the addition of reshow() from the other bug) I
> think that would be desirable, but I'm not sure whether that is feasible.

It may be no problem that we could just call PopupNotifications.show() with paramaters extracted from PopupNotifications::Notifications. But if we change the structure of PopupNotifications::Notifications, we'll need change this part. This is risky. If we can do it, we should call PopupNotifications._update() with wrapper function or observer notifying. It is simply.


(In reply to Brian Smith (:bsmith) from comment #35)
> Could you please post a screenshot or two showing what the UI will be?
>
> In bug 829111, users complained that the current UI isn't discoverable
> enough. I think that this will be a problem, especially if/when we start
> proactively making some/most/nearly-all/all plugins click-to-play even
> before they are known to have vulnerabilities.

This bug will change:
* The CTP of vulnerable plugins is popup (same as PopupNotifications' |dismissed=false| options) per-window per-session when the browser found vulnerable plugins.
* All CTP of normal plugins always are shown with no popup (same as PopupNotifications' |dismissed=true| options)

I think this change will make more discoverable for user with showing popup when The CTP is for vulnerable plugins. Of course, this change has not support yet like this case:

0. User has vulnerable plugin (A) but he haven't update it yet.
1. The CTP of vulnerable plugin (A) was shown in the browser window.
2. Plugins blocklist had been updated After (1), and new vulnerable plugin (B) was added to blocklist.
3. The page which is opened in the same browser window & it has the content of plugin (B).

In this special case, vulnerable CTP will be shown without popup. This may not be discoverable enough. But even if we support this special case, we should treat it in other bug, I think ;)
Attached patch part3-v1.1: Fix test (obsolete) — — Splinter Review 
Rebased.
Attachment #701432 - Attachment is obsolete: true
Attachment #701432 - Flags: review?(jaws)
Attachment #705826 - Flags: review?(jaws)
Rebased & follow bug 831365.
Attachment #701431 - Attachment is obsolete: true
Attachment #701431 - Flags: review?(jaws)
Attachment #706444 - Flags: review?(jaws)
Attached patch part3-v2: Fix test (obsolete) — — Splinter Review 
Update the patch.
Attachment #705826 - Attachment is obsolete: true
Attachment #705826 - Flags: review?(jaws)
Attachment #706767 - Flags: review?(jaws)
Comment on attachment 706444 [details] [diff] [review]
part2-v4: Change the case of showing CTP popup

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

::: browser/base/content/browser-plugins.js
@@ +7,5 @@
>    PLUGIN_SCRIPTED_STATE_NONE: 0,
>    PLUGIN_SCRIPTED_STATE_FIRED: 1,
>    PLUGIN_SCRIPTED_STATE_DONE: 2,
>  
> +  _notificationDisplayedOnce: false,

Please move this to be located above _showClickToPlayNotification
Attachment #706444 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] from comment #43)
> Comment on attachment 706444 [details] [diff] [review]
> part2-v4: Change the case of showing CTP popup
> 
> Review of attachment 706444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-plugins.js
> @@ +7,5 @@
> >    PLUGIN_SCRIPTED_STATE_NONE: 0,
> >    PLUGIN_SCRIPTED_STATE_FIRED: 1,
> >    PLUGIN_SCRIPTED_STATE_DONE: 2,
> >  
> > +  _notificationDisplayedOnce: false,
> 
> Please move this to be located above _showClickToPlayNotification

Update patch.
Thank you for your review!
Attachment #706444 - Attachment is obsolete: true
Attachment #707754 - Flags: review?(jaws)
Comment on attachment 706767 [details] [diff] [review]
part3-v2: Fix test

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

::: browser/base/content/test/browser_CTPScriptPlugin.js
@@ +30,3 @@
>      { func: testExpectNoPopupPart1,
>        url: gHttpTestRoot + "plugin_test_scriptedPopup2.html" },
> +    { func: testExpectNoPopupPart1,

Some of these test cases can probably be removed since there is nothing extra that they are testing, right?

::: browser/base/content/test/browser_pluginnotification.js
@@ +79,5 @@
>    gBrowser.removeCurrentTab();
> +
> +  // For other all tests, Disable to open the doorhanger
> +  // of blocking outdated plugins once per window per session.
> +  gPluginHandler._notificationDisplayedOnce = true;

I still don't think this is the right way to do this. Is there a per-session way that you could store these details? If so, that would also fix the related bug where these get shown once-per-window when it should only be once-per-session.
Attachment #706767 - Flags: review?(jaws) → review-
Attachment #707754 - Attachment is patch: true
Attachment #707754 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] from comment #45)
> Comment on attachment 706767 [details] [diff] [review]
> part3-v2: Fix test
> 
> Review of attachment 706767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/browser_CTPScriptPlugin.js
> @@ +30,3 @@
> >      { func: testExpectNoPopupPart1,
> >        url: gHttpTestRoot + "plugin_test_scriptedPopup2.html" },
> > +    { func: testExpectNoPopupPart1,
> 
> Some of these test cases can probably be removed since there is nothing
> extra that they are testing, right?

Also, since PluginHandler::handlePluginScripted won't do any size checking after these changes, browser_CTPScriptPlugin.js can be updated and these files can be removed:

plugin_test_noScriptNoPopup.html
plugin_test_scriptedNoPopup1.html
plugin_test_scriptedNoPopup2.html
plugin_test_scriptedNoPopup3.html
plugin_test_scriptedPopup2.html
plugin_test_scriptedPopup3.html

And plugin_test_scriptedPopup1.html should probably be renamed to plugin_test_scriptedPopup.html.

Finally, browser_CTPScriptPlugin.js shouldn't use the pref plugins.click_to_play - it should use this: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_pluginnotification.js#596 (don't forget to undo this setting: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_pluginnotification.js#686 )
Thank you Jared, David.
I'm updating the patch. I would attach it as soon as possible.


(In reply to Jared Wein [:jaws] from comment #45)
> ::: browser/base/content/test/browser_pluginnotification.js
> @@ +79,5 @@
> >    gBrowser.removeCurrentTab();
> > +
> > +  // For other all tests, Disable to open the doorhanger
> > +  // of blocking outdated plugins once per window per session.
> > +  gPluginHandler._notificationDisplayedOnce = true;
> 
> I still don't think this is the right way to do this. Is there a per-session
> way that you could store these details? If so, that would also fix the
> related bug where these get shown once-per-window when it should only be
> once-per-session.

This is safe part. If test18f was failed, other tests related to gPluginHandler._notificationDisplayedOnce may be failed even if other tests are correct essentially. So this part increases independence of tests.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #34)
> I will be away for a week starting tomorrow, so I do not have time to look
> into this further now. I'll delegate the decision here to Jared.


Who should I ask the review of part 1 (attachment 701430 [details] [diff] [review]) ?  Should I change the reviewer flag to Jared?
I'm not sure you need the changes in part 1 - doesn't it work without that patch?
(In reply to David Keeler (:keeler) from comment #49)
> I'm not sure you need the changes in part 1 - doesn't it work without that
> patch?

The current PopupNotifications::Notification.reshow() makes all notification's |dismissed| flags into |true|. This means that it always showing the doorhanger popup when we calls Notification.reshow(). Thus we need to this change (or comparable one).

Of course, we can also use PopupNotifications._update() but it oppose Bug 821170. Or We can add the new wrapper method of PopupNotifications._update(), however, it may increase unconsidered methods.

Maybe we need polish & clean up PopupNotifications APIs.
(In reply to David Keeler (:keeler) from comment #46)
>
> Finally, browser_CTPScriptPlugin.js shouldn't use the pref
> plugins.click_to_play - it should use this:
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> browser_pluginnotification.js#596 (don't forget to undo this setting:
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> browser_pluginnotification.js#686 )

In the part-3 (attachment 706767 [details] [diff] [review]), the CTP of vulnerable plugins is tested in browser_pluginnotification.js::test18f(). I don't feel that we should use  setAndUpdateBlocklist() instead of the pref |plugins.click_to_play|. browser_CTPScriptPlugin.js should only test the "PluginScripted" event.
Update the patch.
Attachment #706767 - Attachment is obsolete: true
Attachment #708443 - Flags: review?(jaws)
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #50)
Right - okay, I see where I was mistaken. I think after bug 836194 we won't need the changes to PopupNotifications.

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #51)
> (In reply to David Keeler (:keeler) from comment #46)
> >
> > Finally, browser_CTPScriptPlugin.js shouldn't use the pref
> > plugins.click_to_play - it should use this:
> > https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> > browser_pluginnotification.js#596 (don't forget to undo this setting:
> > https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> > browser_pluginnotification.js#686 )
> 
> In the part-3 (attachment 706767 [details] [diff] [review]), the CTP of
> vulnerable plugins is tested in browser_pluginnotification.js::test18f(). I
> don't feel that we should use  setAndUpdateBlocklist() instead of the pref
> |plugins.click_to_play|. browser_CTPScriptPlugin.js should only test the
> "PluginScripted" event.

But if the idea is to only open the popup when a vulnerable plugin is scripted, then you'll need a vulnerable plugin, hence using setAndUpdateBlocklist. Furthermore, we've had a few bugs that assume a plugin can only be click-to-play when plugins.click_to_play is true, which is not the case (again, a vulnerable plugin can be click-to-play even without the pref being true).
(In reply to David Keeler (:keeler) from comment #53)
> But if the idea is to only open the popup when a vulnerable plugin is
> scripted, then you'll need a vulnerable plugin, hence using
> setAndUpdateBlocklist. Furthermore, we've had a few bugs that assume a
> plugin can only be click-to-play when plugins.click_to_play is true, which
> is not the case (again, a vulnerable plugin can be click-to-play even
> without the pref being true).

browser_CTPScriptPlugin.js is only testing "PluginScripted" event, isn't it? This bug's goal I understood is that: Shows the doorhanger popup of CTP of vulnerable plugins per-window per-session if the page has any vulnerable plugins.
If i'm reading this right, this will completely remove the popup for hidden non-vulnerable plugins when they are scripted?
If yes: the discoverability of the doorhanger as currently implemented is already not too great, so i don't think we should make it harder to find.
Jared, Could you account me about bug 836194?
Will we land bug 836194's patch before landing this bug's one?
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #56)
> Jared, Could you account me about bug 836194?
> Will we land bug 836194's patch before landing this bug's one?

My goal is to land the patch for bug 836194 before this one lands.
(In reply to Georg Fritzsche [:gfritzsche] from comment #55)
> If i'm reading this right, this will completely remove the popup for hidden
> non-vulnerable plugins when they are scripted?

Am i reading this right or do the changes to reshow() make the popup still show in that case?
(In reply to Georg Fritzsche [:gfritzsche] from comment #58)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #55)
> > If i'm reading this right, this will completely remove the popup for hidden
> > non-vulnerable plugins when they are scripted?
> 
> Am i reading this right or do the changes to reshow() make the popup still
> show in that case?

You're reading this right, but I don't think we've made a solid decision here. I'm not firm on my stance in comment #23, and I know that educating users about why their site might not be working is something that we want to get right.
(In reply to Jared Wein [:jaws] from comment #59)
> You're reading this right, but I don't think we've made a solid decision
> here. I'm not firm on my stance in comment #23, and I know that educating
> users about why their site might not be working is something that we want to
> get right.

Ah, thanks. I think that for the "click-to-play by default" plans this would definitely hurt discoverability unless any better UI options come up in time.
Could we leave the "popup on hidden-plugins getting scripted" part as-is for now until this is resolved?
(In reply to Georg Fritzsche [:gfritzsche] from comment #60)
> (In reply to Jared Wein [:jaws] from comment #59)
> > You're reading this right, but I don't think we've made a solid decision
> > here. I'm not firm on my stance in comment #23, and I know that educating
> > users about why their site might not be working is something that we want to
> > get right.
> 
> Ah, thanks. I think that for the "click-to-play by default" plans this would
> definitely hurt discoverability unless any better UI options come up in time.
> Could we leave the "popup on hidden-plugins getting scripted" part as-is for
> now until this is resolved?

Yeah, I think that would be a good compromise.
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #56)
> Jared, Could you account me about bug 836194?
> Will we land bug 836194's patch before landing this bug's one?

Thank you. OK. I'll rebase these patches.
(In reply to Jared Wein [:jaws] from comment #57)
> My goal is to land the patch for bug 836194 before this one lands.

Thank you. OK. I'll rebase these patches.
(In reply to Georg Fritzsche [:gfritzsche] from comment #60)
> 
> Ah, thanks. I think that for the "click-to-play by default" plans this would
> definitely hurt discoverability unless any better UI options come up in time.
> Could we leave the "popup on hidden-plugins getting scripted" part as-is for
> now until this is resolved?

I don't think it's good for user that we leave the "popup on hidden-plugins getting scripted" part as-is for now.

On the first time user meets hidden-plugins, the current approach is good. However, I don't think it's good for user who many-times meets hidden-plugins. This popup steals user focus. Stealing the focus is very useful for getting user attention. But if user have understood the feature, Stealing the focus is not useful. And it's depend on pages which user is visiting. As the result, Stealing the focus is randomly and user cannot stop it. I don't think this is user benefit.
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #64)
> This popup steals user focus. Stealing the focus is very
> useful for getting user attention. But if user have understood the feature,
> Stealing the focus is not useful.

Right, but we do already have feedback from users who can't find this with the current behavior.

With the plans to make all plugins except Flash click-to-play for all users [1] we shouldn't make this less discoverable as it is now - at least until we have a better alternative.

[1] https://blog.mozilla.org/security/2013/01/29/putting-users-in-control-of-plugins/
(In reply to Georg Fritzsche [:gfritzsche] from comment #65)
> 
> Right, but we do already have feedback from users who can't find this with
> the current behavior.
> 
> With the plans to make all plugins except Flash click-to-play for all users
> [1] we shouldn't make this less discoverable as it is now - at least until
> we have a better alternative.
> 
> [1]
> https://blog.mozilla.org/security/2013/01/29/putting-users-in-control-of-
> plugins/

Umm... This is difficult...

How do you think about adding the new pref which control the "popup on hidden-plugins getting scripted" until we get a better alternative?  We need the way to disable it even if we receive the feedback. 

Jared,
Do you think about this? Which should we do that: leaving the current behavior and adding the new pref, or remove the current one?
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #66)
> Jared,
> Do you think about this?

I'm sorry, Jared.
This part is "'How' do you think about this?".
(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #64)
> On the first time user meets hidden-plugins, the current approach is good.
> However, I don't think it's good for user who many-times meets
> hidden-plugins. This popup steals user focus. Stealing the focus is very
> useful for getting user attention. But if user have understood the feature,
> Stealing the focus is not useful. And it's depend on pages which user is
> visiting. As the result, Stealing the focus is randomly and user cannot stop
> it. I don't think this is user benefit.

We cannot have the doorhanger steal focus when we open it automatically, because if we did we would expose the user to "keyboard jacking" attacks analogous to clickjacking. And, also, IMO we're supposed to be making sure it is most convenient to *not* activate the plugin for the user that doesn't want to. That means the user should be able to scroll with the spacebar and other keyboard features without interruption.
My basic stance is that removing "popup on hidden-plugins getting scripted" completely

Because I recognize that the current per-window per-session behavior is based on their feedback.
And, 
1. If we need to increase the discoverability of hidden-plugins, we should show the popup on every-window every-session. But it will be very annoying for user.
2. If we leave the current per-window per-session behavior for "popup on hidden-plugins getting scripted", it's shown on the first time which user meets hidden-plugins. But I doubt this effectiveness. Because the popup will be shown once. This will not increase the discoverability.

So I don't feel that "popup on hidden-plugins getting scripted" is good until we find it even if we haven't found a better alternative. I think it should be removed.

However, if there is many feedback which user hope more discoverable function, I don't oppose it that keeping "popup on hidden-plugins getting scripted".
(In reply to Brian Smith (:bsmith) from comment #68)
> 
> We cannot have the doorhanger steal focus when we open it automatically,
> because if we did we would expose the user to "keyboard jacking" attacks
> analogous to clickjacking. And, also, IMO we're supposed to be making sure
> it is most convenient to *not* activate the plugin for the user that doesn't
> want to. That means the user should be able to scroll with the spacebar and
> other keyboard features without interruption.

I said that user cannot input any scroll event when the doorhanger's popup is opened.
"popup on hidden-plugins getting scripted" opens the doorhanger's popup randomly which is depend on page's behavior. If it's opened when user scrolling the page, user scroll will not be passed to the pages until user close the opend popup.
This is why I suggested:
- Leaving the existing behavior in place.
- Separately tracking whether the popup has been shown for hidden and vulnerable plugins.
- Controlling both behaviors by preference.

It wouldn't take much to extend this to make the hidden plugin notification show once-per-session or once-ever. Simply set the appropriate preference the first time the panel is shown. Then optionally clear it on shutdown to make it once-per-session.

Also, the popup blocking input is a separate issue. See Bug 596723.
Comment on attachment 701430 [details] [diff] [review]
part1-v1: Change PopupNotifications::Notification.reshow()

As I understand it bug 836194 makes this patch unnecessary.
Attachment #701430 - Flags: review?(gavin.sharp)
Attachment #701430 - Attachment is obsolete: true
UI/UX design work is in progress, including how to deal with discoverability and the doorhanger. It's under discussion on dev.apps.firefox.
I'd suggest to wait what the results of the UI/UX design are before going forward here (as currently non-block click-to-play is not user-facing anyway).
I have a suggestion for the doorhanger popup.  How about this:

1. By default show the popup
2. Record the number of times the user responds to the popup  
3. After X responses, subsequent popups get a "Do not show this again" checkbox at the bottom of the popup, with the following exception: Always show the popup whenever a vulnerable plugin exists for that page.
4. If the user clicks the checkbox and takes some action that dismisses the popup, show a doorhanger with a bouncing arrow pointing at the URL bar icon and a message like the following: "You have chosen to dismiss future popup plugin notifications.  This icon will appear whenever there are plugins on the page.  Click it to activate plugins.  From now on, we will show the popup only if you have a dangerous plugin version installed."  The user would then have to click "OK" to dismiss the message.

This way you achieve discoverability while providing an out from the annoyance factor.

Would this be a reasonable compromise or does this seem too complicated?
IU, progress reports on the UI redesign should appear on dev.apps.firefox, it's probably best to participate there.
E.g. maybe we'll end up with a way less intrusive notification instead of improving the current approach.
(In reply to Georg Fritzsche [:gfritzsche] from comment #75)
> IU, progress reports on the UI redesign should appear on dev.apps.firefox

Please use firefox-dev for future development discussion (http://www.gavinsharp.com/blog/2013/02/26/announcing-the-firefox-dev-mailing-list/).
We've started this in dev.apps.firefox, let's please finish it there also.
Per bug 880735 and dependencies the doorhanger popup isn't shown automatically anymore.
Does that resolve the issue(s) discussed here?
On the new click-to-play, this bug is not reproducible. So I close this.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 880735
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: