Closed Bug 574357 Opened 12 years ago Closed 12 years ago

Plugin crash reports are submitted with Throttleable=0

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- beta2+
blocking1.9.2 --- .7+
status1.9.2 --- .7-fixed

People

(Reporter: benjamin, Assigned: ted)

Details

Attachments

(2 files, 1 obsolete file)

Plugin crash reports are being submitted with Throttleable=0. This is bad, we think, and should probably block 3.6.6
This was mentioned/discovered in bug 574250
blocking1.9.2: ? → .6+
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.
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.
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.
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.)
Attachment #453935 - Flags: review?(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.
Attachment #453935 - Flags: review?(dolske) → review-
(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.
I already had bug 548667 on file, so I added your comments there.
Good suggestions, this patch avoids changing browser.js at all, which is nice.
Attachment #453935 - Attachment is obsolete: true
Attachment #455006 - Flags: review?(dolske)
Attachment #455006 - Flags: review?(dolske) → review+
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.
Attachment #455006 - Flags: approval1.9.2.7+
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).
Flags: in-testsuite+
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...
...and looks like other similar failures in M1-5. Se TB+PL for the details.
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.
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
Status: NEW → RESOLVED
Closed: 12 years ago
status1.9.2: ? → ---
Resolution: --- → FIXED
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
Pushed to 1.9.2 (including Dolske's test fix):
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/112c8fabcd49
Target Milestone: --- → mozilla1.9.3
You need to log in before you can comment on or make changes to this bug.