1128768 - Add type of swf file to flash hang annotations

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Refactor hang annotations code

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Add annotation support to BHR

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4: Telemetry changes

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Get topmost routing id from MessageChannel (r2)

Addressed comments. Carrying forward r+.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Refactor hang annotations code (r2)

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4: Telemetry changes (r2)

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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

[Georg Fritzsche [:gfritzsche]]

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

[Georg Fritzsche [:gfritzsche]]

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

[Jim Mathies [:jimm]]

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

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Refactor hang annotations code (r3)

Updated as a consequence of changes made to other patches.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4: Telemetry changes (r3)

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

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 5: Plugin changes (r2)

Addressed all comments.

Comment 17

[Vladan Djeric (:vladan)]

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

[Vladan Djeric (:vladan)]

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

[Vladan Djeric (:vladan)]

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

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

(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

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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

[(No longer employed by Mozilla) Aaron Klotz]

(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 23

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Refactor hang annotations code (r5)

Minor fix for static analysis failure

Comment 25

[Carsten Book [:Tomcat]]

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