Last Comment Bug 1128768 - Add type of swf file to flash hang annotations
: Add type of swf file to flash hang annotations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla39
Assigned To: Aaron Klotz [:aklotz]
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on: 1132305 1133521
Blocks:
  Show dependency treegraph
 
Reported: 2015-02-02 16:19 PST by Aaron Klotz [:aklotz]
Modified: 2015-03-10 06:01 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1: Get topmost routing id from MessageChannel (2.52 KB, patch)
2015-02-18 23:43 PST, Aaron Klotz [:aklotz]
dvander: review+
Details | Diff | Splinter Review
Part 2: Refactor hang annotations code (24.91 KB, patch)
2015-02-18 23:46 PST, Aaron Klotz [:aklotz]
no flags Details | Diff | Splinter Review
Part 3: Add annotation support to BHR (5.70 KB, patch)
2015-02-18 23:48 PST, Aaron Klotz [:aklotz]
vladan.bugzilla: review+
Details | Diff | Splinter Review
Part 4: Telemetry changes (5.85 KB, patch)
2015-02-18 23:51 PST, Aaron Klotz [:aklotz]
no flags Details | Diff | Splinter Review
Part 5 - Plugin changes (preliminary) (18.37 KB, patch)
2015-02-18 23:52 PST, Aaron Klotz [:aklotz]
no flags Details | Diff | Splinter Review
Part 1: Get topmost routing id from MessageChannel (r2) (2.79 KB, patch)
2015-02-20 09:35 PST, Aaron Klotz [:aklotz]
aklotz: review+
Details | Diff | Splinter Review
Part 2: Refactor hang annotations code (r2) (24.91 KB, patch)
2015-02-23 17:46 PST, Aaron Klotz [:aklotz]
no flags Details | Diff | Splinter Review
Part 4: Telemetry changes (r2) (5.97 KB, patch)
2015-02-23 17:47 PST, Aaron Klotz [:aklotz]
gfritzsche: review+
Details | Diff | Splinter Review
Part 5: Plugin changes (24.18 KB, patch)
2015-02-23 17:51 PST, Aaron Klotz [:aklotz]
jmathies: review-
Details | Diff | Splinter Review
Part 2: Refactor hang annotations code (r3) (25.04 KB, patch)
2015-02-25 16:41 PST, Aaron Klotz [:aklotz]
vladan.bugzilla: review+
Details | Diff | Splinter Review
Part 4: Telemetry changes (r3) (10.93 KB, patch)
2015-02-25 16:42 PST, Aaron Klotz [:aklotz]
aklotz: review+
Details | Diff | Splinter Review
Part 5: Plugin changes (r2) (26.19 KB, patch)
2015-02-25 16:43 PST, Aaron Klotz [:aklotz]
jmathies: review+
Details | Diff | Splinter Review
Part 2: Refactor hang annotations code (r4) (26.27 KB, patch)
2015-03-09 15:37 PDT, Aaron Klotz [:aklotz]
aklotz: review+
Details | Diff | Splinter Review
Part 2: Refactor hang annotations code (r5) (26.30 KB, patch)
2015-03-09 19:21 PDT, Aaron Klotz [:aklotz]
aklotz: review+
Details | Diff | Splinter Review

Description User image Aaron Klotz [:aklotz] 2015-02-02 16:19:50 PST
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.
Comment 1 User image Aaron Klotz [:aklotz] 2015-02-18 23:43:52 PST
Created attachment 8566414 [details] [diff] [review]
Part 1: Get topmost routing id from MessageChannel

This bug requires us to be able to determine which actor is highest in the call stack during a plugin hang.
Comment 2 User image Aaron Klotz [:aklotz] 2015-02-18 23:46:47 PST
Created attachment 8566415 [details] [diff] [review]
Part 2: Refactor hang annotations code
Comment 3 User image Aaron Klotz [:aklotz] 2015-02-18 23:48:37 PST
Created attachment 8566416 [details] [diff] [review]
Part 3: Add annotation support to BHR
Comment 4 User image Aaron Klotz [:aklotz] 2015-02-18 23:51:09 PST
Created attachment 8566421 [details] [diff] [review]
Part 4: Telemetry changes
Comment 5 User image Aaron Klotz [:aklotz] 2015-02-18 23:52:41 PST
Created attachment 8566422 [details] [diff] [review]
Part 5 - Plugin changes (preliminary)

WIP of plugin changes. Annotates the entire SWF URL, which of course we don't want in the final revision.
Comment 6 User image David Anderson [:dvander] 2015-02-19 15:43:17 PST
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.
Comment 7 User image Aaron Klotz [:aklotz] 2015-02-20 09:35:54 PST
Created attachment 8567173 [details] [diff] [review]
Part 1: Get topmost routing id from MessageChannel (r2)

Addressed comments. Carrying forward r+.
Comment 8 User image Aaron Klotz [:aklotz] 2015-02-23 17:46:23 PST
Created attachment 8568289 [details] [diff] [review]
Part 2: Refactor hang annotations code (r2)
Comment 9 User image Aaron Klotz [:aklotz] 2015-02-23 17:47:37 PST
Created attachment 8568290 [details] [diff] [review]
Part 4: Telemetry changes (r2)
Comment 10 User image Aaron Klotz [:aklotz] 2015-02-23 17:51:47 PST
Created attachment 8568292 [details] [diff] [review]
Part 5: Plugin changes

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 11 User image Georg Fritzsche [:gfritzsche] 2015-02-25 11:04:18 PST
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.
Comment 12 User image Georg Fritzsche [:gfritzsche] 2015-02-25 11:04:38 PST
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 13 User image Jim Mathies [:jimm] 2015-02-25 14:07:51 PST
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?
Comment 14 User image Aaron Klotz [:aklotz] 2015-02-25 16:41:37 PST
Created attachment 8569538 [details] [diff] [review]
Part 2: Refactor hang annotations code (r3)

Updated as a consequence of changes made to other patches.
Comment 15 User image Aaron Klotz [:aklotz] 2015-02-25 16:42:55 PST
Created attachment 8569539 [details] [diff] [review]
Part 4: Telemetry changes (r3)

Addressed Georg's comments. Will address tests in separate patches. Carrying forward r+.
Comment 16 User image Aaron Klotz [:aklotz] 2015-02-25 16:43:55 PST
Created attachment 8569540 [details] [diff] [review]
Part 5: Plugin changes (r2)

Addressed all comments.
Comment 17 User image Vladan Djeric (:vladan) 2015-03-03 18:42:37 PST
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 18 User image Vladan Djeric (:vladan) 2015-03-03 18:48:32 PST
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
Comment 19 User image Vladan Djeric (:vladan) 2015-03-03 18:50:50 PST
^^ i did jim's review as jim is away for a while
Comment 20 User image Aaron Klotz [:aklotz] 2015-03-09 14:24:13 PDT
(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.
Comment 21 User image Aaron Klotz [:aklotz] 2015-03-09 15:37:41 PDT
Created attachment 8574901 [details] [diff] [review]
Part 2: Refactor hang annotations code (r4)

(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.
Comment 22 User image Aaron Klotz [:aklotz] 2015-03-09 15:47:18 PDT
(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.
Comment 24 User image Aaron Klotz [:aklotz] 2015-03-09 19:21:25 PDT
Created attachment 8575020 [details] [diff] [review]
Part 2: Refactor hang annotations code (r5)

Minor fix for static analysis failure

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