Crash Reporting broken? "message.target.sendAsyncMessge is not a function" in ContentCrashHandlers.jsm

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
Crash Reporting
P4
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: freddyb, Assigned: poiru)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(e10s+, firefox47 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

I have a crash that I found randomly on the internet (actually, from webkits crash/security test suite, see https://twitter.com/blubbfiction/status/691895499104849920 for more).

I wanted to investigate and report the crash, but it turns out crash reporting might be broken?


STR: 
1) Get something to crash a content process, e.g., with http://fallout.patzke.org/security/browsercrasher/tests/svg-deeply-nested-crash.html
2) Observe how it doesn't store the crash in about:crashes
3) The browser console reports "message.target.sendAsyncMessge is not a function" in ContentCrashHandlers.jsm, line 231.
gfritzsche suggested on IRC that I flag this breakage of crash reporting. See comment 0 for steps to repeat.
tracking-e10s: --- → ?
Component: IPC → General
Flags: needinfo?(mconley)
Product: Core → Firefox

Comment 2

3 years ago
The bug is a simple typo. I'm concerned that there isn't any automated test coverage for this, though. poiru could you take a look please?
Assignee: nobody → birunthan
Component: General → Breakpad Integration
Product: Firefox → Toolkit
(Assignee)

Comment 5

3 years ago
Created attachment 8712714 [details] [diff] [review]
Fix broken about:crashes due to typo in ContentCrashHandlers.jsm

This only affects crashes without dumps. I'm not aware of a way to disable dump generation in tests so this just fixes the typo. We could also mess with TabCrashHandler in the test. Mike, any thoughts?
Flags: needinfo?(birunthan)
Attachment #8712714 - Flags: review?(mconley)
(In reply to Birunthan Mohanathas [:poiru] from comment #5)
> Created attachment 8712714 [details] [diff] [review]
> Fix broken about:crashes due to typo in ContentCrashHandlers.jsm
> 
> This only affects crashes without dumps. I'm not aware of a way to disable
> dump generation in tests so this just fixes the typo. We could also mess
> with TabCrashHandler in the test. Mike, any thoughts?

You should be able to test this manually by using SIGKILL on the content process which should not result in a crash report being generated.
That should work on POSIX, but for Windows you will probably have to use TerminateProcess.
Comment on attachment 8712714 [details] [diff] [review]
Fix broken about:crashes due to typo in ContentCrashHandlers.jsm

Review of attachment 8712714 [details] [diff] [review]:
-----------------------------------------------------------------

This looks right, but I think we should get an automated test case here.

Here's what I suggest:

In ContentCrashHandlers.jsm, add a new method to the TabCrashHandler that takes a browser, and will clear out the entry in the childMap for that browser's childID (there's this weird map to map conversion through browserMap you need to do - check out getDumpID for an example).

Modify BrowserTestUtils.jsm's crashBrowser to take an optional argument that, if set, will call the function in TabCrashHandler.

Finally, we should finally protect ourselves by making sure that the method added to TabCrashHandler can only be called if we're testing, so perhaps check for a pref at the start of that function, and throw if it's not detected. Have crashBrowser set that pref before calling the function, and then reset it when it's done clearing.

Sound good?
Attachment #8712714 - Flags: review?(mconley) → review+
tracking-e10s: ? → +
Flags: needinfo?(mconley)
(Assignee)

Comment 9

3 years ago
Created attachment 8713510 [details] [diff] [review]
Fix broken about:crashes due to typo in ContentCrashHandlers.jsm

I went with a different approach, what do you think? I find this to be better than adding test-only code to ContentCrashHandlers.jsm, but I am happy to follow comment 8 if you prefer.
Flags: needinfo?(mconley)
Attachment #8713510 - Flags: review?(mconley)
(Assignee)

Updated

3 years ago
Attachment #8712714 - Attachment is obsolete: true
Comment on attachment 8713510 [details] [diff] [review]
Fix broken about:crashes due to typo in ContentCrashHandlers.jsm

Review of attachment 8713510 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this approach is fine, since it's test only. I do think we should create a new test file though to avoid test timeouts (which browser_aboutTabCrashed.js is already bad at).

::: browser/base/content/test/general/browser_aboutTabCrashed.js
@@ +28,5 @@
>    // slows down the test and results in intermittent test timeouts,
>    // so we'll pump up the expected timeout for this test.
>    requestLongerTimeout(2);
>  
> +  TabCrashHandler.original_getDumpID = TabCrashHandler.getDumpID;

I think I'm fine with this sort of monkey-patching in tests so long as we document it very clearly.

@@ +135,5 @@
>  
>  /**
> + * Tests tab crash page when a dump is not available.
> + */
> +add_task(function* test_without_dump() {

This test file, browser_aboutTabCrashed.js, already runs too long especially with debug builds. Can you write a new test file in browser/base/content/test/general/ to test this case?
Attachment #8713510 - Flags: review?(mconley) → review-
(Assignee)

Comment 11

3 years ago
Created attachment 8713669 [details] [diff] [review]
Fix broken about:crashes due to typo in ContentCrashHandlers.jsm

Done.
Attachment #8713669 - Flags: review?(mconley)
(Assignee)

Updated

3 years ago
Attachment #8713510 - Attachment is obsolete: true
Comment on attachment 8713669 [details] [diff] [review]
Fix broken about:crashes due to typo in ContentCrashHandlers.jsm

Review of attachment 8713669 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great - just some last fixups. Thanks poiru!

::: browser/base/content/test/general/browser_aboutTabCrashed_withoutDump.js
@@ +23,5 @@
> +    url: PAGE,
> +  }, function*(browser) {
> +    let tab = gBrowser.getTabForBrowser(browser);
> +    yield BrowserTestUtils.crashBrowser(browser);
> +    let doc = browser.contentDocument;

If we can, we should avoid using CPOWs to interact with the content like this. Instead, we should use ContentTask, like this:

```JavaScript
yield ContentTask.spawn(browser, null, function*() {
  let doc = content.document;
  ok(!doc.documentElement.classList.contains("crashDumpAvailable"),
     "doesn't have crash dump");

  let container = doc.getElementById("crash-reporter-container");
  ok(container, "has crash-reporter-container");
  ok(container.hidden, "crash-reporter-container is hidden");
});

```

etc

@@ +30,5 @@
> +       "doesn't have crash dump");
> +
> +    let container = doc.getElementById("crash-reporter-container");
> +    ok(container, "has crash-reporter-container");
> +    ok(container.hidden, "crash-reporter-container is hidedn");

Typo - "hidedn" -> "hidden"

@@ +33,5 @@
> +    ok(container, "has crash-reporter-container");
> +    ok(container.hidden, "crash-reporter-container is hidedn");
> +
> +    let tabRemovedPromise = BrowserTestUtils.removeTab(tab, { dontRemove: true });
> +    doc.getElementById("closeTab").click();

You can avoid the CPOW here with:

```JavaScript
yield BrowserTestUtils.synthesizeMouseAtCenter("#closeTab", {}, browser);
```
Attachment #8713669 - Flags: review?(mconley) → review-
(Assignee)

Comment 13

3 years ago
Created attachment 8713680 [details] [diff] [review]
Fix broken about:crashes due to typo in ContentCrashHandlers.jsm

Done, thanks for the info!
Attachment #8713680 - Flags: review?(mconley)
(Assignee)

Updated

3 years ago
Attachment #8713669 - Attachment is obsolete: true
Comment on attachment 8713680 [details] [diff] [review]
Fix broken about:crashes due to typo in ContentCrashHandlers.jsm

Review of attachment 8713680 [details] [diff] [review]:
-----------------------------------------------------------------

This LGTM. Thanks poiru! Great job.
Attachment #8713680 - Flags: review?(mconley) → review+
(In reply to Frederik Braun [:freddyb] from comment #0)
> I have a crash that I found randomly on the internet (actually, from webkits
> crash/security test suite, see
> https://twitter.com/blubbfiction/status/691895499104849920 for more).
> 
> I wanted to investigate and report the crash, but it turns out crash
> reporting might be broken?
> 
> 
> STR: 
> 1) Get something to crash a content process, e.g., with
> http://fallout.patzke.org/security/browsercrasher/tests/svg-deeply-nested-
> crash.html
> 2) Observe how it doesn't store the crash in about:crashes
> 3) The browser console reports "message.target.sendAsyncMessge is not a
> function" in ContentCrashHandlers.jsm, line 231.

Hey freddyb, the patch here gets rid of the error message, but was a bug ever filed for the original crash you saw?
Flags: needinfo?(fbraun)
Nah, but there are probably more crashers lurking in there, it's just a copy of samples from webkit's crash & security test:
http://fallout.patzke.org/security/browsercrasher/crasher.html

It'll need some diligence and automation to file all the crashes that come out of there.
Flags: needinfo?(fbraun)
tracking-e10s: + → ?
This is also on Aurora/Developer Edition and it's probably skewing our crash stats numbers.

Does this warrant an uplift?
(In reply to Frederik Braun [:freddyb] from comment #17)
> This is also on Aurora/Developer Edition and it's probably skewing our crash
> stats numbers.
> 
> Does this warrant an uplift?

To be clear, what this bug is fixing is an error message in the Browser Console, and nothing else.

When about:tabcrashed loads, it defaults to not showing any UI for submitting a crash report (since it hasn't heard yet whether or not a dump exists). Once the page loads, it gets that information from the parent.

What poiru is fixing is the case where there is no dump (this can occur if the content process was killed by SIGKILL, or we couldn't for some reason capture a crash report). In that case, we're throwing when trying to tell about:tabcrashed that there's no dump ID. The only fallout from this is that we won't enter this block in the about:tabcrashed JS: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/browser/base/content/aboutTabCrashed.js#157

Which wouldn't have done anything for the false "hasReport" case, except send up the "AboutTabCrashedReady" custom event, which is really only used by our automated tests to detect when they can safely check for the presence of the crash report submission form.

So, TL;DR:

This patch gets rid of an error message, but the bug itself should not have impacted a users ability to send crash reports, nor our ability to receive them.

The fact that you were able to get a content process crash from http://fallout.patzke.org/security/browsercrasher/tests/svg-deeply-nested-crash.html without a crash report being generated is concerning (which is why I asked about getting a bug on file for it), but I think that's separate from the warning that this patch is fixing.
tracking-e10s: ? → +
Priority: -- → P4
Are you able to reduce the WebKit crasher to a minimal test case that results in a crash report not being generated?
Flags: needinfo?(fbraun)
Yes. The test case is in https://bugzilla.mozilla.org/show_bug.cgi?id=1245474
Flags: needinfo?(fbraun)
(In reply to Frederik Braun [:freddyb] from comment #20)
> Yes. The test case is in https://bugzilla.mozilla.org/show_bug.cgi?id=1245474

Is it possible to give me access to that bug? I'd like to understand why we're not generating a crash dump.
Flags: needinfo?(fbraun)
I have just added you to bug 1245474.
Flags: needinfo?(fbraun)
(Assignee)

Comment 23

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6aff7784c1749558b118d7f9d981a6eba47e4b1
Bug 1242907 - Fix broken about:crashes due to typo in ContentCrashHandlers.jsm. r=mconley
Duplicate of this bug: 1245698

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6aff7784c17
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.