Last Comment Bug 805591 - Add UI to be shown when plugin is unresponsive
: Add UI to be shown when plugin is unresponsive
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: x86 All
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: Aaron Klotz [:aklotz]
:
:
Mentors:
https://wiki.mozilla.org/Features/Fir...
: 794690 (view as bug list)
Depends on: 827330 828034 829909 845491 891035
Blocks: 801806 822051 830910 818307 833560 834127
  Show dependency treegraph
 
Reported: 2012-10-25 13:19 PDT by Aaron Klotz [:aklotz]
Modified: 2013-07-14 03:30 PDT (History)
28 users (show)
philringnalda: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
20+


Attachments
Plugin Hang UI Prototype (46.11 KB, patch)
2012-11-12 10:19 PST, Aaron Klotz [:aklotz]
no flags Details | Diff | Splinter Review
Raw Win32 implementation of Plugin Hang UI (61.88 KB, patch)
2012-11-19 14:26 PST, Aaron Klotz [:aklotz]
no flags Details | Diff | Splinter Review
Plugin Hang UI Patch (72.96 KB, patch)
2012-11-22 08:19 PST, Aaron Klotz [:aklotz]
netzen: review+
Details | Diff | Splinter Review
Plugin Hang UI including telemetry, bug fixes and Win32 review feedback (76.61 KB, patch)
2012-12-04 10:58 PST, Aaron Klotz [:aklotz]
benjamin: review-
Details | Diff | Splinter Review
Plugin Hang UI incorporating bsmedberg's review feedback and UI feedback from shorlander (77.00 KB, patch)
2012-12-21 14:33 PST, Aaron Klotz [:aklotz]
benjamin: review+
Details | Diff | Splinter Review
Rebased patch (76.91 KB, patch)
2013-01-03 13:31 PST, Aaron Klotz [:aklotz]
aklotz: review+
vladan.bugzilla: checkin+
Details | Diff | Splinter Review
Rebased patch v2 (76.92 KB, patch)
2013-01-04 14:03 PST, Aaron Klotz [:aklotz]
aklotz: review+
Details | Diff | Splinter Review

Description Aaron Klotz [:aklotz] 2012-10-25 13:19:40 PDT
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.
Comment 1 Aaron Klotz [:aklotz] 2012-11-12 10:19:19 PST
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.
Comment 2 Aaron Klotz [:aklotz] 2012-11-19 14:26:43 PST
Created attachment 683308 [details] [diff] [review]
Raw Win32 implementation of Plugin Hang UI
Comment 3 Mozilla RelEng Bot 2012-11-19 15:30:36 PST
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 Mozilla RelEng Bot 2012-11-20 12:45:35 PST
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 5 Vladan Djeric (:vladan) 2012-11-20 13:03:36 PST
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?
Comment 6 Aaron Klotz [:aklotz] 2012-11-22 08:19:22 PST
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
Comment 7 Brian R. Bondy [:bbondy] 2012-11-25 18:29:33 PST
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.
Comment 8 Aaron Klotz [:aklotz] 2012-11-26 08:15:56 PST
Yeah, I plan to send it over to bsmedberg to review the plugin stuff.
Comment 9 Vladan Djeric (:vladan) 2012-11-27 10:36:24 PST
It just occurred to me that we don't have Telemetry to track user responses. Can you add a histogram?
Comment 10 Benjamin Smedberg [:bsmedberg] 2012-11-27 10:41:36 PST
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 (dormant account) 2012-11-27 13:16:14 PST
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 12 Brian R. Bondy [:bbondy] 2012-11-30 08:42:04 PST
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}
Comment 13 Mozilla RelEng Bot 2012-11-30 10:15:34 PST
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
Comment 14 Aaron Klotz [:aklotz] 2012-12-04 10:58:05 PST
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.
Comment 15 Mozilla RelEng Bot 2012-12-04 14:30:40 PST
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
Comment 16 John Schoenick [:johns] 2012-12-04 15:37:49 PST
*** Bug 794690 has been marked as a duplicate of this bug. ***
Comment 17 Benjamin Smedberg [:bsmedberg] 2012-12-19 11:10:05 PST
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.
Comment 18 Aaron Klotz [:aklotz] 2012-12-21 14:33:40 PST
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?
Comment 19 Benjamin Smedberg [:bsmedberg] 2013-01-03 05:17:23 PST
Yes, tests can be a separate patch.
Comment 21 Vladan Djeric (:vladan) 2013-01-03 19:25:14 PST
Comment on attachment 697630 [details] [diff] [review]
Rebased patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6acebd9c9d5
Comment 23 Masatoshi Kimura [:emk] 2013-01-03 20:16:27 PST
Hm, tryserver doesn't enable warnings-as-errors?
Comment 24 Masatoshi Kimura [:emk] 2013-01-03 20:18:50 PST
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'
Comment 25 Masatoshi Kimura [:emk] 2013-01-03 20:22:53 PST
Filed bug 826597 for fixing tinderbox configuration.
Comment 26 Ted Mielczarek [:ted.mielczarek] 2013-01-04 05:55:46 PST
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.
Comment 27 Aaron Klotz [:aklotz] 2013-01-04 14:03:37 PST
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
Comment 29 Robert Kaiser 2013-01-04 15:28:18 PST
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).
Comment 30 Phil Ringnalda (:philor) 2013-01-05 16:28:10 PST
https://hg.mozilla.org/mozilla-central/rev/1308210aefb4
Comment 31 Vladan Djeric (:vladan) 2013-01-13 20:31:29 PST
(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).
Comment 32 Vladan Djeric (:vladan) 2013-01-13 20:32:34 PST
D'oh 2:1 ratio of wait vs kill plugin
Comment 33 Robert Kaiser 2013-01-14 05:58:24 PST
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).
Comment 34 John Schoenick [:johns] 2013-01-14 17:51:45 PST
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?
Comment 35 Aaron Klotz [:aklotz] 2013-01-15 09:10:30 PST
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.
Comment 36 Verdi [:verdi] 2013-02-15 13:23:08 PST
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.
Comment 37 Aaron Klotz [:aklotz] 2013-02-15 14:44:56 PST
(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
Comment 38 Aaron Klotz [:aklotz] 2013-02-15 15:14:37 PST
(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).
Comment 39 Steven Michaud [:smichaud] (Retired) 2013-02-19 08:16:29 PST
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.
Comment 40 Ted Mielczarek [:ted.mielczarek] 2013-02-19 09:38:26 PST
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
Comment 41 Benjamin Smedberg [:bsmedberg] 2013-02-19 09:43:06 PST
You can also get SysInternals Process Explorer and suspend the plugin-container process.

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