574357 - Plugin crash reports are submitted with Throttleable=0

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Benjamin Smedberg]

Plugin crash reports are being submitted with Throttleable=0. This is bad, we think, and should probably block 3.6.6

Comment 1

[Benjamin Smedberg]

This was mentioned/discovered in bug 574250

Comment 2

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

we need to decide if it is worth the time to fix.  The 'Throttleable' flag was a feature added to breakpad/socorro back around FF3.5 to aid Socorro in keeping diskspace requirements under control.  The feature was designed to 'discard' initial submissions that would have gone to deferred storage.  Resubmission would happen on a manual request and Socorro would be forbidden to apply any throttling rules.  However, we never actually started using the feature because Socorro was granted more disk space.  Socorro has been running with the 'neverDiscard' rule for months. 

The new HBase/Hadoop system completely eliminates the need for the feature.  Socorro's throttling will soon go away.  Disabling interpretation of the 'Throttleable' flag is simple and effective for right now.  We ought to consider removal the feature entirely at the same time that throttling is retired.

Comment 3

[chris hofmann]

if we don't fix might be worth atleast comments in the code to point at what is supported and whats not on both server and client side of the api (maybe just point to the comments in this bug)   

if we do this it could help tripping over bugs or misunderstandings in the future.

Comment 4

[(not currently active) Ted Mielczarek]

I've got a fix in hand, it's pretty trivial. Just taking a bit of time to write a test since this code path (submitting plugin crash reports) was currently untested.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: Send plugin crash reports without throttling

I think the test is a good thing to have regardless of whether we care about this specific behavior in the future, so I'm glad I spent the time to write it.

There are a few changes in here that I made just to make writing the test easier:
1) In browser.js, fire an observer service notification "plugin-crash-submit" when a plugin crash is submitted (or if there's an error submitting).
2) Hacked EventUtils.js' SendMouseEvent to take an element instead of just an id string for the target, so I could send a mouse click to the anonymous content in the crashed plugin.

Most of the hard parts of the test are just reused from the browser chrome tests for about:crashes resubmission. (The SJS that accepts crash reports, in particular.)

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 453935 [details] [diff] [review]
Send plugin crash reports without throttling

>--- a/browser/base/content/browser.js
...
>       // fix bug 253793 - turn off highlight when page changes
>-      gFindBar.getElement("highlight").checked = false;      
>+      gFindBar.getElement("highlight").checked = false;

Oops, accidental whitespace change?

>+    function submitCallback(succeeded, id, data) {
>+      let serverID = null;
>+      if (data != null && 'CrashID' in data)
>+        serverID = data['CrashID'];
>+      Services.obs.notifyObservers(null, "plugin-crash-submit", serverID);
>+    }

I think this notification really ought to be sent by CrashSubmit.jsm itself, instead of putting it in browser.js. This would also give you the notification for the browser crash report as well (for free!).

In fact, there's already a notification (crash-report-status) being sent there, in notifyStatus, right before the callback is invoked. You'd just need to stuff ret.CrashID into the existing property bag.


>--- a/modules/plugin/test/mochitest/test_crash_notify_no_report.xul
...
>+    // Verify the data. The SJS script will return the data that was POSTed

I like this additional testing, but I think the existing "plugin-crashed" notification tests should remain -- that's basically an interface exported by CrashSubmit.jsm, so testing it directly should be kept.

>-  submit: function CrashSubmit_submit(id, element, submitSuccess, submitError)
>+  submit: function CrashSubmit_submit(id, element, submitSuccess, submitError,
>+                                      noThrottle)

It's kind of unfortunate that this is a static method (in the C++ sense)... It would be much better to be able to do:

  let submission = new CrashSubmit(id, element);
  submission.onSuccess = callback; // optional
  submission.newoption = false;    // also optional

There's a bit of risk in the current code with getting arguments in the wrong order... Because existing callers have not been changed to explicitly pass in noThrottle==false, the next time an argument is added someone will just append it to one of these existing calls and it'll be passed in as noThrottle.

Maybe spin this out to a new bug? It's ok for now.

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to comment #6)
> I think this notification really ought to be sent by CrashSubmit.jsm itself,
> instead of putting it in browser.js. This would also give you the notification
> for the browser crash report as well (for free!).
> 
> In fact, there's already a notification (crash-report-status) being sent there,
> in notifyStatus, right before the callback is invoked. You'd just need to stuff
> ret.CrashID into the existing property bag.

Ok, I'll change this to use that existing notification.

> >--- a/modules/plugin/test/mochitest/test_crash_notify_no_report.xul
> ...
> >+    // Verify the data. The SJS script will return the data that was POSTed
> 
> I like this additional testing, but I think the existing "plugin-crashed"
> notification tests should remain -- that's basically an interface exported by
> CrashSubmit.jsm, so testing it directly should be kept.

I mentioned this on IRC, but for the record, I did a "hg cp" here, so the original test is not touched, this is a new test based on the old one.

> 
> >-  submit: function CrashSubmit_submit(id, element, submitSuccess, submitError)
> >+  submit: function CrashSubmit_submit(id, element, submitSuccess, submitError,
> >+                                      noThrottle)
> 
> It's kind of unfortunate that this is a static method (in the C++ sense)... It
> would be much better to be able to do:
> ...

I'll file a followup on this. This method ought to be refactored to use FormData instead of an iframe anyway.

Comment 8

[(not currently active) Ted Mielczarek]

I already had bug 548667 on file, so I added your comments there.

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: Send plugin crash reports without throttling, rev 2

Good suggestions, this patch avoids changing browser.js at all, which is nice.

Comment 10

[LegNeato]

Comment on attachment 455006 [details] [diff] [review]
Send plugin crash reports without throttling, rev 2

a=LegNeato for 1.9.2.7 (preapproving). Please land this on mozilla-1.9.2 default once it has passed tests.

Comment 11

[Justin Dolske [:Dolske]]

Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/64fe5516169a

Will land on m-c when it greens up (from an unrelated backout).

Comment 12

[Justin Dolske [:Dolske]]

Landed on m-c: http://hg.mozilla.org/mozilla-central/rev/e112f68bc941

...and then backed out on m-c and 1.9.2 due to test failures.

Linux Mo3:

6912 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/whatwg/test_bug500328.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - aTarget.dispatchEvent is not a function at http://mochi.test:8888/tests/SimpleTest/EventUtils.js:60

Linux Mo5:

8043 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_hiddenitems.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - aTarget.dispatchEvent is not a function at http://mochi.test:8888/tests/SimpleTest/EventUtils.js:60
10666 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_richlist_direction.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - aTarget.dispatchEvent is not a function at http://mochi.test:8888/tests/SimpleTest/EventUtils.js:60

The test added here also failed on 1.9.2 because it was using the trunk-only Services.jsm, I'll attach the trivial patch for that, although the test still times out on my local Linux box even with the patch...

Comment 13

[Justin Dolske [:Dolske]]

Attachment: trivial branch testfix

Comment 14

[Justin Dolske [:Dolske]]

...and looks like other similar failures in M1-5. Se TB+PL for the details.

Comment 15

[(not currently active) Ted Mielczarek]

Blah. I should have known I couldn't get away with touching EventUtils so easily.

Thanks for trying, I'll give it a sanity check today.

Comment 16

[(not currently active) Ted Mielczarek]

Ugh, that EventUtils change was stupid, sorry about that. I fixed it and pushed the patch to m-c:
http://hg.mozilla.org/mozilla-central/rev/2c2f08a6a325

Comment 17

[(not currently active) Ted Mielczarek]

Pushed a followup to not run the test on platforms where we don't build the crashreporter:
http://hg.mozilla.org/mozilla-central/rev/3b1c1d1fe7aa

Comment 18

[(not currently active) Ted Mielczarek]

Pushed to 1.9.2 (including Dolske's test fix):
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/112c8fabcd49