Closed Bug 818307 Opened 11 years ago Closed 10 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

Attachments

(2 files, 7 obsolete files)

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."
Depends on: 805591
Assignee: nobody → aklotz
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?
Attachment #8448460 - Flags: feedback?(benjamin)
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?
Flags: needinfo?(aklotz)
Comment on attachment 8448460 [details] [diff] [review]
Part 1: Implement a thread safe variant

That's reasonable. It simplifies a lot of stuff.
Attachment #8448460 - Flags: feedback?(benjamin)
Flags: needinfo?(aklotz)
Attachment #8448460 - Attachment is obsolete: true
Attachment #8448461 - Attachment is obsolete: true
Attachment #8460599 - Flags: review?(vdjeric)
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.
Attachment #8448462 - Attachment is obsolete: true
Attachment #8460600 - Flags: review?(benjamin)
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?
Attachment #8460600 - Flags: review?(benjamin) → review?(jschoenick)
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
Attachment #8460599 - Flags: review?(vdjeric)
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.
Attachment #8460600 - Flags: review?(jschoenick)
Comment on attachment 8460599 [details] [diff] [review]
Part 1: Add annotation support to ChromeHangs

As discussed in today's 1:1
Attachment #8460599 - Flags: review?(vdjeric)
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
Attachment #8460599 - Flags: review?(vdjeric) → review+
(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.
(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.
(and outlives NSPR)
Attachment #8460600 - Flags: review?(benjamin)
Attachment #8460600 - Flags: review?(benjamin) → review?(georg.fritzsche)
Sorry for the delay here Aaron, i will get to this tomorrow.
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?
Attachment #8460600 - Flags: review?(georg.fritzsche)
(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.
Attachment #8460600 - Attachment is obsolete: true
Attachment #8502302 - Flags: review?(georg.fritzsche)
Attachment #8502302 - Flags: review?(georg.fritzsche) → review+
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+.
Attachment #8502302 - Attachment is obsolete: true
Attachment #8503269 - Flags: review+
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+.
Attachment #8460599 - Attachment is obsolete: true
Attachment #8503271 - Flags: review+
Trivial fix. Carrying forward r+.
Grumble, grumble, grumble...
Attachment #8503271 - Attachment is obsolete: true
Attachment #8508859 - Flags: review+
Depends on: 1087370
Depends on: 1087410
Depends on: 1088126
No longer depends on: 1088126
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.