Work - Headless crash reporting in metrofx doesn't report if the browser crashes on startup

RESOLVED WONTFIX

Status

Firefox for Metro
General
P2
normal
RESOLVED WONTFIX
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work)

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
We submit crashes from within the browser vs. launching an external program. Hence, if the browser crashes on startup, we'll never get a crash report. 


reference bug 797013
(Assignee)

Comment 1

6 years ago
I wonder if we could switch from using browser.js submission to launching the desktop client in some sort of silent mode.
Whiteboard: metro-beta
(Assignee)

Updated

6 years ago
Component: General → General
Product: Firefox → Firefox for Metro

Updated

6 years ago
Whiteboard: metro-beta → [metro-mvp]
Whiteboard: [metro-mvp] → [metro-mvp] [LOE:3]
(Assignee)

Updated

6 years ago
Depends on: 801200

Updated

6 years ago
Blocks: 831972
Whiteboard: [metro-mvp] [LOE:3] → [metro-mvp] [LOE:3] feature=work

Updated

6 years ago
Summary: Headless crash reporting in metrofx doesn't report if the browser crashes on startup → Work - Headless crash reporting in metrofx doesn't report if the browser crashes on startup
Whiteboard: [metro-mvp] [LOE:3] feature=work → feature=work
(Assignee)

Updated

6 years ago
Blocks: 844608
(Assignee)

Updated

6 years ago
Assignee: nobody → jmathies
(Assignee)

Comment 2

6 years ago
Created attachment 718404 [details] [diff] [review]
crashreporter cleanup

This eliminates the else block, no logic changes.
(Assignee)

Comment 3

6 years ago
Created attachment 718411 [details] [diff] [review]
cr silent mode v.1

This adds a silent mode to crash reporter.
(Assignee)

Comment 4

6 years ago
Created attachment 718412 [details] [diff] [review]
apprunner v.1

This handles launching cr in silent mode early in startup for metro.
(Assignee)

Updated

6 years ago
Depends on: 845308
(Assignee)

Comment 5

6 years ago
I have a front end patch for this as well. However, beofre I get to that there is an unresolved problem here. Processes that are integrated into the metro ui receive undocumented process management behavior. A couple issues:

- despite being launched as an independent process, crashreporter still gets latched into metro ui process suspend/resume behavior. So for example if if you have crashreporter spawned off and metrofx gets suspended, crashreporter gets suspended also. This can cause problems with sending reports.

- also, the crashreporter process will get torn down unceremoniously if metrofx exits for any reason, including, a startup crash.

There must be a way to separate crashreporter from metro's process management, but afaict it's not document anywhere.
we could disable the suspending of the crash reporting process completely. Here's some example code:
http://dxr.mozilla.org/mozilla-central/widget/windows/winrt/MetroAppShell.cpp#l268

> There must be a way to separate crashreporter from metro's process management, but afaict it's not document anywhere.

What about using cmd.exe to start it or something like that?
Maybe there's a createprocess or shellexecute flag as well.
(Assignee)

Comment 7

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #6)
> we could disable the suspending of the crash reporting process completely.
> Here's some example code:
> http://dxr.mozilla.org/mozilla-central/widget/windows/winrt/MetroAppShell.
> cpp#l268
> 
> > There must be a way to separate crashreporter from metro's process management, but afaict it's not document anywhere.
> 
> What about using cmd.exe to start it or something like that?
> Maybe there's a createprocess or shellexecute flag as well.

I was looking around for something like that but didn't find anything. The old desktop enabled browser doc doesn't mention it either. I'm wondering if a support request might be the best route to take.
(Assignee)

Updated

6 years ago
Attachment #718404 - Flags: review?(ted)
(Assignee)

Comment 8

6 years ago
Created attachment 718569 [details] [diff] [review]
client v.1

Got this working using shell execute. (hat tip bbondy)
Attachment #718411 - Attachment is obsolete: true
Attachment #718412 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #718569 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 718570 [details] [diff] [review]
client v.1
(Assignee)

Comment 10

6 years ago
Comment on attachment 718570 [details] [diff] [review]
client v.1

This is the silent submit part in crashreporter.exe.
Attachment #718570 - Flags: review?(ted)
(Assignee)

Comment 11

6 years ago
Created attachment 718572 [details] [diff] [review]
apprunner/exception handler bits v.1
(Assignee)

Comment 12

6 years ago
Comment on attachment 718572 [details] [diff] [review]
apprunner/exception handler bits v.1

sorry ted, if there's someone you can delegate this to, please do.
Attachment #718572 - Flags: review?(ted)
(Assignee)

Comment 13

6 years ago
Created attachment 718573 [details] [diff] [review]
front end v.1
Attachment #718573 - Flags: review?(netzen)
(Assignee)

Comment 14

6 years ago
(In reply to Jim Mathies [:jimm] from comment #13)
> Created attachment 718573 [details] [diff] [review]
> front end v.1

Note the other little bit of front end code to this is over in bug 844619.
Comment on attachment 718573 [details] [diff] [review]
front end v.1

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

yay for removal only patch
Attachment #718573 - Flags: review?(netzen) → review+

Updated

6 years ago
Priority: -- → P2
(Assignee)

Comment 16

6 years ago
Created attachment 718693 [details] [diff] [review]
crashreporter cleanup -w

Makes it a bit easier to see what changed.
Attachment #718404 - Attachment is obsolete: true
Attachment #718404 - Flags: review?(ted)
Attachment #718693 - Flags: review?(ted)
(Assignee)

Updated

6 years ago
Attachment #718570 - Flags: review?(ted)
(Assignee)

Updated

6 years ago
Attachment #718572 - Flags: review?(ted)
(Assignee)

Updated

6 years ago
Attachment #718693 - Flags: review?(ted)
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
This is WONTFIX per the discussion in bug 831972?
(Assignee)

Comment 18

6 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> This is WONTFIX per the discussion in bug 831972?

I think so. We could still land it, tie it to a registry key so we have it for debugging. If the user sets a reg key that's the same as opting in. However from the discussion in bug 831972, it's clear our normal submit method will have to be an about page where the user approves every submit.

Would you be up for landing this even if it's not going to be used on a regular basis?
The crashreporter code is already pretty complicated, I'd rather avoid additional features we don't actually need.
(Assignee)

Updated

5 years ago
No longer blocks: 844608
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.