818307 - Add flag to chromehang when user selects "Continue" in Plugin Hang UI

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

In bug 805591, Taras wrote, "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 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Implement a thread safe variant

Since the subsequent patches in this bug require a variant to be created on the HangMonitor thread but accessed on the main thread, I needed a thread-safe version of nsVariant.

I made a new class, nsThreadSafeVariant, to avoid forcing other nsVariant users to pay the price for thread safety when they don't need it and because I know mccr8 is adding cycle collection to nsVariant in bug 931283.

Benjamin, are you okay with these changes or would you favor a different approach?

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Add annotation support to ChromeHangs

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Plugin Hang UI ChromeHang annotations

Comment 4

[Benjamin Smedberg]

Why does this need to be a variant? Variants are almost never the right thing (they are mainly used to pass "any" data through XPCOM boundaries. Can we just use strings or something less heavy?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8448460 [details] [diff] [review]
Part 1: Implement a thread safe variant

That's reasonable. It simplifies a lot of stuff.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add annotation support to ChromeHangs

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Plugin ChromeHang annotations

This patch adds a handler to annotate hangs when they occur. PluginModuleParent registers itself with the hang monitor so that when a hang occurs, a callback implemented by PluginModuleParent is invoked to populate any annotations.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8460600 [details] [diff] [review]
Part 2: Plugin ChromeHang annotations

Benjamin will be away for one more week. John, can you take a look?

Comment 9

[Vladan Djeric (:vladan)]

Comment on attachment 8460599 [details] [diff] [review]
Part 1: Add annotation support to ChromeHangs

Let's postpone these reviews until we've talked to the Fennec team about the future of BHR vs chrome-hangs

Comment 10

[John Schoenick [:johns]]

Comment on attachment 8460600 [details] [diff] [review]
Part 2: Plugin ChromeHang annotations

(In reply to Vladan Djeric (:vladan), PTO Aug 4-5 from comment #9)
> Comment on attachment 8460599 [details] [diff] [review]
> Part 1: Add annotation support to ChromeHangs
> 
> Let's postpone these reviews until we've talked to the Fennec team about the
> future of BHR vs chrome-hangs

I held on to this in case the part 1 stuff got sorted before benjamin returned, but since he's back now, he really should be the one to look at this.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8460599 [details] [diff] [review]
Part 1: Add annotation support to ChromeHangs

As discussed in today's 1:1

Comment 12

[Vladan Djeric (:vladan)]

Comment on attachment 8460599 [details] [diff] [review]
Part 1: Add annotation support to ChromeHangs

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +209,5 @@
>  }
>  
>  class HangReports {
>  public:
> +  struct AnnotationInfo {

Document what AnnotationInfo, HangAnnotations, and HangInfo represent

@@ +212,5 @@
>  public:
> +  struct AnnotationInfo {
> +    AnnotationInfo(uint32_t aHangIndex,
> +                   HangAnnotations* aAnnotations)
> +      : mHangIndex(aHangIndex)

Do you really need mHangIndex? Can't you just keep a counter as a local variable while iterating over mAnnotationInfo?

@@ +220,5 @@
> +      : mHangIndex(aOther.mHangIndex)
> +      , mAnnotations(aOther.mAnnotations)
> +    {}
> +    ~AnnotationInfo() {}
> +    uint32_t                    mHangIndex;

there's an issue with the spacing

::: xpcom/threads/HangMonitor.cpp
@@ +18,3 @@
>  #include "nsXULAppAPI.h"
> +
> +#include <set>

Nit: Apparently standard library includes are now supposed to be before Mozilla includes

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

@@ +135,5 @@
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE;
> +  bool IsEmpty() const MOZ_OVERRIDE;
> +  bool GetEnumerator(Enumerator** aOutEnum) MOZ_OVERRIDE;
> +
> +  typedef std::pair<nsString,nsString> AnnotationType;

Nit: add a space in the middle of "nsString,nsString"

@@ +224,5 @@
> +}
> +
> +bool
> +ChromeHangAnnotationEnumerator::Next(nsAString& aOutName, nsAString& aOutValue)
> +{

Document somewhere why this class doesn't need synchronization

@@ +486,5 @@
>    delete gMonitor;
>    gMonitor = nullptr;
> +
> +#ifdef REPORT_CHROME_HANGS
> +  gAnnotators = nullptr;

maybe add a comment pointing out gAnnotators is a StaticAutoPtr?

@@ +582,5 @@
> +{
> +#ifdef REPORT_CHROME_HANGS
> +  MonitorAutoLock lock(*gMonitor);
> +  MOZ_ASSERT(gAnnotators);
> +  if (!gAnnotators) {

Why both the ASSERT + the nullness check?
Wouldn't we want to faily visibly with a segfault on Nightly, if somehow RegisterAnnotator was called before gAnnotators was allocated in Startup() or after Shutdown()?

::: xpcom/threads/HangMonitor.h
@@ +45,5 @@
> +{
> +public:
> +  virtual ~HangAnnotations() {}
> +
> +  virtual void AddAnnotation(const nsAString& aName, const int32_t aData) = 0;

Too bad we can't use templates to combine the AddAnnotations methods

@@ +82,5 @@
> +void RegisterAnnotator(Annotator& aAnnotator);
> +
> +/**
> + * Registers an Annotator that was previously registered via RegisterAnnotator.
> + */

Document all the params

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) from comment #12)
> > +#include <set>
> 
> Nit: Apparently standard library includes are now supposed to be before
> Mozilla includes

Are we actually ok with standard library containers in gecko now?
A quick check on #developers didn't bring more clarity here.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> Are we actually ok with standard library containers in gecko now?
> A quick check on #developers didn't bring more clarity here.

In general I believe the answer is "no," however they may be (and are) used in code that outlives XPCOM.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

(and outlives NSPR)

Comment 16

[Georg Fritzsche [:gfritzsche]]

Sorry for the delay here Aaron, i will get to this tomorrow.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8460600 [details] [diff] [review]
Part 2: Plugin ChromeHang annotations

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

I think we need to clear up first how this deals with re-entering the stack.

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +389,4 @@
>  void
>  PluginModuleParent::ExitedCxxStack()
>  {
> +    mHangAnnotationFlags = 0;

What if we re-entered the CxxStack for this instance?

@@ +1377,5 @@
>          return NS_ERROR_FAILURE;
>      }
>  
> +    if (mPluginName.IsEmpty()) {
> +        GetPluginName(mPluginName, mPluginVersion);

If we retrieve the data for every PluginModuleParent anyway, why can't we just set it in e.g. SetPlugin() instead of this lazy approach?

::: dom/plugins/ipc/PluginModuleParent.h
@@ +367,5 @@
>      FinishHangUI();
>  #endif
>  
> +    bool
> +    GetPluginName(nsACString& aPluginName, nsACString& aPluginVersion);

This doesn't only return the name - something like GetPluginDetails() instead?

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Plugin ChromeHang annotations (rev. 2)

(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> Comment on attachment 8460600 [details] [diff] [review]
> Part 2: Plugin ChromeHang annotations
> 
> Review of attachment 8460600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we need to clear up first how this deals with re-entering the stack.
> 
> ::: dom/plugins/ipc/PluginModuleParent.cpp
> @@ +389,4 @@
> >  void
> >  PluginModuleParent::ExitedCxxStack()
> >  {
> > +    mHangAnnotationFlags = 0;
> 
> What if we re-entered the CxxStack for this instance?

This is not an issue: These virtual functions are only called on the first CxxStack entry/last CxxStack exit. See
http://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#151

> 
> @@ +1377,5 @@
> >          return NS_ERROR_FAILURE;
> >      }
> >  
> > +    if (mPluginName.IsEmpty()) {
> > +        GetPluginName(mPluginName, mPluginVersion);
> 
> If we retrieve the data for every PluginModuleParent anyway, why can't we
> just set it in e.g. SetPlugin() instead of this lazy approach?

Done.

> 
> ::: dom/plugins/ipc/PluginModuleParent.h
> @@ +367,5 @@
> >      FinishHangUI();
> >  #endif
> >  
> > +    bool
> > +    GetPluginName(nsACString& aPluginName, nsACString& aPluginVersion);
> 
> This doesn't only return the name - something like GetPluginDetails()
> instead?

Done.

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Plugin ChromeHang annotations (rev. 3)

I turns out that we can't move GetPluginDetails() to SetPlugin() as the plugin tag isn't fully initialized until after NP_Initialize. I discussed with gfritzsche on IRC. Carrying forward r+.

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add annotation support to ChromeHangs (rev.2)

I addressed all of vladan's comments except for the one about mHangIndex; we need that because we only create annotation info as needed, so the indexes of the annotations do mot match up with the indexes of the hang data. Carrying forward r+.

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

https://tbpl.mozilla.org/?tree=Try&rev=26dd0bdf820a

https://hg.mozilla.org/integration/mozilla-inbound/rev/313123f12e95
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6af789bc67

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows non-unified bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/15a271e87dd3

https://tbpl.mozilla.org/php/getParsedLog.php?id=49983242&tree=Try

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add annotation support to ChromeHangs (rev.3)

Trivial fix. Carrying forward r+.
Grumble, grumble, grumble...

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

let's try this again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4bd80941d41
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ea9f5f9b9f

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Bustage fix :-(
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbfb7abec255

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4bd80941d41
https://hg.mozilla.org/mozilla-central/rev/b3ea9f5f9b9f
https://hg.mozilla.org/mozilla-central/rev/cbfb7abec255