741741 - Crashreporter UI shows up on desktop for metro applications

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Jim Mathies [:jimm]]

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.

Comment 1

[(not currently active) Ted Mielczarek]

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).

Comment 2

[Jim Mathies [:jimm]]

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.

Comment 3

[(not currently active) Ted Mielczarek]

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.

Comment 4

[Justin Dolske [:Dolske]]

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.

Comment 5

[Jim Mathies [:jimm]]

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.

Comment 6

[Jim Mathies [:jimm]]

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.

Comment 7

[(not currently active) Ted Mielczarek]

This sounds pretty similar to what we want to do for B2G, actually. See bug 761909.

Comment 8

[Jim Mathies [:jimm]]

(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.)

Comment 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[Jim Mathies [:jimm]]

(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)

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Jim Mathies [:jimm]]

(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.

Comment 13

[Jim Mathies [:jimm]]

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?

Comment 14

[(not currently active) Ted Mielczarek]

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.

Comment 15

[(not currently active) Ted Mielczarek]

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. :)

Comment 16

[Jim Mathies [:jimm]]

Attachment: reporter patch v.1

as things stand currently, will update once bug 761909 lands.

Comment 17

[Jim Mathies [:jimm]]

Attachment: front end patch

Comment 18

[Jim Mathies [:jimm]]

Attachment: front end patch [checked-in on elm]

improvements on the front end:
- reporting tied to a pref
- delayed submit
- misc. nits fixed

Comment 19

[Jim Mathies [:jimm]]

Attachment: reporter patch [checked-in on mc]

Comment 20

[Jim Mathies [:jimm]]

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.

Comment 21

[(not currently active) Ted Mielczarek]

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. :)

Comment 22

[Jim Mathies [:jimm]]

(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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6b3dbee10d9