Change - Disable crash reporting when running with -metrodesktop

VERIFIED FIXED in Firefox 25

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 25
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=change c=tbd u=tbd p=1)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Updated

6 years ago
Blocks: 886892
(Assignee)

Comment 1

6 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attachment #781655 - Flags: review?(netzen)
(Assignee)

Comment 2

6 years ago
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.
Attachment #781655 - Flags: review?(netzen)
(Assignee)

Comment 3

6 years ago
We don't save out a headless report either, the crash reporter dialog comes up.
(Assignee)

Comment 4

6 years ago
Posted patch fix (obsolete) — Splinter Review
Attachment #781655 - Attachment is obsolete: true
Attachment #781666 - Flags: review?(netzen)
Hey Jim, can you provide a point value estimate.
Blocks: metrov1it11
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
Flags: needinfo?(jmathies)
Priority: -- → P2
QA Contact: jbecerra
Summary: Disable crash reporting when running with -metrodesktop → Change - Disable crash reporting when running with -metrodesktop
Whiteboard: feature=change c=tbd u=tbd p=0
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.
Attachment #781666 - Flags: review?(netzen)
(Assignee)

Comment 7

6 years ago
Ah yes, good catch!
Flags: needinfo?(jmathies)
Whiteboard: feature=change c=tbd u=tbd p=0 → feature=change c=tbd u=tbd p=1
(Assignee)

Comment 8

6 years ago
Posted patch fix (obsolete) — Splinter Review
Attachment #781666 - Attachment is obsolete: true
Attachment #781676 - Flags: review?(netzen)
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.
Attachment #781676 - Flags: review?(netzen)
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.
(Assignee)

Comment 11

6 years ago
(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
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.
(Assignee)

Comment 13

6 years ago
(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.
>  I realized we don't use -enable-metro in official builds 

oh ya, haha
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.
(Assignee)

Comment 16

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

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/999f228c8ef5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Blocks: metrov1it12
No longer blocks: metrov1it11
(Assignee)

Comment 19

6 years ago
Dropped off the map 7/27.
Status: RESOLVED → VERIFIED
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?
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!
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.