792118 - Make sure "PluginHang" reports are processed as hangs

Status: VERIFIED FIXED

(Socorro :: Backend, task)

Description

[Benjamin Smedberg]

New-style hang reports won't have a HangID but will have PluginHang=1 in their raw JSON. This processor change synthesizes a HangID so that queries and the webapp continue to function. I'm going to put up a PR which is probably correct but untested and I'll need help getting it tested and landed.

Comment 1

[Benjamin Smedberg]

https://github.com/mozilla/socorro/pull/839

Comment 2

[Laura Thomson :laura]

Needs to be in production before November 19.  Lars, please review the PR and land.

Comment 3

[Benjamin Smedberg]

Samples can be had:

Prerequisites:

* Windows (can be done on other platforms with slightly different results)
* Firefox Nightly/Aurora build
* Sysinternals Process Explorer <http://technet.microsoft.com/en-us/sysinternals/bb896653.aspx>
* Change the pref "toolkit.crashreporter.pluginHangSubmitURL" to point to your server
* Change the pref "dom.ipc.plugins.timeoutSecs" to something smaller like 10

Then, to force a plugin to hang:

* Launch Firefox
* browse to a site that uses Flash such as youtube
* In process explorer, find your plugin-container process and suspend it
* Wait 10 seconds and Firefox should say that the plugin has crashed

Comment 4

[K Lars Lohn [:lars] [:klohn]]

unfortunately, I do not any access to a windows machine.  can I get a usable result if I were to try these instructions under Linux (my only option)

Comment 5

[Benjamin Smedberg]

Yes, although you'll only get two minidumps (plugin and browser) instead of the four that come with Windows results.

You can probably suspend a process by sending it SIGSTOP or attaching with GDB.

Comment 6

[[github robot]]

Commits pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/84f9772c63fa284b8daa5c03b202b89f06c309eb
Bug 792118 - Make sure that new-style plugin hang reports (which have PluginHang=1 but no HangID) are processed correctly as plugin hangs.

https://github.com/mozilla/socorro/commit/b339563e8814f92f8dd06bb0cff4b44f909311ba
Merge pull request #915 from twobraids/hang2012

fixes Bug 792118 - new style PluginHang / HangID hack

Comment 7

[Benjamin Smedberg]

brandt, what does [qa-] mean here? I really think this should be verified, because if FF18 hits beta without this fixed we could lose important stats.

Comment 8

[Matt Brandt [:mbrandt]]

:bsmedberg -- sorry for the confusion. I didn't realize the impact. I'll bump this to [qa+].

We use [qa-] for features that don't need testing by a QA and can be covered by dev.

Comment 9

[K Lars Lohn [:lars] [:klohn]]

Attachment: sample crash for verifying this bug

The attached crash pair is a synthesized crash with the attributes to trigger the new code in the bug.  The crash has PluginHang=1, but no HangID.  The correctly working processor should create a processed crash with hang_type=-1 and a hang_id starting with "fake-" followed by the crash's crash_id/uuid.

Comment 10

[Benjamin Smedberg]

I'd like to have this verified against Firefox Aurora by changing the pref and submitting an actual crash.

Comment 11

[Laura Thomson :laura]

This is busted; all crashes are showing up as hangs.  Rolling back 26 in bug 810298.

Comment 12

[K Lars Lohn [:lars] [:klohn]]

I've found the flaw in the logic.  

        if int(jsonDocument.get("Hang", False)):
          hangType = 1
        elif jsonDocument.get("PluginHang", False):
          hangType = -1
        elif 'hangid' in newReportRecordAsDict:
          hangType = -1
        else:
          hangType = 0

the problem is in the line "elif 'hangid' in newReportRecordAsDict".   "hangid" is _always_ in the crash.  If the crash isn't a hang, the value is NULL.  Since that part of the if chain is True, the hangType is always set to -1.  This, in turn, induces the SignatureUtility to prepend the word "hang" in front of the signature.  

the line should read:

    elif newReportRecordAsDict["hangid"]:

that way it senses the NULL case and doesn't force hang_type to -1 for all crashes.  Interestingly, the patch for the configmanized processor doesn't contain this flaw.

Comment 13

[Laura Thomson :laura]

Let's test the fix in staging as bsmedberg suggests.

Comment 14

[[github robot]]

Commits pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/f6951f93bcaa6c8513a0d0e25e42bbdd509ea740
Revert "Bug 792118 - Make sure that new-style plugin hang reports (which have PluginHang=1 but no HangID) are processed correctly as plugin hangs."

This reverts commit 84f9772c63fa284b8daa5c03b202b89f06c309eb.

https://github.com/mozilla/socorro/commit/84bcd00e50b6daf86ef1448af4068ccdcd7549dc
Merge pull request #929 from selenamarie/revert-bug792118-master

Revert "Bug 792118 - Make sure that new-style plugin hang reports (which...

Comment 15

[[github robot]]

Commits pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/fdefc36bf43e00250e234a43f919b437790fc896
Bug 792118 - support for new-style plugin hangs (2nd version)

https://github.com/mozilla/socorro/commit/0982953426e857eeebb1fb31e1bcf2f318e775ab
Merge pull request #930 from twobraids/hang2012retry

Bug 792118 - support for new-style plugin hangs (2nd version)

Comment 16

[Laura Thomson :laura]

Should this be re-resolved?  Needs to be released ASAP

Comment 17

[K Lars Lohn [:lars] [:klohn]]

I need independent confirmation from QA before I'm comfortable with this going into production.

Comment 18

[Matt Brandt [:mbrandt]]

Thanks lars -- we'll (QA) do a positive (comment 3) and negative test.

Comment 19

[Matt Brandt [:mbrandt]]

QA Verified on stage -- lars thanks for the hand-holding on helping me test this.
+ testcase passes (comment 3) - https://crash-stats.allizom.org/report/index/bp-011802e9-0867-4864-b033-e306f2121115
+ also the tests run for https://bugzilla.mozilla.org/show_bug.cgi?id=792082#c11 also aply here

Comment 20

[Robert Helmer [:rhelmer]]

(In reply to Matt Brandt [:mbrandt] from comment #19)
> QA Verified on stage -- lars thanks for the hand-holding on helping me test
> this.
> + testcase passes (comment 3) -
> https://crash-stats.allizom.org/report/index/bp-011802e9-0867-4864-b033-
> e306f2121115


I can't seem to pull the raw crash from stage  stephend can you redo this one against prod when you have a chance?

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

I followed comment 3 and changed toolkit.crashreporter.pluginHangSubmitURL to point to https://crash-reports.mozilla.com/submit, used process explorer to suspend Pandora's plugin-container for Flash, using nightly build Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0

then got https://crash-stats.mozilla.com/report/index/19c967db-990e-4554-963a-3e4552121116.

Verified in prod, yay!

Comment 22

[Matt Brandt [:mbrandt]]

Yay -- thanks guys for doing this! Congrats on the release!