crash reporter should allow resubmission of pending reports

VERIFIED FIXED in mozilla1.9.3a1

Status

()

defect
P3
major
VERIFIED FIXED
12 years ago
2 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Depends on 1 bug, {regression, verified1.9.1, verified1.9.2})

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
blocking1.9.2 +
blocking1.9.1.1 -
blocking1.9 -
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .4+, status1.9.1 .4-fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

I punted on this in bug 365344.  Now that that is checked in, the client will save pending reports in a pending/ dir, so it should look for those and try submitting any that exist when it runs.
Depends on: 394490
In local testing I've had a large number of crash reports in the pending folder for some reason.   These reports would get lost without this patch - so nominating for blocking.  Ideally we'd get this in for b2 or b3 so we can get a sense of how we are doing with crashes.
Flags: blocking1.9?
schrep:  are all your pending crash reports from trunk nightlies from 11/20 or later?

if so, you may be hitting bug #404893
Not sure - I can re-check next time I'm in the office since its on my mini there..
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Why is this a blocker? Losing reports isn't a big deal, statistically...
I would argue with a case such as Bug 390568, that it can be a big issue.
Assignee: nobody → ted.mielczarek
Just a string to squeak in for the string freeze.
Attachment #298399 - Flags: review?(dcamp)
Attachment #298399 - Flags: review?(dcamp) → review+
Attachment #298399 - Attachment description: string → string [checked in]
Flags: tracking1.9+ → wanted-next+
dveditz and I talked about this today (completely unrelated). This is something we really should have for times when Socorro is down or whatever else might be an issue with the server or the user's connection.

Not having this feature would also be a regression from the branch, technically.

Re-requesting blocking.
Flags: blocking1.9?
Keywords: regression
at a minimum, do we have any way to track the number of reports we might have lost?    we frequently compare the numbers of crashes received at check points during a release cycle and calibrate for number of users to try and determine the over all stability of a release to determine if we are getting worse or better from release to release.  in this sense it is statistically significant.

even if we are only counting incompleted submission requests on the server side it would give us a bit better sense of the over all number.
If the upload failed, odds are they didn't get to our server at all due to some network problem, so we have no traceability on that.
chofmann, comparing raw crash counts is always going to be pretty meaningless. The time-since-last-crash and uptime numbers that we do collect provide at least some degree of comparability.
ted, does, or could, the server allow for tracking any or all of these events?
1) the start of the transaction with the client
2) start of upload from the client
3) completion of the upload from the client.

The proposal would be to do some metrics/tracking on ratio of 1 or 2 to 3.

ben, why would it be meaningless?  thats the point of the whole crash reporting system -- reduce the total number of crashes experienced and reported by firefox users...  everything we do ought to be toward making sure that we have believable numbers and the right data and analysis tools to fix crashes and make the number go lower.

It's just an HTTP POST, so I don't know if we can get that out of the logs or not. There's not much of a transaction going on, the client POSTs the info, and the server returns the crash ID in the response body. As I said in comment 9, I would guess that in 99% of cases, crashes that fail to submit never touch our server at all, but fail due to network issues on the client side.
It would be really nice to fix this, but given the disagreement about its importance, I don't see how it can block.
Flags: blocking1.9? → blocking1.9-
(In reply to comment #13)
> It would be really nice to fix this, but given the disagreement about its
> importance, I don't see how it can block.

That's fair. I'd like us to consider this for a minor release, post-Firefox 3, however. Nominating for 1.9.0.x so we keep it on that list.
Flags: wanted1.9.0.x?
Same issue for me. Crash reports in pendir with current nightly- not submitted automatically. Couldn't find a way to do this manually. Crashreporter \pathtodump just crashes the reporter...
Firefox just crashed and I told the crash reporter to send a crash report, but it said it wasn't able to. However, when I looked at the submit.log file in the Crash Reports directory, I see that it says:

[04/29/08 22:51:02] Crash report submission failed: The operation completed successfully.

Does this mean that the crash report was in fact correctly sent? about:crashes still tells me that "No crash reports have been submitted."

Also, is there a way to force the crash reporter to try and resend pending reports?
It did not submit. Unfortunately sometimes it's also unable to get a real error code out of the operating system. There is currently no way to re-submit reports, hence this bug.
This collection of characters can manually submit reports that didn't submit for some reason, if you have perl and curl, and run from "Crash Reports/pending":

perl -we '$d=$ARGV[0];$d=~s/extra$/dmp/;@p=map{chomp;s/^ServerURL=//?($s=$_)&&():("--form-string",$_)}<>;exec("curl","-F","upload_file_minidump=\@$d",@p,$s)' 27ae338b-62a9-ac57-2c0ec905-72403618.extra

(You'll need to remove any line-breaks that bugzilla adds and replace the .extra file with one in the pending directory.)

Make sure to record the id from the output and remove the .extra and .dmp files after successful submission.
Karl, thanks for that, very useful.
Flags: wanted1.9.0.x? → wanted1.9.0.x+
(In reply to comment #17)
> Unfortunately sometimes it's also unable to get a real error
> code out of the operating system.
is there a bug tracking this ?
(In reply to comment #20)
> is there a bug tracking this ?

Not AFAIK. You are welcome to file one.
Ok, I have a new plan, borne of the desire for client-side throttling. I think we should add another section to about:crashes that lists all reports in the pending directory by date. Each report would have a "Resubmit" button next to it, and we'll just use a HTML form in the page to submit the reports from right there. Automated resubmission doesn't seem worth the effort, since we're drowning in crash reports anyway. This proposal will allow users to resubmit crash reports that failed for whatever reason, and if we implement functionality to let the server reject incoming crash reports to throttle them, then it will also let users resubmit those reports as well.
Summary: crash reporter should attempt to resubmit pending reports → crash reporter should allow resubmission of pending reports
Manual would be fine for me- excellent idea, thanks!
Blocks: 469862
We shouldn't have a resubmit button. Instead, when a user clicks on the link for a crash report, if that report was previously rejected by the server, we should resubmit it automatically and take them to the landing page which shows that it's processing.
Flags: wanted1.9.1.x+
Status: NEW → ASSIGNED
There isn't a link currently, but we could probably add one!
Here's what I intend to do:
1) Add reports in pending/ to the list of reports in about:crashes, with no differentiation between submitted and unsubmitted reports (at least for now)
2) When clicking on an unsubmitted report, re-submit it in the background with "Throttleable=0" so that Socorro is forced to process it. Display a throbber or something next to the report while this is happening so the user knows something is happening.
3) After the report is submitted, navigate to the crash-stats page as normal.

We can probably improve the experience a bit on trunk with some string changes, but this should let us land this on 1.9.1 without any l10n impact, so we can start using the server-side throttling without problems.

I'm currently writing some browser tests for about:crashes, I should be able to get this implemented pretty easily.
Flags: wanted1.9.0.x+
Here are some browser chrome tests for the existing about:crashes behavior.
Attachment #382154 - Flags: review?(dtownsend)
Depends on: 496927
Comment on attachment 382154 [details] [diff] [review]
step 1: add some browser chrome tests for existing about:crashes behavior

Might be nice to also test that clicking on the link takes you through to the crash reports server, but this is good as it is. Just a couple of nits.

>diff --git a/toolkit/crashreporter/test/browser/aboutcrashes_utils.js b/toolkit/crashreporter/test/browser/aboutcrashes_utils.js

>+function add_fake_crashes(crD, count) {
>+  let results = [];
>+  let uuidGenerator = Components.classes["@mozilla.org/uuid-generator;1"]
>+                        .getService(Components.interfaces.nsIUUIDGenerator);
>+  let submitdir = crD.clone();
>+  submitdir.append("submitted");
>+  // create them from oldest to newest, to ensure that about:crashes
>+  // displays them in the correct order
>+  let date = Date.now() - count * 60000;
>+  for (let i=0; i<count; i++) {

Spaces around operators please.

>+    let uuid = uuidGenerator.generateUUID().toString();
>+    // ditch the {}
>+    uuid = uuid.substring(1,uuid.length-2);
>+    let fn = "bp-" + uuid + ".txt";
>+    let file = submitdir.clone();
>+    file.append(fn);
>+    file.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);
>+    file.lastModifiedTime = date;
>+    results.push({'id': uuid, 'date': date, 'pending': false});
>+
>+    date += 60000;
>+  }
>+  // we want them sorted newest to oldest, since that's the order
>+  // that about:crashes lists them in
>+  results.sort(function(a,b) b.date-a.date);

And again

>diff --git a/toolkit/crashreporter/test/browser/browser_aboutCrashes.js b/toolkit/crashreporter/test/browser/browser_aboutCrashes.js

>+function check_crash_list(tab, crashes) {
>+  let doc = gBrowser.getBrowserForTab(tab).contentDocument;
>+  let crashlinks = doc.getElementById("tbody").getElementsByTagName("a");
>+  is(crashlinks.length, crashes.length, "about:crashes lists correct number of crash reports");
>+  for(let i=0; i<crashes.length; i++) {

And again...
Attachment #382154 - Flags: review?(dtownsend) → review+
(In reply to comment #29)
> (From update of attachment 382154 [details] [diff] [review])
> Might be nice to also test that clicking on the link takes you through to the
> crash reports server, but this is good as it is. Just a couple of nits.

Yeah, I figured I'd test that in a followup. That's going to involve fiddling with httpd.js, and I figured this was good as a first pass. I'm going to have to cross that bridge to test the resubmission code anyway, so I'll get there.
Comment on attachment 382154 [details] [diff] [review]
step 1: add some browser chrome tests for existing about:crashes behavior

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/9373d3073796
Attachment #382154 - Attachment description: step 1: add some browswer chrome tests for existing about:crashes behavior → step 1: add some browser chrome tests for existing about:crashes behavior
Posted patch almost done (obsolete) — Splinter Review
Ok, this is an implementation of "click to resubmit pending reports" along with a browser chrome test. The implementation loads pending crashes as well as submitted crashes into about:crashes, but when you click on a pending crash it submits it in the background (showing an in-page throbber while it loads). When the crash submits successfully it redirects you to the crash report page. Internally it does this by loading an <iframe> with a form, sticking in the minidump file and the data from the .extra file, and submitting it.

I wound up having to implement multipart/form-data processing in an SJS in order to accept the crash report submission and test it properly. The test loads a fake pending report into pending/, then loads about:crashes and clicks on the report. The SJS prints out the information it received in JSON format when you load it with ?id=xxx, so after the redirect the test checks that the data we intended to send made it. It also tests that navigating back to about:crashes using the back button has updated the crash entry to point to the new report, so you can't resubmit by going back.

I still need to do some cleanup and error handling here, but it's mostly finished.
Ok, this cleans up my debug dumps and fixes a few little thinngs. The error handling is still not very good, it doesn't give any visual indicator that anything happened. I'm not really sure what the right behavior is there. We could just display a little warning icon or something next to the crash report?

Anyway, this works and the tests pass and I think they're fairly comprehensive.
Attachment #383978 - Attachment is obsolete: true
Attachment #384466 - Flags: review?(dtownsend)
Comment on attachment 384466 [details] [diff] [review]
add resubmission of pending reports to about:crashes

Most of the actual fix looks ok, though I really dislike using an iframe for this. Didn't smaug make it easy to send files over xmlhttprequest? The test however seems broken and I'm not sure that your understanding multipart/form-data correctly (or I'm not, but fixing the test should answer that). Also please turn on strict JS errors and fix the warnings that show up from the test run.

>diff --git a/toolkit/crashreporter/content/crashes.js b/toolkit/crashreporter/content/crashes.js

>+function parseKeyValuePairs(text) {
>+  var lines = text.split('\n');
>+  var data = {};
>+  for (let i=0; i<lines.length; i++) {

Nit: spaces around operators, same elsewhere throughout the patch.

>+    if (lines[i] == '')
>+      continue;
>+
>+    [key,value] = lines[i].split('=',2);

Nit: space after the comma, and similar throughout the patch.

>+    if (value)
>+      data[key] = value.replace("\\n", "\n").replace("\\\\", "\\");
>+  }
>+  return data;
>+}

>+// Since we're basically re-implementing part of the crashreporter
>+// client here, we'll just steal the strings we need from crashreporter.ini
>+function getL10nStrings() {
>+  let dirSvc = Cc["@mozilla.org/file/directory_service;1"]
>+                 .getService(Ci.nsIProperties);
>+  let path =  dirSvc.get("GreD", Ci.nsIFile);
>+  path.append("crashreporter.ini");
>+  if (!path.exists()) {
>+    // see if we're on a mac
>+    path = path.parent;
>+    path.append("crashreporter.app");
>+    path.append("Contents");
>+    path.append("MacOS");
>+    path.append("crashreporter.ini");
>+    if (!path.exists()) {
>+      // very bad, but I don't know how to recover
>+      return;
>+    }
>+  }
>+  let crstrings = parseKeyValuePairsFromFile(path);

Is there a reason not to use the INI parser component here?

>+function getPendingMinidump(id) {
>+  let directoryService = Cc["@mozilla.org/file/directory_service;1"].
>+                           getService(Ci.nsIProperties);
>+  let pendingDir = directoryService.get("UAppData", Ci.nsIFile);
>+  pendingDir.append("Crash Reports");
>+  pendingDir.append("pending");

Not too fussed but it may be easier just to cache pendingDir globally like we do for reportsDir.

>+    onStateChange: function(aWebProgress, aRequest, aFlag, aStatus) {
>+      if(aFlag & STATE_STOP) {
>+        iframe.docShell.removeProgressListener(myListener);
>+        link.className = "";
>+
>+        //XXX: give some indication of failure?

Maybe just a crappy alert is enough. Presumably the user can't do anything about it but at least it can tell them something went wrong.

>+function submitPendingReport(event) {
>+  var link = event.target;
>+  var id = link.firstChild.textContent;

Just use link.id

>+function findInsertionPoint(reports, date) {
>+  if (reports.length == 0)
>+    return 0;
>+
>+  var min = 0;
>+  var max = reports.length - 1;
>+  while (min < max) {
>+    var mid = parseInt((min + max) / 2);
>+    if (reports[mid].date < date)
>+      max = mid - 1;
>+    else if (reports[mid].date > date)
>+    min = mid + 1;

Nit: fix the indent here

>+function populateReportList() {
>+  var prefService = Cc["@mozilla.org/preferences-service;1"].
>+    getService(Ci.nsIPrefBranch);

Nit: The general style is to have the dot at the end of the first line and line up the start of the next line with the start of Cc. You break one or both of those things throughout the patch.

>+  if (pendingDir.exists() && pendingDir.isDirectory()) {
>+    var entries = pendingDir.directoryEntries;
>+    while (entries.hasMoreElements()) {
>+      var file = entries.getNext().QueryInterface(Ci.nsIFile);
>+      var leaf = file.leafName;

Should we do anything if a pending item has the same ID as a submitted item (maybe because the pending file couldn't be deleted)?

>+  var formatter = Cc["@mozilla.org/intl/scriptabledateformat;1"].
>+    createInstance(Ci.nsIScriptableDateFormat);
>+  var body = document.getElementById("tbody");
>+  var ios = Cc["@mozilla.org/network/io-service;1"].
>+              getService(Ci.nsIIOService);
>+  var aboutThrottling = ios.newURI(reportURL, null, null);
>+  aboutThrottling.path = "/about/throttling";

Is this right? Are Socorro instances always at the root of a webhost. If not you should use a resolve a relative URL against the reportURL to do this, or just add another pref.

>diff --git a/toolkit/crashreporter/jar.mn b/toolkit/crashreporter/jar.mn
>--- a/toolkit/crashreporter/jar.mn
>+++ b/toolkit/crashreporter/jar.mn
>@@ -1,2 +1,5 @@
> toolkit.jar:
>   content/global/crashes.xhtml              (content/crashes.xhtml)
>+  content/global/crashes.js                    (content/crashes.js)
>+  content/global/crash-submit-form.xhtml              (content/crash-submit-form.xhtml)
>+

Line these up please

>diff --git a/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js b/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js

>+function check_submit_pending(tab, crashes) {
>+   let browser = gBrowser.getBrowserForTab(tab);

Nit: indent fail

>+  let CrashID = null;
>+  let CrashURL = null;
>+  function csp_onload() {
>+    if (browser.contentWindow.location != 'about:crashes') {
>+      browser.removeEventListener("load", csp_onload, true);
>+      // loaded the crash report page
>+      ok(true, 'got submission onload');
>+      // grab the Crash ID here to verify later
>+      CrashID = browser.contentWindow.location.search.split("=")[1];
>+      CrashURL = browser.contentWindow.location.toString();
>+      // check the JSON content vs. what we submitted
>+      let result = JSON.parse(browser.contentDocument.documentElement.textContent);
>+      is(result.upload_file_minidump, "MDMP", "minidump file sent properly");
>+      is(result.Throttleable, 0, "correctly sent as non-throttleable");
>+      delete result.upload_file_minidump;
>+      delete result.Throttleable;
>+      for each(let x in result) {
>+        is(result[x], crash[x], "submitted values match expected");
>+      }

You check that every property in result exists in crash, but not that every property in crash exists in result. If you had you'd quickly spot that crash is an nsIFile at this point. Perhaps you meant to use sent?

>+      executeSoon(function() {
>+                    browser.addEventListener("pageshow", csp_pageshow, true);
>+                    // now navigate back
>+                    browser.goBack();
>+                  });
>+    }
>+  }
>+  browser.addEventListener("load", csp_onload, true);
>+  function csp_pageshow() {
>+    browser.removeEventListener("pageshow", csp_pageshow, true);
>+    executeSoon(function () {
>+                  is(browser.contentWindow.location, "about:crashes", "navigated back successfully");
>+                  let link = browser.contentDocument.getElementById(CrashID);
>+                  isnot(link, null, "crash report link changed correctly");
>+                  if (link)
>+                    is(link.href, CrashURL, "crash report link points to correct href");
>+                  cleanup_and_finish();
>+                });
>+  }
>+
>+  // try submitting the pending report
>+  let id = null;
>+  for each(crash in crashes) {
>+    if (crash.pending) {
>+      id = crash.id;
>+      break;
>+    }
>+  }

I think with no declaration this crash is global scoped, declare it with var or let.

>+  let dirSvc = Cc["@mozilla.org/file/directory_service;1"]
>+    .getService(Ci.nsIProperties);
>+  let crash = dirSvc.get("UAppData", Ci.nsILocalFile);
>+  crash.append("Crash Reports");
>+  crash.append("pending");
>+  crash.append(id + ".extra");

And maybe give this crash a different name to stop my head hurting.

>+  //XXX: is this cheating? I don't know. Seems silly to copy and paste this code
>+  let sent = browser.contentWindow.wrappedJSObject.parseKeyValuePairsFromFile(crash);

Yeah I don't like this. The properties are just those passed to addPendingCrashreport right? Why not just pass them into the function?

>+function test() {
>+  waitForExplicitFinish();
>+  let appD = make_fake_appdir();
>+  let crD = appD.clone();
>+  crD.append("Crash Reports");
>+  let crashes = add_fake_crashes(crD, 1);
>+  // we don't need much data here, it's not going to a real Socorro
>+  crashes.push(addPendingCrashreport(crD, {'ServerURL': 'http://example.com/browser/toolkit/crashreporter/test/browser/crashreport.sjs',
>+                                             'ProductName': 'Test App'
>+                                       }));

Nit: the indentation is a bit screwy here

The sjs looks pretty much ok but I suspect it needs changes to fix the test so I'll review that further on the next round.
Attachment #384466 - Flags: review?(dtownsend) → review-
Flags: blocking1.9.1.1+
Ted: Where's this patch at?
blocking1.9.1: --- → .2+
Flags: blocking1.9.1.1+ → blocking1.9.1.1-
On the beach, y'know. Will try to get it turned around this week. (Catching up from vacation + attending OSCON).
Ted: we really, really, really want this, but would need it PDQ. Welcome back from vacation. If you get to this soon, please mark the patch for approval1.9.1.2?
blocking1.9.1: .2+ → needed
Ted: Where are we on a patch here?
Flags: wanted1.9.1.x+
New patch in a minute, just wanted to reply to some specific points here. If I didn't reply, I probably just fixed whatever you mentioned.

(In reply to comment #34)
> (From update of attachment 384466 [details] [diff] [review])
> Most of the actual fix looks ok, though I really dislike using an iframe for
> this. Didn't smaug make it easy to send files over xmlhttprequest?

I asked bz and smaug about this, and they said there's no such thing yet. Note that I specifically need to send a multipart/form-data request, since that's what the server handles. I agree that using an iframe sucks.

> The test
> however seems broken and I'm not sure that your understanding
> multipart/form-data correctly (or I'm not, but fixing the test should answer
> that).

I fixed everything you pointed out, and the tests still pass, so either I still missed something or your understanding was clouded by the mess of other sloppiness in my previous patch!

> >+        //XXX: give some indication of failure?
> 
> Maybe just a crappy alert is enough. Presumably the user can't do anything
> about it but at least it can tell them something went wrong.

As noted on IRC, I'd like to get this on branch, so I'm trying to avoid L10N changes at the moment. We'll revisit in a followup.

> >+  if (pendingDir.exists() && pendingDir.isDirectory()) {
> >+    var entries = pendingDir.directoryEntries;
> >+    while (entries.hasMoreElements()) {
> >+      var file = entries.getNext().QueryInterface(Ci.nsIFile);
> >+      var leaf = file.leafName;
> 
> Should we do anything if a pending item has the same ID as a submitted item
> (maybe because the pending file couldn't be deleted)?

The IDs for submitted items are returned from the server, so they'll be different from the pending IDs (they're both UUIDs, just generated in different places).

> >+  var aboutThrottling = ios.newURI(reportURL, null, null);
> >+  aboutThrottling.path = "/about/throttling";
> 
> Is this right? Are Socorro instances always at the root of a webhost. If not
> you should use a resolve a relative URL against the reportURL to do this, or
> just add another pref.

It's hard to say, since we only have one Socorro instance in existence with this page. I changed to a relative URL for now, it should work either way. I'd rather not add another pref, seems like overkill.

Thanks for the thorough review, hope you like my new patch. :)
Addresses review comments.
Attachment #384466 - Attachment is obsolete: true
Attachment #394084 - Flags: review?(dtownsend)
Comment on attachment 394084 [details] [diff] [review]
add resubmission of pending reports to about:crashes, rev 2

Looks good, just a few nits to clear up still.

>diff --git a/toolkit/crashreporter/content/crashes.js b/toolkit/crashreporter/content/crashes.js

>+function parseKeyValuePairs(text) {
>+  var lines = text.split('\n');
>+  var data = {};
>+  for (let i = 0; i < lines.length; i++) {
>+    if (lines[i] == '')
>+      continue;
>+
>+    [key, value] = lines[i].split('=', 2);
>+    if (value)
>+      data[key] = value.replace("\\n", "\n").replace("\\\\", "\\");

This is going to fail to convert the string "\\n" I believe. I'll leave it up to you to decide if that is a real problem or not.

>+  var data = strings.crashid.replace("%s", crashID);
>+  if (viewURL) {
>+     data += "\n" + strings.reporturl.replace("%s", viewURL);
>+  }

Nit: No surrounding braces for a single line block

>diff --git a/toolkit/crashreporter/test/browser/aboutcrashes_utils.js b/toolkit/crashreporter/test/browser/aboutcrashes_utils.js

>   for (let i = 0; i < count; i++) {
>     let uuid = uuidGenerator.generateUUID().toString();
>     // ditch the {}
>-    uuid = uuid.substring(1,uuid.length-2);
>-    let fn = "bp-" + uuid + ".txt";
>+    uuid = "bp-"+uuid.substring(1,uuid.length-2);

I know it was wrong before, but put spaces around the operators and after the ,.

>+function writeDataToFile(file, data) {
>+  var fstream = Cc["@mozilla.org/network/file-output-stream;1"]
>+    .createInstance(Ci.nsIFileOutputStream);
>+  // open, write, truncate
>+  fstream.init(file, -1, -1, 0);
>+  var os = Cc["@mozilla.org/intl/converter-output-stream;1"]
>+    .createInstance(Ci.nsIConverterOutputStream);

You missed formatting some of these properly in this file.

>+function addPendingCrashreport(crD, extra) {
>+  let pendingdir = crD.clone();
>+  pendingdir.append("pending");
>+  let date = Date.now() - Math.round(Math.random() * 10 * 60000);
>+  let uuidGenerator = Cc["@mozilla.org/uuid-generator;1"]
>+                        .getService(Ci.nsIUUIDGenerator);
>+  let uuid = uuidGenerator.generateUUID().toString();
>+  // ditch the {}
>+  uuid = uuid.substring(1,uuid.length-2);

Same as above.

>diff --git a/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js b/toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js

>+  if (crashlinks.length == crashes.length) {
>+    for(let i=0; i<crashes.length; i++) {
>+      is(crashlinks[i].id, crashes[i].id, i + ": crash ID is correct");
>+      if (crashes[i].pending) {
>+        // we set the breakpad.reportURL pref in test()
>+        is(crashlinks[i].getAttribute("href"), "http://example.com/browser/toolkit/crashreporter/about/throttling", "pending URL links to the correct static page");

Do some wrapping here to reduce the line length.

>+      // Likewise, this is discarded before it gets to the server
>+      delete SubmittedCrash.extra.ServerURL;
>+
>+      for(let x in result) {
>+        if (x in SubmittedCrash.extra)
>+          is(result[x], SubmittedCrash.extra[x], "submitted values match expected");

It's probably helpful to include x in the message to help debugging in the future.

>+        else
>+          ok(false, "property " + x + " missing from submitted data!");
>+      }
>+      for(let y in SubmittedCrash.extra) {
>+        if (y in result)
>+          is(SubmittedCrash.extra[y], result[y], "submitted values match expected");

Technically you don't need to test this case, it will have been tested above.
Attachment #394084 - Flags: review?(dtownsend) → review+
Pushed to m-c with those nits fixed:
http://hg.mozilla.org/mozilla-central/rev/21ad4f1ce214

Assuming no problems on trunk I'll try to get it landed on branches soon.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Backed out due to leaks:
http://hg.mozilla.org/mozilla-central/rev/649fa161f1bd
http://hg.mozilla.org/mozilla-central/rev/a6fa91f76877
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Something like this:
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1312 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 160 instances of nsStringBuffer with size 8 bytes each (1280 bytes total)
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 8 instances of nsTArray_base with size 4 bytes each (32 bytes total)
Tracked down the leak, it was in nsINIParser. Fixed with bsmedberg's help, and re-pushed this patch:
http://hg.mozilla.org/mozilla-central/rev/fe54d679cd16 (leak fix)
http://hg.mozilla.org/mozilla-central/rev/32e91c748a42
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Verified fixed on trunk with builds on Windows and OS X like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090821 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20090821041741

I raise the severity because it has already blocked us to check some of the user crashes because they weren't submitted. Tests have been already checked in too.
Severity: normal → major
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: blocking1.9.2?
Target Milestone: --- → mozilla1.9.3a1
Is there supposed to be some way of identify pending reports and if so was that part of the fix supposed to be retroactive?  

I tried it out on my system which had a number of pending reports and while the about:crash screen listed them, they all showed as submitted and were intermixed with the submitted reports.  I couldn't figure out which were pending and which had been submitted until I used the "Remove Reports" button which removed the submitted reports, but left the pending ones.
No, this patch didn't add any clear way to identify pending reports. It simply lists them along with the submitted reports. You can identify them by mousing over them and noting the URL in the status bar. I'm open to ideas as to how to make this better. (File new bugs if you have ideas.)
A new bug would be great. We could use CSS to style those pending reports in the list which would make them more discoverable.
Blocks: 512479
I filed bug 512479 with some suggestions on making pending reports easier to
identify.
Ted: Is this ready for 1.9.1? Can you create a roll-up patch?
I have a small followup fix from the security review (nothing major, just hygiene) that I'd like to land first. What's the branch policy, can I land on 1.9.2 and 1.9.1 at the same time?
Typically we'd prefer it to land on 1.9.2 before 1.9.1. Code freeze for 1.9.1.4 isn't until September 22, so you have some time.
Comment on attachment 394084 [details] [diff] [review]
add resubmission of pending reports to about:crashes, rev 2

Seeking approval1.9.2 for this patch and the followup fixes in bug 512853. It's been on trunk for a while, has had a security review, and has automated tests.
Attachment #394084 - Flags: approval1.9.2?
Comment on attachment 394084 [details] [diff] [review]
add resubmission of pending reports to about:crashes, rev 2

a192=beltzner; doesn't look like there are strings here, but if there are, please get this landed today.
Attachment #394084 - Flags: approval1.9.2? → approval1.9.2+
No strings, will land when I have time.
Attachment #394084 - Flags: approval1.9.0.14?
Comment on attachment 394084 [details] [diff] [review]
add resubmission of pending reports to about:crashes, rev 2

Seeking 1.9.1 approval, landed on trunk & 1.9.2 already.
Attachment #394084 - Flags: approval1.9.0.14? → approval1.9.1.4?
Comment on attachment 394084 [details] [diff] [review]
add resubmission of pending reports to about:crashes, rev 2

Moving approval request to 1.9.1.4, since I think that's what Ted intended.
Flags: blocking1.9.2? → blocking1.9.2+
blocking1.9.1: needed → .4+
Comment on attachment 394084 [details] [diff] [review]
add resubmission of pending reports to about:crashes, rev 2

scary, better sooner than later.

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #394084 - Flags: approval1.9.1.4? → approval1.9.1.4+
Verified fixed on 1.9.2 with builds on Windows and OS X like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090909 Namoroka/3.6a2pre ID:20090909040930

Ted, as date we don't use the crash date but the submit date. Shouldn't we use the value of CrashTime for the date part? Shall I file a bug?
Keywords: verified1.9.2
Hm. That's a fair point. We currently just use the file creation time from the file we save in the submitted directory. Once we've submitted the file we don't save the CrashTime, so we don't currently have a way to do that. I guess we could change the file's ctime or mtime or whatever when we submit via about:crashes, so that it matches the crash time. A new bug would be a good place to deal with that, though, so yes.
Ted, do we have any problems actually with the Soccorro? I'm not able to send pending crash reports at all on Linux. On OS X and Windows the report seems to have been sent but it is processed forever. See the following incident id: bp-b9d74f6f-d207-436a-b0b3-539872090204.

That happens for all version of Shiretoko, Namoroka, and Minefield.
There was maintenence being done on Socorro last night, that might be related. If you can't resubmit on Linux though, that might be a new bug.
Ok, the problem on Linux was because of a missing library Breakpad needs to send the report. See bug 517493.

Verified fixed on 1.9.1 with builds on all platforms like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090916 Shiretoko/3.5.4pre (.NET CLR 3.5.30729) ID:20090916041504

Thanks Ted! That's a really helpful feature we have now on all branches.
Keywords: verified1.9.1
You need to log in before you can comment on or make changes to this bug.