Closed
Bug 818307
Opened 12 years ago
Closed 10 years ago
Add flag to chromehang when user selects "Continue" in Plugin Hang UI
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
Attachments
(2 files, 7 obsolete files)
14.42 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
27.46 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
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."
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → aklotz
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Comment 4•10 years ago
|
||
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?
Updated•10 years ago
|
Flags: needinfo?(aklotz)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8448460 -
Attachment is obsolete: true
Attachment #8448461 -
Attachment is obsolete: true
Attachment #8460599 -
Flags: review?(vdjeric)
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
(and outlives NSPR)
Assignee | ||
Updated•10 years ago
|
Attachment #8460600 -
Flags: review?(benjamin)
Updated•10 years ago
|
Attachment #8460600 -
Flags: review?(benjamin) → review?(georg.fritzsche)
Comment 16•10 years ago
|
||
Sorry for the delay here Aaron, i will get to this tomorrow.
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8502302 -
Flags: review?(georg.fritzsche) → review+
Assignee | ||
Comment 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
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+
Assignee | ||
Comment 21•10 years ago
|
||
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•10 years ago
|
||
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
Assignee | ||
Comment 23•10 years ago
|
||
Trivial fix. Carrying forward r+. Grumble, grumble, grumble...
Attachment #8503271 -
Attachment is obsolete: true
Attachment #8508859 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
let's try this again: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4bd80941d41 https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ea9f5f9b9f
Assignee | ||
Comment 25•10 years ago
|
||
Bustage fix :-( https://hg.mozilla.org/integration/mozilla-inbound/rev/cbfb7abec255
https://hg.mozilla.org/mozilla-central/rev/f4bd80941d41 https://hg.mozilla.org/mozilla-central/rev/b3ea9f5f9b9f https://hg.mozilla.org/mozilla-central/rev/cbfb7abec255
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•