The default bug view has changed. See this FAQ.

Add UI to be shown when plugin is unresponsive

RESOLVED FIXED in mozilla20

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla20
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(relnote-firefox 20+)

Details

(Whiteboard: [Snappy:P1], URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
Currently when the Flash plugin stops responding, Firefox also stops responding. Firefox waits a preset amount of time (45 seconds) and then kills the plugin. During this time Firefox appears to be locked and doesn't respond.

This feature will improve the user experience of a plugin not responding by showing UI which allows the user to see that it is the plugin (not Firefox) which is broken, and allowing the user to kill the plugin right away if they don't want to wait.

The current plan is to implement this only for Windows.
(Assignee)

Updated

5 years ago
Assignee: nobody → aklotz
Blocks: 801806

Updated

5 years ago
Whiteboard: [Snappy:P1]
(Assignee)

Comment 1

4 years ago
Created attachment 680689 [details] [diff] [review]
Plugin Hang UI Prototype

Here's the first cut of the plugin hang UI prototype. Please note the following:

* To use this in a debug build, the dom.ipc.plugins.timeoutSecs and dom.ipc.plugins.hangUITimeoutSecs preferences must be manually set. dom.ipc.plugins.hangUITimeoutSecs is the timeout before displaying the UI, dom.ipc.plugins.timeoutSecs is the timeout that is used once the user has elected to wait.
* The patch inserts a sleep into the nptest.dll plugin such that a mouse click induces a 20 second hang.
* Some assertions are disabled by this patch for testing purposes as they annoyingly fire while a plugin is hung and disrupt the UI with dialog boxes.
* There probably needs to be some adjustments to the semantics of the "don't ask again" checkbox.
(Assignee)

Comment 2

4 years ago
Created attachment 683308 [details] [diff] [review]
Raw Win32 implementation of Plugin Hang UI
Attachment #680689 - Attachment is obsolete: true
Attachment #683308 - Flags: feedback?(vdjeric)

Comment 3

4 years ago
Try run for 08089ed26ac1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=08089ed26ac1
Results (out of 4 total builds):
    exception: 2
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/aklotz@mozilla.com-08089ed26ac1

Comment 4

4 years ago
Try run for c6033e55c1e0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c6033e55c1e0
Results (out of 26 total builds):
    success: 18
    warnings: 2
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/aklotz@mozilla.com-c6033e55c1e0
Comment on attachment 683308 [details] [diff] [review]
Raw Win32 implementation of Plugin Hang UI

This is a partial review. Someone more experienced with Windows development needs to review this


>+#include "hangui/PluginHangUIParent.h"

You won't need the relative path if you tweak LOCAL_INCLUDES in the makefile

>+#ifdef XP_WIN
>+    } else if (!strcmp(aPref, kHangUITimeoutPref)) {
>+      // The timeout value used by the parent for the Hang UI
>+      int32_t timeoutSecs = Preferences::GetInt(kHangUITimeoutPref, 0);
>+      PluginModuleParent* module = static_cast<PluginModuleParent*>(aModule);
>+      if (timeoutSecs > 0) {
>+          if (!module->mHangUIEnabled) {
>+              module->mHangUIEnabled = true;
>+              Preferences::UnregisterCallback(TimeoutChanged, kChildTimeoutPref, aModule);

add a short comment explaining how we're monitoring two prefs to set the timeout value and how kHangUITimeoutPref's value is used unless it's zero

>+    mHangUIParent = new PluginHangUIParent();
>+    const InfallibleTArray<PPluginInstanceParent*>& managees =
>+        ManagedPPluginInstanceParent();

Add a check mHangUIParent == nullptr

>+void
>+PluginModuleParent::CancelHangUI()
>+{
>+    if (mHangUIParent && mHangUIParent->IsShowing()) {
>+        TimeoutChanged(kHangUITimeoutPref, this);
>+        mHangUIParent->Cancel();
>+    }
>+}

Add a comment explaining why you're changing timeout here + handle edge-case:

(2:42:57 PM) vladan: what if kHangUITimeoutPref > kChildTimeoutPref
(2:43:10 PM) vladan: or kChildTimeoutPref is very small
...
(2:45:07 PM) aklotz: That's a good point. As written, in that case the UI would be displayed and then dismissed quickly.

>diff --git a/dom/plugins/ipc/PluginModuleParent.h b/dom/plugins/ipc/PluginModuleParent.h
>+    bool
>+    LaunchHangUI();

Nitpick: Add a comment explaining the meaning of the return value

>diff --git a/dom/plugins/ipc/hangui/HangUIDlg.h b/dom/plugins/ipc/hangui/HangUIDlg.h

>+#ifndef dom_plugins_ipc_hangui_HangUIDlg_h
>+#define dom_plugins_ipc_hangui_HangUIDlg_h

Nit: I think we generally use #ifndef <namespace name>_<file name>_h

>diff --git a/dom/plugins/ipc/hangui/MiniShmBase.cpp b/dom/plugins/ipc/hangui/MiniShmBase.cpp
>new file mode 100644

- Add some comments to the declarations of any major new functions describing their functionality
- If we're going to add the plugin hang UI on other platforms we'll have to isolate the code that deals that calls platform-specific APIs into separate files

>diff --git a/dom/plugins/ipc/hangui/MiniShmBase.h b/dom/plugins/ipc/hangui/MiniShmBase.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/plugins/ipc/hangui/MiniShmBase.h
>+  struct MiniShmInit
>+  {
>+    enum identifier_t
>+    {
>+      identifier = RESERVED_CODE_INIT
>+    };
>+    HANDLE    mParentEvent;
>+    HANDLE    mParentGuard;
>+    HANDLE    mChildEvent;
>+    HANDLE    mChildGuard;
>+  };

Nit: We don't usually indent-align member variable names

>diff --git a/dom/plugins/ipc/hangui/plugin-hang-ui.exe.manifest b/dom/plugins/ipc/hangui/plugin-hang-ui.exe.manifest
>+  <ms_asmv3:trustInfo xmlns:ms_asmv3="urn:schemas-microsoft-com:asm.v3">
>+    <ms_asmv3:security>
>+      <ms_asmv3:requestedPrivileges>
>+        <ms_asmv3:requestedExecutionLevel level="asInvoker" uiAccess="false" />
>+      </ms_asmv3:requestedPrivileges>
>+    </ms_asmv3:security>
>+  </ms_asmv3:trustInfo>

Would uiAccess be true if we wanted to focus/unminimize the Firefox window before showing the message?
Attachment #683308 - Flags: feedback?(vdjeric)
(Assignee)

Comment 6

4 years ago
Created attachment 684428 [details] [diff] [review]
Plugin Hang UI Patch

This latest patch is green across the board on try.
https://tbpl.mozilla.org/?tree=Try&rev=192a654f8ad6
Build available at
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/aklotz@mozilla.com-192a654f8ad6
Attachment #683308 - Attachment is obsolete: true
Attachment #684428 - Flags: review?(netzen)
This might be a couple days for a review, you should also have someone else review the plug-in bits as I don't feel comfortable reviewing that.
(Assignee)

Comment 8

4 years ago
Yeah, I plan to send it over to bsmedberg to review the plugin stuff.
It just occurred to me that we don't have Telemetry to track user responses. Can you add a histogram?
If the user chooses to kill the plugin or it times out and kills automatically, we should still be getting the hang report via crash-stats. So really all that we'd need to measure via telemetry is the times when the UI showed up and the user chose *not* to kill the plugin, and then it recovered. Right?

Comment 11

4 years ago
To gauge success of this feature, it would be good to add a flag to chromehangs where the user explicitly decided to wait for flash.
Comment on attachment 684428 [details] [diff] [review]
Plugin Hang UI Patch

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

I mainly only looked at the Win32 code and didn't actually run it, but it looks good.  Just to be clear the r+ only covers those bits and not the entire patch for landing.

::: browser/installer/package-manifest.in
@@ +78,5 @@
>  #else
>  @BINPATH@/@MOZ_CHILD_PROCESS_NAME@
>  #endif
>  #ifdef XP_WIN32
> +@BINPATH@/@MOZ_HANGUI_PROCESS_NAME@

I *think* this should also be listed in:
browser\installer\removed-files.in

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +229,5 @@
> +#ifdef XP_WIN
> +    } else if (!strcmp(aPref, kHangUITimeoutPref)) {
> +      // The timeout value used by the parent for the Hang UI
> +      int32_t timeoutSecs = Preferences::GetInt(kHangUITimeoutPref, 0);
> +      int32_t minDispSecs = Preferences::GetInt(kHangUIMinDisplayPref, 10U);

nit:
- 10U);
+ 10);

::: dom/plugins/ipc/hangui/HangUIDlg.rc
@@ +9,5 @@
> +
> +/////////////////////////////////////////////////////////////////////////////
> +//
> +// Dialog
> +//

I think you also need:
CREATEPROCESS_MANIFEST_RESOURCE_ID RT_MANIFEST "plugin-hang-ui.exe.manifest"

::: dom/plugins/ipc/hangui/Makefile.in
@@ +38,5 @@
> +
> +DEFINES += \
> +  -DUNICODE \
> +  -D_UNICODE \
> +  -DMOZ_HANGUI_PROCESS_NAME=\"$(MOZ_HANGUI_PROCESS_NAME)\" \

Not sure if needed, but we do this for another similar program:
-DNS_NO_XPCOM \

@@ +52,5 @@
> +
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk
> +
> +MOZ_GLUE_LDFLAGS =

Not sure if needed but we do this for another similar program:
MOZ_GLUE_PROGRAM_LDFLAGS =

::: dom/plugins/ipc/hangui/PluginHangUIChild.cpp
@@ +112,5 @@
> +  return FALSE;
> +}
> +
> +INT_PTR
> +PluginHangUIChild::HangUIDlgProc(HWND aDlgHandle,

nit: here and elsewhere, join subsequent lines until length > 80 chars, then split before 80 chars.

@@ +232,5 @@
> +bool
> +PluginHangUIChild::RecvCancel()
> +{
> +  if (mDlgHandle) {
> +    PostMessage(mDlgHandle,WM_CLOSE,0,0);

nit: space after each ,

@@ +243,5 @@
> +{
> +  if (!SetMainThread()) {
> +    return false;
> +  }
> +  DWORD waitResult = WaitForMultipleObjectsEx(1,

nit: Might as well use WaitForSingleObjectEx

@@ +275,5 @@
> +} // namespace plugins
> +} // namespace mozilla
> +
> +int
> +wmain(int argc, wchar_t *argv[], wchar_t *envp[])

nit: you can just remove envp since you aren't using it and that's an acceptable entry point signature

@@ +277,5 @@
> +
> +int
> +wmain(int argc, wchar_t *argv[], wchar_t *envp[])
> +{
> +  mozilla::plugins::PluginHangUIChild hangui;

You have the manfiest setup for newer user interface, but I think you're getting old style controls.
I think you should also be linking to comCtl32.lib and also calling InitCommonControls.

::: dom/plugins/ipc/hangui/plugin-hang-ui.exe.manifest
@@ +27,5 @@
> +    </ms_asmv3:security>
> +  </ms_asmv3:trustInfo>
> +  <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
> +    <application>
> +      <supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}" />

nit: Not needed, but please also explicitly specify Vista support here:
{e2011457-1546-43c5-a5fe-008deee3d3f0}
Attachment #684428 - Flags: review?(netzen) → review+

Comment 13

4 years ago
Try run for b7f76d82b75d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b7f76d82b75d
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/aklotz@mozilla.com-b7f76d82b75d
(Assignee)

Comment 14

4 years ago
Created attachment 688358 [details] [diff] [review]
Plugin Hang UI including telemetry, bug fixes and Win32 review feedback

This latest revision incorporates feedback from bbondy's Win32 review and adds telemetry as suggested by vladan. It also fixes some bugs that I found.

Sending to bsmedberg to review the plugin aspect of this patch.
Attachment #684428 - Attachment is obsolete: true
Attachment #688358 - Flags: ui-review?(shorlander)
Attachment #688358 - Flags: review?(benjamin)

Comment 15

4 years ago
Try run for f5d8fdf4f29a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f5d8fdf4f29a
Results (out of 26 total builds):
    success: 25
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/aklotz@mozilla.com-f5d8fdf4f29a
Duplicate of this bug: 794690
(Assignee)

Updated

4 years ago
Blocks: 818307
(Assignee)

Updated

4 years ago
Blocks: 822051
Comment on attachment 688358 [details] [diff] [review]
Plugin Hang UI including telemetry, bug fixes and Win32 review feedback

I'm sorry this took so long!

>diff --git a/dom/plugins/ipc/hangui/defs.mk b/dom/plugins/ipc/hangui/defs.mk
>new file mode 100644
>--- /dev/null
>+++ b/dom/plugins/ipc/hangui/defs.mk
>@@ -0,0 +1,6 @@
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+MOZ_HANGUI_PROCESS_NAME := plugin-hang-ui$(BIN_SUFFIX)

I really don't think we need to make this configurable or a variable. Can we just hardcode "plugin-hang-ui" in the places that matter and avoid this defs.mk file altogether? This will avoid a bunch of DEFINES goop in browser/installer/Makefile.in and elsewhere; we could just put "plugin-hang-ui$(BIN_SUFFIX)" directly in package-manifest.in, for example.

>diff --git a/dom/plugins/ipc/hangui/PluginHangUIParent.h b/dom/plugins/ipc/hangui/PluginHangUIParent.h

Originally I was confused why these were named "Parent" and "Child", since normally those are used for IPDL actors. Please add a significant doccomment to these classes explaining that they are a matched pair, even though they aren't IPDL actors at all.

>+++ b/dom/plugins/ipc/hangui/PluginHangUIParent.h

>+#include "nsstring.h"

nit capitalization, "nsString.h"

>+class PluginHangUIParent : public MiniShmObserver

This class could use a doccomment explaining what its purpose is.

>diff --git a/dom/plugins/ipc/PluginModuleParent.cpp b/dom/plugins/ipc/PluginModuleParent.cpp

>+
>+bool
>+PluginModuleParent::GetPluginName(PluginInstanceParent* aPluginInst,
>+                                  nsAString& aPluginName)
>+{

This method is too complicated. You are in PluginModuleParent, so you can just use mPlugin to get the nsNPAPIPlugin, and then get the tag from that. Then you don't need to pass around the instance either.

>+bool
>+PluginModuleParent::LaunchHangUI()
>+{
>+    if (mHangUIParent) {
>+        if (mHangUIParent->IsShowing()) {
>+            // We've already shown the UI but the timeout has expired again.
>+            return false;
>+        }
>+        if (mHangUIParent->DontShowAgain()) {
>+            return !mHangUIParent->WasLastHangStopped();
>+        }
>+        delete mHangUIParent;
>+        mHangUIParent = nullptr;
>+    }
>+    mHangUIParent = new PluginHangUIParent();
>+    if (!mHangUIParent) {
>+        return false;
>+    }

"new" is infallible by default, so remove this null check.

>+    const InfallibleTArray<PPluginInstanceParent*>& managees =
>+        ManagedPPluginInstanceParent();
>+    // This array should not be empty because if a plugin is hanging,
>+    // then by definition there should be at least one instance
>+    NS_ASSERTION(!managees.IsEmpty(), "Expected at least one plugin instance");
>+    PluginInstanceParent *anInstance =
>+        static_cast<PluginInstanceParent*>(managees.ElementAt(0));

And now you don't need all this managee stuff! Yay.

>diff --git a/dom/plugins/ipc/hangui/Makefile.in b/dom/plugins/ipc/hangui/Makefile.in

>+MODULE = dom

this should be unncessary nowadays, please remove it

>+STATIC_LIBRARY_NAME = pluginhanguiparent_s
>+FORCE_STATIC_LIB = 1
>+MOZILLA_INTERNAL_API = 1
>+EXPORT_LIBRARY = 1

This directory is a bit confusing because it is producing both a static library (linked into libxul) and a standalone program. Perhaps it makes more sense to move the linked-into-libxul bits into dom/plugins/ipc and have this directory just for the standalone program?

>+$(PROGOBJS) : DEFINES += \
>+  -DNS_NO_XPCOM \
>+  $(NULL)
>+
>+$(PROGOBJS) : STL_FLAGS = \
>+  -D_HAS_EXCEPTIONS=0 \
>+  $(NULL)

We avoid target-specific variables in our build system because they propagate weirdly in GNU make and will confuse future build system plans.

>diff --git a/dom/plugins/ipc/hangui/MiniShmChild.cpp b/dom/plugins/ipc/hangui/MiniShmChild.cpp

What led you to choose shared memory over just sending the data via pipes? Shared memory seems like a pretty big hammer for the job. Note: I have not reviewed, nor would I be a good reviewer for the actual SHM code. If bbondy is comfortable doing that review I'm fine with it, or else cjones or somebody else should be asked.

>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -309,16 +309,21 @@ NON_DIST_FILES = \
>   $(NULL)
> 
> UPLOAD_EXTRA_FILES += gecko-unsigned-unaligned.apk
> 
> include $(MOZILLA_DIR)/ipc/app/defs.mk
> 
> DIST_FILES += $(MOZ_CHILD_PROCESS_NAME)
> 
>+ifeq (WINNT,$(OS_ARCH))
>+include $(MOZILLA_DIR)/dom/plugins/ipc/hangui/defs.mk
>+DIST_FILES += $(MOZ_HANGUI_PROCESS_NAME)
>+endif

This really doesn't seem like it should be here. This should be in the makefile that ships the hangui process (dom/plugins/ipc/hangui).

Finally, I think we need another crash annotation. Specifically we want to know for every hang report that is submitted, whether the user chose to kill it using the UI, and how many seconds they waited. I think one annotation for how many seconds they waited would tell us both things.

Are there unit tests for this? In particular, I want to make sure that the following things are true:

* That the hang UI shows on a hang
* That if the plugin recovers, the hang UI goes away
* If the user chooses to kill the plugin, we get a useful hang report
* If the UI times out, we get a useful hang report

I'm going to mark r- just for the build system issues. The rest are just nits.
Attachment #688358 - Flags: review?(benjamin) → review-
(Assignee)

Comment 18

4 years ago
Created attachment 695000 [details] [diff] [review]
Plugin Hang UI incorporating bsmedberg's review feedback and UI feedback from shorlander

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> >diff --git a/dom/plugins/ipc/hangui/defs.mk b/dom/plugins/ipc/hangui/defs.mk
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/plugins/ipc/hangui/defs.mk
> >@@ -0,0 +1,6 @@
> >+# This Source Code Form is subject to the terms of the Mozilla Public
> >+# License, v. 2.0. If a copy of the MPL was not distributed with this
> >+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >+
> >+MOZ_HANGUI_PROCESS_NAME := plugin-hang-ui$(BIN_SUFFIX)
> 
> I really don't think we need to make this configurable or a variable. Can we
> just hardcode "plugin-hang-ui" in the places that matter and avoid this
> defs.mk file altogether? This will avoid a bunch of DEFINES goop in
> browser/installer/Makefile.in and elsewhere; we could just put
> "plugin-hang-ui$(BIN_SUFFIX)" directly in package-manifest.in, for example.

Done.

> >diff --git a/dom/plugins/ipc/hangui/PluginHangUIParent.h b/dom/plugins/ipc/hangui/PluginHangUIParent.h
> 
> Originally I was confused why these were named "Parent" and "Child", since
> normally those are used for IPDL actors. Please add a significant doccomment
> to these classes explaining that they are a matched pair, even though they
> aren't IPDL actors at all.
> 

Done.

> >+++ b/dom/plugins/ipc/hangui/PluginHangUIParent.h
> 
> >+#include "nsstring.h"
> 
> nit capitalization, "nsString.h"
> 

Done.

> >+class PluginHangUIParent : public MiniShmObserver
> 
> This class could use a doccomment explaining what its purpose is.
> 

Done.

> >diff --git a/dom/plugins/ipc/PluginModuleParent.cpp b/dom/plugins/ipc/PluginModuleParent.cpp
> 
> >+
> >+bool
> >+PluginModuleParent::GetPluginName(PluginInstanceParent* aPluginInst,
> >+                                  nsAString& aPluginName)
> >+{
> 
> This method is too complicated. You are in PluginModuleParent, so you can
> just use mPlugin to get the nsNPAPIPlugin, and then get the tag from that.
> Then you don't need to pass around the instance either.
> 

Done.

> >+bool
> >+PluginModuleParent::LaunchHangUI()
> >+{
> >+    if (mHangUIParent) {
> >+        if (mHangUIParent->IsShowing()) {
> >+            // We've already shown the UI but the timeout has expired again.
> >+            return false;
> >+        }
> >+        if (mHangUIParent->DontShowAgain()) {
> >+            return !mHangUIParent->WasLastHangStopped();
> >+        }
> >+        delete mHangUIParent;
> >+        mHangUIParent = nullptr;
> >+    }
> >+    mHangUIParent = new PluginHangUIParent();
> >+    if (!mHangUIParent) {
> >+        return false;
> >+    }
> 
> "new" is infallible by default, so remove this null check.
> 

Done.

> >+    const InfallibleTArray<PPluginInstanceParent*>& managees =
> >+        ManagedPPluginInstanceParent();
> >+    // This array should not be empty because if a plugin is hanging,
> >+    // then by definition there should be at least one instance
> >+    NS_ASSERTION(!managees.IsEmpty(), "Expected at least one plugin instance");
> >+    PluginInstanceParent *anInstance =
> >+        static_cast<PluginInstanceParent*>(managees.ElementAt(0));
> 
> And now you don't need all this managee stuff! Yay.

Done. Yay!
 
> >diff --git a/dom/plugins/ipc/hangui/Makefile.in b/dom/plugins/ipc/hangui/Makefile.in
> 
> >+MODULE = dom
> 
> this should be unncessary nowadays, please remove it
> 

Done.

> >+STATIC_LIBRARY_NAME = pluginhanguiparent_s
> >+FORCE_STATIC_LIB = 1
> >+MOZILLA_INTERNAL_API = 1
> >+EXPORT_LIBRARY = 1
> 
> This directory is a bit confusing because it is producing both a static
> library (linked into libxul) and a standalone program. Perhaps it makes more
> sense to move the linked-into-libxul bits into dom/plugins/ipc and have this
> directory just for the standalone program?
> 

Done. *Parent classes have been moved to dom/plugins/ipc, allowing for this simplification.

> >+$(PROGOBJS) : DEFINES += \
> >+  -DNS_NO_XPCOM \
> >+  $(NULL)
> >+
> >+$(PROGOBJS) : STL_FLAGS = \
> >+  -D_HAS_EXCEPTIONS=0 \
> >+  $(NULL)
> 
> We avoid target-specific variables in our build system because they
> propagate weirdly in GNU make and will confuse future build system plans.
> 

Done.

> >diff --git a/dom/plugins/ipc/hangui/MiniShmChild.cpp b/dom/plugins/ipc/hangui/MiniShmChild.cpp
> 
> What led you to choose shared memory over just sending the data via pipes?
> Shared memory seems like a pretty big hammer for the job. Note: I have not
> reviewed, nor would I be a good reviewer for the actual SHM code. If bbondy
> is comfortable doing that review I'm fine with it, or else cjones or
> somebody else should be asked.
> 

I chose SHM for a couple of reasons. One is that I didn't want to use anonymous pipes since they don't allow for OVERLAPPED I/O. While I could have used named pipes, I opted for SHM because I suspected that we might need to transfer some image data over to the child process. My feedback from UX is indicating that we will need to add an icon to the UI.

bbondy reviewed the SHM code when he gave the r+ for patch 684428 (a previous revision of this patch).

> >diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
> >--- a/toolkit/mozapps/installer/packager.mk
> >+++ b/toolkit/mozapps/installer/packager.mk
> >@@ -309,16 +309,21 @@ NON_DIST_FILES = \
> >   $(NULL)
> > 
> > UPLOAD_EXTRA_FILES += gecko-unsigned-unaligned.apk
> > 
> > include $(MOZILLA_DIR)/ipc/app/defs.mk
> > 
> > DIST_FILES += $(MOZ_CHILD_PROCESS_NAME)
> > 
> >+ifeq (WINNT,$(OS_ARCH))
> >+include $(MOZILLA_DIR)/dom/plugins/ipc/hangui/defs.mk
> >+DIST_FILES += $(MOZ_HANGUI_PROCESS_NAME)
> >+endif
> 
> This really doesn't seem like it should be here. This should be in the
> makefile that ships the hangui process (dom/plugins/ipc/hangui).
> 

Done.

> Finally, I think we need another crash annotation. Specifically we want to
> know for every hang report that is submitted, whether the user chose to kill
> it using the UI, and how many seconds they waited. I think one annotation
> for how many seconds they waited would tell us both things.
> 

Done. I made some changes such that both code paths (hang UI or termination due to timeout) go through the same hang report code and annotate as necessary.

> Are there unit tests for this? In particular, I want to make sure that the
> following things are true:
> 
> * That the hang UI shows on a hang
> * That if the plugin recovers, the hang UI goes away
> * If the user chooses to kill the plugin, we get a useful hang report
> * If the UI times out, we get a useful hang report

Agreed. Can I post that as a follow-up patch?
Attachment #688358 - Attachment is obsolete: true
Attachment #688358 - Flags: ui-review?(shorlander)
Attachment #695000 - Flags: review?(benjamin)
(Assignee)

Updated

4 years ago
Attachment #695000 - Flags: ui-review?(shorlander)
Attachment #695000 - Flags: review?(benjamin) → review+
Yes, tests can be a separate patch.
(Assignee)

Comment 20

4 years ago
Created attachment 697630 [details] [diff] [review]
Rebased patch

Rebased patch, carrying forward r+
Latest try build:
https://tbpl.mozilla.org/?tree=Try&rev=01ce815e3b14
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/aklotz@mozilla.com-01ce815e3b14/try-win32/
Attachment #695000 - Attachment is obsolete: true
Attachment #695000 - Flags: ui-review?(shorlander)
Attachment #697630 - Flags: review+
Attachment #697630 - Flags: checkin?(vdjeric)
(Assignee)

Updated

4 years ago
Whiteboard: [Snappy:P1] → [Snappy:P1][leave open]
Comment on attachment 697630 [details] [diff] [review]
Rebased patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6acebd9c9d5
Attachment #697630 - Flags: checkin?(vdjeric) → checkin+
Backed out because of build failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a6acebd9c9d5

https://hg.mozilla.org/integration/mozilla-inbound/rev/1940c5a8eec5
Hm, tryserver doesn't enable warnings-as-errors?
Indeed the warning was ignored on try:

make.py[6]: Leaving directory 'e:\builds\moz2_slave\try-w32\build\obj-firefox\dom\interfaces/apps'
evaluation from e:\builds\moz2_slave\try-w32\build\config\rules.mk:1571:4:4:0$ nsinstall nsinstall -t -m 644 "_xpidlgen/plugin.xpt" "../../../dist/bin/components"
e:\builds\moz2_slave\try-w32\build\config\rules.mk:1038:0$ rc.exe  -r -DNO_NSPR_10_SUPPORT -DNS_NO_XPCOM  -DEXCLUDE_SKIA_DEPENDENCIES  -DUNICODE -D_UNICODE -DNOMINMAX -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -D_SECURE_ATL -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DOS_WIN=1 -DWIN32 -D_WIN32 -D_WINDOWS -DWIN32_LEAN_AND_MEAN  -DCOMPILER_MSVC -Ie:/builds/moz2_slave/try-w32/build/ipc/chromium/src -Ie:/builds/moz2_slave/try-w32/build/ipc/glue -I../../../../ipc/ipdl/_ipdlheaders  -Ie:/builds/moz2_slave/try-w32/build/dom/plugins/ipc/hangui -I. -I../../../../dist/include  -Ie:/builds/moz2_slave/try-w32/build/obj-firefox/dist/include/nspr -Ie:/builds/moz2_slave/try-w32/build/obj-firefox/dist/include/nss     -Fomodule.res e:/builds/moz2_slave/try-w32/build/obj-firefox/dom/plugins/ipc/hangui/module.rc
make.py[6]: Leaving directory 'e:\builds\moz2_slave\try-w32\build\obj-firefox\dom\audiochannel'
PluginHangUIChild.cpp

e:\builds\moz2_slave\try-w32\build\dom\plugins\ipc\hangui\HangUIDlg.h(15) : warning C4005: 'IDC_ICON' : macro redefinition

        c:\tools\sdks\v7.0\include\winuser.h(8833) : see previous definition of 'IDC_ICON'

Microsoft (R) Windows (R) Resource Compiler Version 6.1.7600.16385

Copyright (C) Microsoft Corporation.  All rights reserved.


make.py[6]: Leaving directory 'e:\builds\moz2_slave\try-w32\build\obj-firefox\dom\interfaces/devicestorage'
Filed bug 826597 for fixing tinderbox configuration.
Pushing to try builds whatever changesets you push it. If the try push didn't contain your WARNINGS_AS_ERRORS changes then it wouldn't have been used.
(Assignee)

Comment 27

4 years ago
Created attachment 698078 [details] [diff] [review]
Rebased patch v2

Carrying forward r+. Here's the try push for this patch:
https://tbpl.mozilla.org/?tree=Try&rev=b0437882b021
Attachment #697630 - Attachment is obsolete: true
Attachment #698078 - Flags: review+
Attachment #698078 - Flags: checkin?(vdjeric)
(Assignee)

Updated

4 years ago
Whiteboard: [Snappy:P1][leave open] → [Snappy:P1]
https://hg.mozilla.org/integration/mozilla-inbound/rev/1308210aefb4

Comment 29

4 years ago
Benjamin, we should watch closely how this affects the volume of hang reports we get - right now on your servers, I guess.

We also should inform the input / SUMO / user feedback people of this, as we should monitor how annoying this UI might become for users (or not).
Attachment #698078 - Flags: checkin?(vdjeric)
https://hg.mozilla.org/mozilla-central/rev/1308210aefb4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 827330
(In reply to Taras Glek (:taras) from comment #11)
> To gauge success of this feature, it would be good to add a flag to
> chromehangs where the user explicitly decided to wait for flash.

Based on 53k user choices: http://tinyurl.com/bvned9e

Stop plugin: 5%
Keep waiting for plugin: 10%
Plugin recovered on its own before user responded: 85%
---
Users checked "Don't ask again" in 1.5% of hangs

Since the plugin recovers in 85% of cases and we have a 2:1 ratio of wait vs continue, it seems to me the threshold for showing the UI should be set higher than the current 5 seconds.

At some point, we should investigate scaling the threshold based on the machine specs or maybe even past outcomes (plugin recovered vs killed vs waited).
D'oh 2:1 ratio of wait vs kill plugin

Comment 33

4 years ago
5 seconds always sounded pretty low to me for this, given that we have enough that time out at the 45s forced-kill timeout we've had for ages.

Do we know how the rate of those warnings being shown vs. the rates of the forced kills before and after the introduction of this UI? (Benjamin should have the data for the latter, as for Nightly and Aurora the hang reports are sent to his server atm).
Does this 85% is include 45s hang kills or cases where the user kills the browser? Do we know how long the average hang-recovery is?
(Assignee)

Comment 35

4 years ago
The 85% solely represents the plugin resuming execution on its own and the Hang UI being cancelled. The user did not respond and the plugin was not terminated.

We don't have any telemetry for the duration of these stalls, but already annotate Flash crash reports with the duration when the user elects to terminate, so it would be easy to add.

Updated

4 years ago
Blocks: 830910
Depends on: 829909
Depends on: 828034
(Assignee)

Updated

4 years ago
Blocks: 833560
(Assignee)

Updated

4 years ago
Blocks: 834127
relnote-firefox: --- → ?

Comment 36

4 years ago
I'm trying to document this for support. Is there a way that I can trigger a plugin hang so that I can see what this UI looks like and how it works? Thanks.
(Assignee)

Comment 37

4 years ago
(In reply to Verdi [:verdi] from comment #36)
> I'm trying to document this for support. Is there a way that I can trigger a
> plugin hang so that I can see what this UI looks like and how it works?
> Thanks.

This one works best on Nightly (a fix still needs to be uplifted for Aurora) but it is reliable:

1. Go to http://www.tink.ws/examples/spark/ListTest/
2. Click the "percentWidth 20" button
3. Click the "percentWidth 100" button
4. Then click somewhere on the tab strip
(Assignee)

Comment 38

4 years ago
(In reply to Aaron Klotz [:aklotz] from comment #37)
> (In reply to Verdi [:verdi] from comment #36)
> > I'm trying to document this for support. Is there a way that I can trigger a
> > plugin hang so that I can see what this UI looks like and how it works?
> > Thanks.
> 
> This one works best on Nightly (a fix still needs to be uplifted for Aurora)
> but it is reliable:
> 
> 1. Go to http://www.tink.ws/examples/spark/ListTest/
> 2. Click the "percentWidth 20" button
> 3. Click the "percentWidth 100" button
> 4. Then click somewhere on the tab strip

Also please note that this repro is good but isn't 100% reliable. Approximately 20% of the time it doesn't trigger the Hang UI. This isn't a bug in Firefox, unfortunately it occasionally hangs in a way that we can't detect due to some internal Windows stuff. We can't work around it (or at least not yet).
By the way, Ted Mielczarek has written a Crashme extension:

https://quality.mozilla.org/docs/misc/tips-and-tricks/#crash
http://code.google.com/p/crashme/downloads/list

I can't remember whether or not it supports hanging.  But if it doesn't, it should be easy to add that capability.
It doesn't, and it doesn't support causing plugin hangs either. You can test this by downloading a test package from one of our builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-21.0a1.en-US.linux-i686.tests.zip
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-21.0a1.en-US.linux-x86_64.tests.zip
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-21.0a1.en-US.mac.tests.zip
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-21.0a1.en-US.win32.tests.zip

and extracting the test plugin binary from there (should be in bin/plugins). I think you may still be able to put that in the "plugins" subdirectory in your install directory. If not you can definitely put it in a "plugins" subdirectory of your profile directory. Then you can simply call the hang API we provide on the test plugin.

For example:
http://people.mozilla.com/~tmielczarek/testplugincrash.html
You can also get SysInternals Process Explorer and suspend the plugin-container process.
relnote-firefox: ? → 20+
Depends on: 845491
Flags: in-testsuite?

Updated

4 years ago
Depends on: 891035
You need to log in before you can comment on or make changes to this bug.