Closed Bug 741741 Opened 10 years ago Closed 10 years ago

Crashreporter UI shows up on desktop for metro applications

Categories

(Toolkit :: Crash Reporting, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [completed-elm][leave open])

Attachments

(2 files, 2 obsolete files)

I think the best interim solution here would be to just submit any crash reports automatically and silently since we can't expect users to "check the desktop" whenever something goes wrong. about:crashes can provide the information users need on crashes.
We can (and probably should) write a native Metro crashreporter app. It's not terribly complicated, we have native implementations for Win32/Cocoa/Gtk2/Maemo/Android:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/CrashReporter.java.in

All it needs to do is parse a few simple text files for some key=value pairs, display some simple UI, and submit a HTTP POST to send the report. The Android client does all of this in Java. The other implementations share some C++ code just for convenience, but none of them link to Gecko (for obvious reasons).
We can probably work something like that up. We have apis that allow us to request an app be activated in the interface. Presumably Windows won't block launching something like that. Not sure about bringing a metro app down with our install and launching it in the interface though, but we can look at it.
I don't know the first thing about how Metro apps work, but on Android we compile the crashreporter into the same app we ship, just as a different "activity" that we can trigger separately from the main app.
Warning: scope creep...

We've talked a bit in the past about wanting to move some of this code to in-product, since it's a bit of a pain to have to maintain code for each platform. The general idea would be to prompt/submit through Gecko on the next launch, yadda yadda lots of handwaving.

The big downside would be for startup crashes, where Gecko can't get running enough to manage to submit a report. But maybe for that case we can have a simplified native reporter? It, arguably, might not need to even prompt... If you're crashing on startup, it seems far less likely that the crash would have sensitive data in it (assuming we try to submit before restoring anything from a previous session).

I guess that even if we did that, for Win8 we'd still (?) need a native reporter to handle this, albeit perhaps without any UI. Not sure if that actually makes anything easier or not.
The big issue with a metro style app is that all standard metro apps must come down through the ms store. There is one way around this for enterprises, details of which can be found here - 

http://technet.microsoft.com/en-us/library/hh852635.aspx

There is something called "sideloading product activation key" which allows normal, non-domained systems to install metro apps from outside the store, but it doesn't sound very friendly for what we would want to do.

I'm going to do some more poking around on this, but it looks like our options are limited:

1) send everything silently without displaying UI.
2) place the UI in process and display it on the next restart per Justin's comments.

I'm curious how we would detect the difference between a startup crash and a normal crash? Is there some standard way to do that?

For simplicity maybe we should do all submissions through a background app that displays no UI. This could handle auto startup crash reporting (controlled by a pref for privacy geeks?), and could also be triggered by the browser for other crashes. 

Maybe this functionality can be in process? I sense startup crashes related to networking issues, malware, and the like, would not be reliable without a early startup type secondary app doing the submitting. I haven't delt much with startup crashes though so I'm not sure.

I think we need to get something basic put together because currently crash reporting is unreliable. This is going hurt the quality of our crash report data once we get nightlies going in a few weeks or so.
For initial in-process UI, I'm thinking of something really super simple to start with, maybe just the about:crashes page with an added button that reads "submit all". The page would open up as the home page when a recent crash is detected. This would be metro specific for now.
This sounds pretty similar to what we want to do for B2G, actually. See bug 761909.
(In reply to Ted Mielczarek [:ted] from comment #7)
> This sounds pretty similar to what we want to do for B2G, actually. See bug
> 761909.

Looks like CrashSubmit.jsm relies on browser networking vs. an external app. Re my comments below, is this considered generally safe to do? (Assuming we detect startup crashes properly and submit those immediately w/o ui via crashreporter.exe.)
Yeah, that should be fine. Handling startup crashes is obviously harder, but I have to do some work to make this work for B2G anyway, so we might as well leverage that.

My thought is just to have the exception handler save the dump and leave a note that we crashed, and on restart we can notice that and send out an observer service notification with the crash ID that you can use to submit it.
(In reply to Ted Mielczarek [:ted] from comment #9)
> Yeah, that should be fine. Handling startup crashes is obviously harder, but
> I have to do some work to make this work for B2G anyway, so we might as well
> leverage that.

Is this something you were planning to work on soon? If not I'd be happy to take a shot at working something up.

> My thought is just to have the exception handler save the dump and leave a
> note that we crashed, and on restart we can notice that and send out an
> observer service notification with the crash ID that you can use to submit
> it.

Are you suggesting doing the submit automatically, or on the event open a tab with some ui and giving the user the option? (A tab could also give them a chance to leave notes.)(In reply to Ted Mielczarek [:ted] from comment #9)
Yes, it's on my plate for B2G work. I'd guess making it support Metro wouldn't be much more work.

For B2G we were going to do silent submission (pref controlled) in the short-term, we probably want some UI for Metro.
(In reply to Ted Mielczarek [:ted] from comment #11)
> Yes, it's on my plate for B2G work. I'd guess making it support Metro
> wouldn't be much more work.
> 
> For B2G we were going to do silent submission (pref controlled) in the
> short-term, we probably want some UI for Metro.

Ok, we can make this bug about additional metro specific work once the core work is done in bug 761909.
Depends on: 761909
Instead of having the define NO_CRASHREPORTER_CLIENT, I'm thinking the way to solve this with metro is to make that a static bool and in the init of exception handler, have something like:

#ifdef MOZ_WIDGET_GONK
  sHeadlessClient = true;
#endif
#ifdef MOZ_METRO
  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro)
    sHeadlessClient = true;
#endif

Then check sHeadlessClient in place of NO_CRASHREPORTER_CLIENT in the rest of the code.

https://bugzilla.mozilla.org/attachment.cgi?id=642154&action=diff

Ted, sound like a good approach?
Yeah, that sounds great. I took the hack-and-slash approach for my first iteration of the patch. It hasn't even been submitted for review yet, so I can make that change before putting it up for review.
I made that change in my patch, there's just one TODO left you'll have to fix:
https://bugzilla.mozilla.org/attachment.cgi?id=643498&action=diff#a/toolkit/crashreporter/nsExceptionHandler.cpp_sec13

In CheckForLastRunCrash, the contents of the lastCrashFile will be UTF-16, and I didn't handle that case because I was lazy. :)
Attached patch reporter patch v.1 (obsolete) — Splinter Review
as things stand currently, will update once bug 761909 lands.
Assignee: nobody → jmathies
Attached patch front end patch (obsolete) — Splinter Review
improvements on the front end:
- reporting tied to a pref
- delayed submit
- misc. nits fixed
Attachment #644363 - Attachment is obsolete: true
Attachment #644362 - Attachment is obsolete: true
Comment on attachment 645005 [details] [diff] [review]
reporter patch [checked-in on mc]

Really simple change on top of your updated patch. Note XRE_GetWindowsEnvironment() landed on mc a while back so it's safe to call here. Although there's no way it can return anything but Type_Desktop for the time being.
Attachment #645005 - Flags: review?(ted.mielczarek)
Whiteboard: completed-elm
Comment on attachment 645005 [details] [diff] [review]
reporter patch [checked-in on mc]

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

Glad to see this patch was so tiny on top of the other one. :)
Attachment #645005 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted] from comment #21)
> Glad to see this patch was so tiny on top of the other one. :)

yep your updated patch made this easy. I've tested this in metro with the front end changes and everything is working as expected.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b3dbee10d9
Whiteboard: completed-elm → [completed-elm][leave open]
Attachment #645005 - Attachment description: reporter patch → reporter patch [checked-in on mc]
Attachment #644391 - Attachment description: front end patch → front end patch [checked-in on elm]
Blocks: 831972
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.