1757786 - extract payload handles parts wrong

Status: RESOLVED FIXED

(Socorro :: Antenna, task, P1)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

Sentry: https://sentry.prod.mozaws.net/operations/socorro-new-stage/issues/18728170/?environment=production

JSONDecodeError: Expecting value: line 1 column 1 (char 0)
  File "socorro/external/crashstorage_base.py", line 478, in save_processed_crash
    a_store.save_processed_crash(raw_crash, processed_crash)
  File "socorro/external/crashstorage_base.py", line 639, in benchmarker
    return wrapped_attr(*args, **kwargs)
  File "socorro/external/boto/crashstorage.py", line 331, in save_processed_crash
    crash_report = json_schema_reducer.make_reduced_dict(
  File "json_schema_reducer.py", line 12, in make_reduced_dict
    return _make_reduced_dict(
  File "json_schema_reducer.py", line 27, in _make_reduced_dict
    new_dict[key] = make_reduced_dict(
  File "json_schema_reducer.py", line 14, in make_reduced_dict
    dictify(original)
  File "json_schema_reducer.py", line 48, in dictify
    thing = json.loads(thing)
  File "__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None

This is rough. In production, the raw crash has the value of memory_report as null (JS). The socorro-submitter packaged that up and sent it to stage. In stage, the raw crash has the value of memory_report as "None".

That "None" triggers this issue in the json_schema_reducer.

Why is the value different in prod and stage? The crash report is "payload: multipart" so it's not using the JSON-encoded extra value for annotations. It shouldn't be a null.

Is this a change from the extract_payload rewrite?

Comment 1

[Will Kahn-Greene [:willkg] ET needinfo? me]

It's the memory_report attachment--somehow it's ending up as None sometimes. Here's some:

• edd36100-2cb7-4a8e-8657-7c9d70220302
• 1e9c5a91-a39d-4f43-a5e0-676b70220302
• e2019947-cc06-4883-b0d0-8364e0220302

I don't see any prior to the extract_payload rewrite, but if it only sometimes happens maybe I'm just not lucky.

This code looks fishy:

https://github.com/falconry/falcon/blob/master/falcon/media/multipart.py#L284

I can't figure out what the crash reports look like in this scenario. I'll write a patch that will make the handling for annotations more explicit and also send errors to sentry for things I don't recognize.

Comment 2

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 802: bug 1757786: restrict annotation handling and add debugging

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #802: "bug 1757786: restrict annotation handling and add debugging" in 22524cc.

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

This deployed to stage. I verified that the item doesn't get added as an annotation and it does send an error to Sentry if the content type isn't recognized.

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

I deployed this much to prod in bug #1757819. Now we're seeing all manner of content types.

Redoing the summary because this is really a problem with extract_payload and not TelemetryBotoS3CrashStorage.

Comment 6

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 804: bug 1757786: adjust the payload handling to be more permissive

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #804: "bug 1757786: adjust the payload handling to be more permissive" in c0fbefc.

Comment 8

[Will Kahn-Greene [:willkg] ET needinfo? me]

That last change makes the code more permissive. I was seeing all sorts of content types. I suspect the crash reporter clients assemble payloads using whatever system libraries they have available or something like that.

Comment 9

[Will Kahn-Greene [:willkg] ET needinfo? me]

I deployed the last change in bug #1757826. I don't see any Sentry errors anymore.

I need to find out what content types are acceptable and adjust the tests and crash report payload accordingly.

Comment 10

[Will Kahn-Greene [:willkg] ET needinfo? me]

Gabriele pointed out this code:

https://searchfox.org/mozilla-central/rev/ad7ecfa618ec3a65db8405d9f1125059fe4a6a15/toolkit/crashreporter/CrashSubmit.jsm#238-268

It explicitly sets the mimetype for the part for the JSON-encoded extra field, but doesn't set the mimetype for dump parts.

I want to tweak the extract_payload code so that it keeps a log of weird things it's seeing (weird content types, cases where a dump looks like a part header, etc) and then passes that along in the raw crash so we can see these issues (and other issues) better.

Comment 11

[Will Kahn-Greene [:willkg] ET needinfo? me]

I wrote up bug #1758008 for the CrashSubmit.jsm changes.

Comment 12

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 6034: bug 1757786: memory_report content type should be application/gzip

Comment 13

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #6034: "bug 1757786: memory_report content type should be application/gzip" in faa261e.

This updates the payload specification with a note about memory_report files.

Comment 14

[Will Kahn-Greene [:willkg] ET needinfo? me]

The last bit in comment #10 is covered in bug #1760814.

I think everything in this bug is accounted for now, so we can close it out.