Closed Bug 1133003 Opened 5 years ago Closed 5 years ago

When users experience a Flash plugin hang, turn off Flash protected mode

Categories

(Core :: Plug-ins, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla38
Tracking Status
firefox38 --- disabled

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

User Story

* If Flash hangs twice in the same session and Flash protected mode is active, Firefox will automatically disable protected mode.
* Firefox will show a notification to the user explaining that we have made a change on their behalf.
* via bug 1133000, the user can undo this change.
* If the user turns protected mode back on, we won't repeat the automatic change. (We will only make this automatic change once.)

Attachments

(3 files, 1 obsolete file)

Currently we have a hook which disabled Flash protected mode, but no UI to expose this to users.

Since turning protected mode off for all users may be a security risk we are unwilling to bear, we are considering offering users a way to opt into this option if/when they experience a Flash hang.

This is complicated because most users are incapable of making an informed choice to trade off security versus functionality, and the security benefits and risks are not easily measurable.
User Story: (updated)
Adding myself to this but I believe, once the overall plan is adopted / approved, the next action item here is to develop the actual copy that will make up the notice (I am not sure who should own that whether Eric, Erica or Mary Ellen or a combination), implemented by Madhava's team (I assume this is a door hanger or pop up notice of some kind), and in my view perhaps we add a Learn More link to that notice which maps to a Wiki, SUMO article, or Ben perhaps a Blog post explaining all of the ins and out.
UX team typically writes the in-product notice. They'll decide whether to include a learn more link, which would go to SUMO.
Attached patch 1133003-partASplinter Review
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #8566687 - Flags: review?(aklotz)
Attachment #8566687 - Flags: review?(aklotz) → review+
Attachment #8566805 - Flags: ui-review?(madhava)
Attached patch 1133003-partB (obsolete) — Splinter Review
Haven't gotten final UX review on this, but we're close to the 38 wire so I'm going to put up what I've got for code review. I like simple!
Attachment #8566826 - Flags: review?(jaws)
Comment on attachment 8566805 [details]
2015-02-19_flash-flipui.png

Firefox has changed some Adobe Flash settings to improve performance.
Where doe the Learn More button go and what is on the Learn More link page? Who is authoring that content?
Active voice for Chad:

Firefox changed some Adobe Flash settings to improve performance.             [Learn More]
(In reply to Madhava Enros [:madhava] from comment #8)
> Active voice for Chad:
> 
> Firefox changed some Adobe Flash settings to improve performance.           
> [Learn More]

"to improve performance" or "to improve reliability"? I think reliability makes more sense here since it will be (hopefully) less crashy after the change. But maybe performance and reliability are synonymous here.
Flags: needinfo?(madhava)
I'd like to stick with "performance" both because I think it covers the whole gamut of how well software performs, but also because the things we're optimizing here is hangs, which are primarily perceived by users as Firefox being slow.
Comment on attachment 8566826 [details] [diff] [review]
1133003-partB

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

r=me with the following fixed

::: browser/components/nsBrowserGlue.js
@@ +2074,5 @@
> +      return;
> +    }
> +    let productName = Services.strings
> +      .createBundle("chrome://branding/locale/brand.properties")
> +      .GetStringFromName("brandFullName");

The .properties file says that it expects brandShortname here.
Attachment #8566826 - Flags: review?(jaws) → review+
Update: doing a "Learn More" link required a little bit of plumbing in the <setting> binding. Looks pretty straightforward to me, but I wanted somebody else to read it.
Attachment #8566826 - Attachment is obsolete: true
Attachment #8567293 - Flags: review?(MattN+bmo)
Attachment #8566805 - Flags: ui-review?(madhava)
Comment on attachment 8567293 [details] [diff] [review]
bug1133000-flash-toggle.patch

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

::: toolkit/mozapps/extensions/content/extensions.css
@@ +73,5 @@
>    display: -moz-grid-line;
>    -moz-binding: url("chrome://mozapps/content/extensions/setting.xml#setting-localized-bool");
>  }
>  
> +setting[type="bool"] .preferences-learnmore {

You could also do the following so you don't need to hard-code "visibility: visible;":
setting[type="bool"]:not([learnmore] .preferences-learnmore {
  visibility: collpase;
}

::: toolkit/mozapps/extensions/content/setting.xml
@@ +144,5 @@
>            <xul:label class="preferences-title" flex="1" xbl:inherits="xbl:text=title"/>
>          </xul:hbox>
>          <xul:description class="preferences-description" flex="1" xbl:inherits="xbl:text=desc"/>
> +        <xul:label class="preferences-learnmore text-link"
> +                   onclick="document.getBindingParent(this).openLearnMore()">&setting.learnmore;</xul:label>

You should probably check with UX about the desired location of the Learn More link. It seems to be on a new line below the description but I wonder if it should be immediately after the description or after the checkbox.

@@ +176,5 @@
> +
> +      <method name="openLearnMore">
> +        <body>
> +        <![CDATA[
> +          window.open(this.getAttribute("learnmore"), "_blank");

Why are you specifying "_blank"? I think it should open in a new tab by default without it.
Attachment #8567293 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/11b57818df63
https://hg.mozilla.org/mozilla-central/rev/973d369b4f28
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
There is a typo in the string:

%S changed some Adobe Flash settings improve performance.
(Missing "to" before "improve")

Could you please land a quick fix before string freeze?

I'd update the entity name though, because as a localizer, I had to read c#8 before getting what the string really meant.
Flags: needinfo?(benjamin)
Fixed typo at https://hg.mozilla.org/integration/mozilla-inbound/rev/cef15f16f5a5

I didn't change the name.
Flags: needinfo?(benjamin)
Some questions:

* What benefit is this to someone whose Flash crashes are not caused by protected mode?

* If the user has been presented a choice - or the consequence of a choice made for them - to decrease security protection, is the gravity of this decision conveyed by the current messaging herein?
Turning off protected mode decreases the general Flash crash and hang rate by more than half. We typically don't know whether a particular hang is caused by protected mode, but if it's not then this change may not benefit an individual user.

Users are not presented with a choice, just a notification, because we believe that users are unable to make an informed decision about this, and so we are making this decision as the user agent with the notification. The Learn More link will be the primary place we can provide additional context.
Flags: needinfo?(madhava)
Disabled at https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1809fa5a22

Before doing something this drastic we're going to see if we can embed somebody full-time into an Adobe office to fix the most prevalent hang bugs.
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.