Closed
Bug 898370
Opened 11 years ago
Closed 11 years ago
Change - Disable crash reporting when running with -metrodesktop
Categories
(Firefox for Metro Graveyard :: General, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 25
People
(Reporter: jimm, Assigned: jimm)
References
Details
(Whiteboard: feature=change c=tbd u=tbd p=1)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•11 years ago
|
||
Assignee: nobody → jmathies
Attachment #781655 -
Flags: review?(netzen)
Assignee | ||
Comment 2•11 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•11 years ago
|
||
We don't save out a headless report either, the crash reporter dialog comes up.
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #781655 -
Attachment is obsolete: true
Attachment #781666 -
Flags: review?(netzen)
Comment 5•11 years ago
|
||
Hey Jim, can you provide a point value estimate.
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 6•11 years ago
|
||
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•11 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•11 years ago
|
||
Attachment #781666 -
Attachment is obsolete: true
Attachment #781676 -
Flags: review?(netzen)
Comment 9•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
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•11 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
Comment 12•11 years ago
|
||
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•11 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.
Comment 14•11 years ago
|
||
> I realized we don't use -enable-metro in official builds
oh ya, haha
Comment 15•11 years ago
|
||
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•11 years ago
|
||
Attachment #781676 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 17•11 years ago
|
||
Keywords: checkin-needed
Comment 18•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Updated•11 years ago
|
Comment 20•11 years ago
|
||
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•11 years ago
|
||
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!
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•