For privacy reasons we won't log the actual URL. Instead we'll flag whether the swf file is an ad or came from Facebook.

This bug requires us to be able to determine which actor is highest in the call stack during a plugin hang.

WIP of plugin changes. Annotates the entire SWF URL, which of course we don't want in the final revision.

Comment on attachment 8566414 [details] [diff] [review] Part 1: Get topmost routing id from MessageChannel Review of attachment 8566414 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/glue/MessageChannel.h @@ +134,5 @@ > bool IsOnCxxStack() const { > return !mCxxStackFrames.empty(); > } > > + int32_t GetTopmostMessageRoutingId() const; r=me w/ a comment here explaining what this should be used for.

Addressed comments. Carrying forward r+.

This patch grabs a mutex whenever entering or leaving a call or sync send, but it should rarely be contended (and even then only immediately after a hang).

Comment on attachment 8568290 [details] [diff] [review] Part 4: Telemetry changes (r2) Review of attachment 8568290 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Telemetry.cpp @@ +2661,5 @@ > + if (!jsAnnotation) { > + continue; > + } > + nsAutoPtr<HangAnnotations::Enumerator> annotationsEnum; > + if (!(*i)->GetEnumerator(annotationsEnum.StartAssignment())) { For readability you could use a temporary, e.g.: const UniquePtr<HangAnnotations>& ptr = *i; ::: toolkit/components/telemetry/ThreadHangStats.h @@ +120,5 @@ > HangStack mNativeStack; > // Use a hash to speed comparisons > const uint32_t mHash; > + // Annotations attributed to this stack > + Vector<UniquePtr<HangMonitor::HangAnnotations>> mAnnotations; Can we typedef this to, say, something like |HangAnnotationVector|? This would make this file and Telemetry.cpp look cleaner.

As mentioned on IRC, i think this feature should get some basic test coverage. We could: * fake a flash plugin (i.e. fake the mimetype etc. like dom/plugins/test/testplugin/javaplugin/) * run the plugin in a mochitest-chrome * use plugin.stallPlugin() * collect the telemetry payload and sanity check things

Comment on attachment 8568292 [details] [diff] [review] Part 5: Plugin changes Review of attachment 8568292 [details] [diff] [review]: ----------------------------------------------------------------- Genrally this looks good, just "comment me" comments. r- though since I'd really like to see the next revision. ::: dom/plugins/base/nsPluginPlayPreviewInfo.h @@ +22,5 @@ > const char* aRedirectURL, > const char* aWhitelist); > explicit nsPluginPlayPreviewInfo(const nsPluginPlayPreviewInfo* aSource); > > + static nsresult CheckWhitelist(const nsACString& aPageURI, lets add a good comment for this, since it's now a public static util. ::: dom/plugins/ipc/PluginInstanceParent.cpp @@ +55,5 @@ > #elif defined(XP_MACOSX) > #include <ApplicationServices/ApplicationServices.h> > #endif // defined(XP_MACOSX) > > +static const char kShumwayWhitelistPref[] = "shumway.swf.whitelist"; nit - comment me @@ -142,5 @@ > #endif > } > > bool > -PluginInstanceParent::Init() we finally found a use for this! :) @@ +170,5 @@ > + if (NS_FAILED(rv)) { > + return false; > + } > + auto whitelist = Preferences::GetCString(kShumwayWhitelistPref); > + if (whitelist.IsEmpty()) { Lets add a comment here about the importance of this check - it looks like CheckWhitelist considers an empty string to be a "match everything" case. We don't want someone removing this accidentally. ::: dom/plugins/ipc/PluginInstanceParent.h @@ +278,5 @@ > + aOutput = mSrcAttribute; > + } > + > + bool > + IsWhitelistedForShumway() const comment me please. ::: dom/plugins/ipc/PluginModuleParent.cpp @@ +870,5 @@ > + mProtocolCallStack.RemoveElementAt(mProtocolCallStack.Length() - 1); > +} > + > +mozilla::ipc::IProtocol* > +PluginModuleChromeParent::GetInvokingProtocol() this whole method could stand some commenting. What are the two routingId checks for for example?

Updated as a consequence of changes made to other patches.

Addressed Georg's comments. Will address tests in separate patches. Carrying forward r+.

Addressed all comments.

Comment on attachment 8569538 [details] [diff] [review] Part 2: Refactor hang annotations code (r3) Review of attachment 8569538 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/HangAnnotations.cpp @@ +16,5 @@ > +namespace HangMonitor { > + > +// Chrome hang annotators. This can go away once BHR has completely replaced > +// ChromeHangs. > +static StaticAutoPtr<Observer::Annotators> gAnnotators; I know this is old code, but can we call this gChromehangAnnotators? @@ +53,5 @@ > + MOZ_COUNT_DTOR(BrowserHangAnnotations); > +} > + > +void > +BrowserHangAnnotations::AddAnnotation(const nsAString& aName, const int32_t aData) We still don't have any variant types other than nsIVariant? :( @@ +161,5 @@ > + return result; > +} > + > +bool > +BrowserHangAnnotations::GetEnumerator(HangAnnotations::Enumerator** aOutEnum) Does this method really need a return value + an out pointer? @@ +232,5 @@ > +{ > + BackgroundHangMonitor::RegisterAnnotator(aAnnotator); > + // We still register annotators for ChromeHangs > + if (NS_IsMainThread() && > + GeckoProcessType_Default == XRE_GetProcessType()) { don't we have chromehang reporting in child processes as well? @@ +236,5 @@ > + GeckoProcessType_Default == XRE_GetProcessType()) { > + if (!gAnnotators) { > + gAnnotators = new Observer::Annotators(); > + } > + gAnnotators->Register(aAnnotator); Is the return value of Annotators::Register used or going to be used anywhere?

Comment on attachment 8566416 [details] [diff] [review] Part 3: Add annotation support to BHR Review of attachment 8566416 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/BackgroundHangMonitor.cpp @@ +566,5 @@ > +BackgroundHangMonitor::RegisterAnnotator(HangMonitor::Annotator& aAnnotator) > +{ > +#ifdef MOZ_ENABLE_BACKGROUND_HANG_MONITOR > + BackgroundHangThread* thisThread = BackgroundHangThread::FindThread(); > + if (!thisThread) { just to make sure I understand.. if I want to provide annotations for compositor thread hangs, i have to call RegisterAnnotator from the compositor thread? if so, i think we'll want to add a "thread" argument in the future so RegisterAnnotator can be called from other threads

^^ i did jim's review as jim is away for a while

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #18) > Comment on attachment 8566416 [details] [diff] [review] > Part 3: Add annotation support to BHR > > Review of attachment 8566416 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: xpcom/threads/BackgroundHangMonitor.cpp > @@ +566,5 @@ > > +BackgroundHangMonitor::RegisterAnnotator(HangMonitor::Annotator& aAnnotator) > > +{ > > +#ifdef MOZ_ENABLE_BACKGROUND_HANG_MONITOR > > + BackgroundHangThread* thisThread = BackgroundHangThread::FindThread(); > > + if (!thisThread) { > > just to make sure I understand.. if I want to provide annotations for > compositor thread hangs, i have to call RegisterAnnotator from the > compositor thread? if so, i think we'll want to add a "thread" argument in > the future so RegisterAnnotator can be called from other threads Correct. Filed bug 1141284 to add this functionality.

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #17) > Comment on attachment 8569538 [details] [diff] [review] > Part 2: Refactor hang annotations code (r3) > > Review of attachment 8569538 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: xpcom/threads/HangAnnotations.cpp > @@ +16,5 @@ > > +namespace HangMonitor { > > + > > +// Chrome hang annotators. This can go away once BHR has completely replaced > > +// ChromeHangs. > > +static StaticAutoPtr<Observer::Annotators> gAnnotators; > > I know this is old code, but can we call this gChromehangAnnotators? > Sure. Done. > @@ +53,5 @@ > > + MOZ_COUNT_DTOR(BrowserHangAnnotations); > > +} > > + > > +void > > +BrowserHangAnnotations::AddAnnotation(const nsAString& aName, const int32_t aData) > > We still don't have any variant types other than nsIVariant? :( This is the best we have ATM. > > @@ +161,5 @@ > > + return result; > > +} > > + > > +bool > > +BrowserHangAnnotations::GetEnumerator(HangAnnotations::Enumerator** aOutEnum) > > Does this method really need a return value + an out pointer? Good point. We can just return nullptr upon failure. Fixed. > > @@ +232,5 @@ > > +{ > > + BackgroundHangMonitor::RegisterAnnotator(aAnnotator); > > + // We still register annotators for ChromeHangs > > + if (NS_IsMainThread() && > > + GeckoProcessType_Default == XRE_GetProcessType()) { > > don't we have chromehang reporting in child processes as well? This was added in bug 1087410. At that time chromehangs were not initialized in content processes. > > @@ +236,5 @@ > > + GeckoProcessType_Default == XRE_GetProcessType()) { > > + if (!gAnnotators) { > > + gAnnotators = new Observer::Annotators(); > > + } > > + gAnnotators->Register(aAnnotator); > > Is the return value of Annotators::Register used or going to be used > anywhere? It's not used yet, but I felt that it might be useful in the future.

(In reply to Georg Fritzsche [:gfritzsche] [away Feb 27 - March 15] from comment #12) > As mentioned on IRC, i think this feature should get some basic test > coverage. > We could: > * fake a flash plugin (i.e. fake the mimetype etc. like > dom/plugins/test/testplugin/javaplugin/) > * run the plugin in a mochitest-chrome > * use plugin.stallPlugin() > * collect the telemetry payload and sanity check things Filed bug 1141331 for this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/613ea27a306e https://hg.mozilla.org/integration/mozilla-inbound/rev/13eb8592325c https://hg.mozilla.org/integration/mozilla-inbound/rev/c3501a329f69 https://hg.mozilla.org/integration/mozilla-inbound/rev/2f0b44330ffb https://hg.mozilla.org/integration/mozilla-inbound/rev/0df0abc0f58c

Minor fix for static analysis failure

https://hg.mozilla.org/mozilla-central/rev/613ea27a306e https://hg.mozilla.org/mozilla-central/rev/13eb8592325c https://hg.mozilla.org/mozilla-central/rev/c3501a329f69 https://hg.mozilla.org/mozilla-central/rev/2f0b44330ffb https://hg.mozilla.org/mozilla-central/rev/0df0abc0f58c