898370 - Change - Disable crash reporting when running with -metrodesktop

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Jim Mathies [:jimm]]

Looking at some of our top crashes, #3 appears to be metrodesktop specific. I don't think we're interested in desktop crashes, and we don't need desktop polluting our "real" crash reports. So when running in metrodesktop mode, I'd like to disable crash reporter.

https://crash-stats.mozilla.com/report/list?product=MetroFirefox&range_value=7&range_unit=days&date=2013-07-26&signature=mozalloc_abort%28char+const*+const%29+|+NS_DebugBreak+|+mozilla%3A%3Alayers%3A%3APCompositorChild%3A%3ASendPLayerTransactionConstructor%28mozilla%3A%3Alayers%3A%3APLayerTransactionChild*%2C+mozilla%3A%3Alayers%3A%3ALayersBackend+const%26%2C+unsigned+__int64+const%26%2C+mozilla%3A%3Alayers%3A%3ATextureFa...&version=MetroFirefox%3A25.0a1

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 781655 [details] [diff] [review]
patch

Hmm, actually, this may not work. The crash happens on startup in desktop mode, which saves the headless report out to the metro profile. Then the user boots up in metro mode and submits the report. We would need to disable crashreporter much earlier than browser startup to prevent the report from getting written.

Comment 3

[Jim Mathies [:jimm]]

We don't save out a headless report either, the crash reporter dialog comes up.

Comment 4

[Jim Mathies [:jimm]]

Attachment: fix

Comment 5

[Marco Mucci [:MarcoM]]

Hey Jim, can you provide a point value estimate.

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 781666 [details] [diff] [review]
fix

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

::: browser/app/nsBrowserApp.cpp
@@ +241,5 @@
>        for (int idx = 1; idx < argc; idx++) {
>          if (IsArg(argv[idx], "metrodesktop")) {
>            metroOnDesktop = true;
> +          // Disable crash reporting when running in metrodesktop mode.
> +          _putenv_s("MOZ_CRASHREPORTER_DISABLE", "1");

I think you need to use putenv ere instead since this code can run from any platform, not only Windows.

Comment 7

[Jim Mathies [:jimm]]

Ah yes, good catch!

Comment 8

[Jim Mathies [:jimm]]

Attachment: fix

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 781676 [details] [diff] [review]
fix

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

Pretty sure this won't build on some platforms because putenv requires a non-const char* on some platforms.
Please pass in a buffer with this value set or else const_cast, but I think I prefer the former.

Comment 10

[Brian R. Bondy [:bbondy]]

See here: https://bugzilla.mozilla.org/show_bug.cgi?id=797229#c3
You can r=me with an analogous same change, I'm not sure if it produces errors or warnings with the build config but we should change it to avoid both.

Comment 11

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> See here: https://bugzilla.mozilla.org/show_bug.cgi?id=797229#c3
> You can r=me with an analogous same change, I'm not sure if it produces
> errors or warnings with the build config but we should change it to avoid
> both.

I am kinda curious what happens, so I pushed to try just to see - 

https://tbpl.mozilla.org/?tree=Try&rev=5e66bf5ec1e6

Comment 12

[Brian R. Bondy [:bbondy]]

Looks like it will be fine, it'll produce a warning at least one some platforms, and if the error level is high enough an error. As mentioned in comment 10 tough you can r=me with nits addressed.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #12)
> Looks like it will be fine, it'll produce a warning at least one some
> platforms, and if the error level is high enough an error. As mentioned in
> comment 10 tough you can r=me with nits addressed.

I'll go ahead and update. Besides I realized we don't use -enable-metro in official builds so the push to try was basically pointless.

Comment 14

[Brian R. Bondy [:bbondy]]

>  I realized we don't use -enable-metro in official builds 

oh ya, haha

Comment 15

[Brian R. Bondy [:bbondy]]

well I guess it would still be compiled code regardless though so I think it showed it's a warning on some platforms and not an error.

Comment 16

[Jim Mathies [:jimm]]

Attachment: export w/nits addressed

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/999f228c8ef5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/999f228c8ef5

Comment 19

[Jim Mathies [:jimm]]

Dropped off the map 7/27.

Comment 20

[Kamil Jozwiak [:kjozwiak]]

not really sure on how to test this for iteration #12. Ran Firefox Metro via -metrodesktop but not sure where to check if the crashes are being sent over. Will mark this as passed for this iteration as Jim mentioned in comment #19 that the crashes have fallen off the map.

If there's some specific way to test this, could someone please add some steps/notes?

Comment 21

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Change" for iteration #13. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-08-31-03-02-24-mozilla-central/

As stated in comment #20, not really sure how this can be tested. If Jim sees something out of the ordinary, I am sure he will create a new ticket!